Skip to content

Commit 08914ef

Browse files
zjw1111claude
andcommitted
test: improve spill test clarity and rename compaction to merge in comments
- Rename compact/compaction to merge in spill-related comments and test names - Refine Write/FlushMemory return value variable names for clearer semantics - Add Write 3 case to TestSpillDiskQuotaEnforcement - Add comments in sort_buffer_test.cpp for Clear and empty batch behavior - Fix ASSERT_LE to ASSERT_EQ for deterministic spill file counts in inte tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent b3cecf0 commit 08914ef

5 files changed

Lines changed: 121 additions & 104 deletions

File tree

src/paimon/core/mergetree/merge_tree_writer_test.cpp

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1112,7 +1112,7 @@ TEST_F(MergeTreeWriterTest, TestSpillWithSameKeyDeduplicate) {
11121112
WriteBatch(batch1, /*row_kinds=*/{}, merge_writer.get());
11131113
WriteBatch(batch2, /*row_kinds=*/{}, merge_writer.get());
11141114
// WRITE_BUFFER_SIZE=1 causes UpdateSpillParameters() to clamp actual_max_fan_in_ to 2,
1115-
// triggering leveled compaction after 2 spill files are produced, merging them into 1.
1115+
// triggering leveled merge after 2 spill files are produced, merging them into 1.
11161116
ASSERT_EQ(1u, TestHelper::CountChannelFiles(file_system_, dir->Str() + "/tmp"));
11171117

11181118
std::shared_ptr<arrow::Array> batch3 =
@@ -1184,7 +1184,7 @@ TEST_F(MergeTreeWriterTest, TestIntermediateMergeSpillFileBound) {
11841184
ASSERT_EQ(1u, TestHelper::CountChannelFiles(file_system_, dir->Str() + "/tmp"));
11851185

11861186
WriteBatch(batch2, /*row_kinds=*/{}, merge_writer.get());
1187-
// Level 0: [A,B] hits max_fan_in=2, compaction -> Level 0: [], Level 1: [C], total = 1
1187+
// Level 0: [A,B] hits max_fan_in=2, merge -> Level 0: [], Level 1: [C], total = 1
11881188
ASSERT_EQ(1u, TestHelper::CountChannelFiles(file_system_, dir->Str() + "/tmp"));
11891189

11901190
WriteBatch(batch3, /*row_kinds=*/{}, merge_writer.get());
@@ -1433,22 +1433,18 @@ TEST_F(MergeTreeWriterTest, TestMultiplePrepareCommitWithSpill) {
14331433
ASSERT_OK(merge_writer->Close());
14341434
}
14351435

1436-
TEST_P(MergeTreeWriterTest, TestSpillWithIOException) {
1436+
TEST_F(MergeTreeWriterTest, TestSpillWithIOException) {
14371437
// This test exercises IO error injection across all spill-related code paths:
14381438
// 1. SpillToDisk (spill write)
1439-
// 2. MergeAndReplaceFiles (intermediate compaction triggered by LeveledMerger)
1439+
// 2. MergeAndReplaceFiles (intermediate merge triggered by SpillFileMerger)
14401440
// 3. RunFinalCleanupIfNeeded (final merge in CreateReaders/PrepareCommit)
14411441
// 4. FlushWriteBuffer (reading merged spill data back + writing output data file)
14421442
//
14431443
// WRITE_BUFFER_SIZE=1 causes actual_max_fan_in_ to be clamped to 2, so every
1444-
// 2 spill files at the same level triggers compaction.
1444+
// 2 spill files at the same level triggers merge.
14451445
// Each WriteBatch with 2 rows fills the in-memory buffer (write_buffer_size param
14461446
// in InMemorySortBuffer is controlled via WRITE_BUFFER_SIZE in CoreOptions for
14471447
// MergeTreeWriter::Create), triggering a spill.
1448-
if (!GetParam()) {
1449-
return;
1450-
}
1451-
14521448
ASSERT_OK_AND_ASSIGN(CoreOptions options,
14531449
CoreOptions::FromMap({{Options::FILE_FORMAT, "orc"},
14541450
{Options::WRITE_BUFFER_SIZE, "1"},
@@ -1481,7 +1477,7 @@ TEST_P(MergeTreeWriterTest, TestSpillWithIOException) {
14811477
auto b1 = CreateBatch(batch1, {});
14821478
CHECK_HOOK_STATUS(merge_writer->Write(std::move(b1)), i);
14831479

1484-
// Batch 2: triggers spill file 2 → intermediate compaction (merge 2 files into 1)
1480+
// Batch 2: triggers spill file 2 → intermediate merge (merge 2 files into 1)
14851481
std::shared_ptr<arrow::Array> batch2 =
14861482
arrow::ipc::internal::json::ArrayFromJSON(value_type_, R"([
14871483
["Alice", 10, 0, 10.0],
@@ -1501,8 +1497,8 @@ TEST_P(MergeTreeWriterTest, TestSpillWithIOException) {
15011497
auto b3 = CreateBatch(batch3, {});
15021498
CHECK_HOOK_STATUS(merge_writer->Write(std::move(b3)), i);
15031499

1504-
// Batch 4: triggers spill file at level 0 → another compaction at level 0,
1505-
// then level 1 has 2 files → compaction at level 1 as well.
1500+
// Batch 4: triggers spill file at level 0 → another merge at level 0,
1501+
// then level 1 has 2 files → merge at level 1 as well.
15061502
std::shared_ptr<arrow::Array> batch4 =
15071503
arrow::ipc::internal::json::ArrayFromJSON(value_type_, R"([
15081504
["Charlie", 30, 0, 30.0],

src/paimon/core/mergetree/sort_buffer_test.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,9 +300,11 @@ TEST_F(SortBufferTest, TestInMemorySortBufferEstimateMemoryUseForEachRow) {
300300
ASSERT_EQ(buffer.GetEstimateMemoryUseForEachRow(), cached_memory_use_per_row);
301301
}
302302

303+
// Clear does not reset the estimated per-row memory usage.
303304
buffer.Clear();
304305
ASSERT_EQ(buffer.GetEstimateMemoryUseForEachRow(), cached_memory_use_per_row);
305306

307+
// Verify behavior when writing an empty batch.
306308
std::shared_ptr<arrow::Array> empty_array =
307309
arrow::ipc::internal::json::ArrayFromJSON(value_type_, R"([])").ValueOrDie();
308310
::ArrowArray c_array;

src/paimon/core/mergetree/spill_file_merger_test.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -239,15 +239,15 @@ TEST_F(SpillFileMergerTest, SetMaxFanInAffectsMerge) {
239239
ASSERT_EQ(merger.GetAllFiles().size(), 1);
240240
}
241241

242-
TEST_F(SpillFileMergerTest, SetMaxFanInToLargerValueSuppressesCompaction) {
242+
TEST_F(SpillFileMergerTest, SetMaxFanInToLargerValueSuppressesMerge) {
243243
SpillFileMerger merger(3);
244244

245245
merger.AddFile(MakeFile(1, 100));
246246
merger.AddFile(MakeFile(2, 200));
247247
merger.AddFile(MakeFile(3, 300));
248248

249-
// At fan_in=3, compaction should trigger
250-
// But first, increase fan_in to 5 before running compaction
249+
// At fan_in=3, merge should trigger
250+
// But first, increase fan_in to 5 before running merge
251251
merger.SetMaxFanIn(5);
252252
ASSERT_OK(merger.RunMergeIfNeeded(CreateMockMergeFn()));
253253
ASSERT_EQ(merge_call_count_, 0);
@@ -256,19 +256,19 @@ TEST_F(SpillFileMergerTest, SetMaxFanInToLargerValueSuppressesCompaction) {
256256
auto files = merger.GetAllFiles();
257257
ASSERT_EQ(files.size(), 3);
258258

259-
// Add more files up to 5, still no compaction
259+
// Add more files up to 5, still no merge
260260
merger.AddFile(MakeFile(4, 400));
261261
ASSERT_OK(merger.RunMergeIfNeeded(CreateMockMergeFn()));
262262
ASSERT_EQ(merge_call_count_, 0);
263263
ASSERT_EQ(merger.GetAllFiles().size(), 4);
264264

265-
// Add 5th file, now compaction triggers
265+
// Add 5th file, now merge triggers
266266
merger.AddFile(MakeFile(5, 500));
267267
ASSERT_OK(merger.RunMergeIfNeeded(CreateMockMergeFn()));
268268
ASSERT_EQ(merge_call_count_, 1);
269269
}
270270

271-
TEST_F(SpillFileMergerTest, CompactionOnlyTakesFanInFilesFromLevel) {
271+
TEST_F(SpillFileMergerTest, MergeOnlyTakesFanInFilesFromLevel) {
272272
SpillFileMerger merger(3);
273273

274274
// Add 5 files to level 0 (exceeds fan_in=3)

0 commit comments

Comments
 (0)