Skip to content

Commit c9db9b7

Browse files
shangxinliclaude
andcommitted
test: strengthen ReplacePartitions assertions from second-pass review
- NoValidationSamePartitionConcurrentReplaceCommits: assert the replacement is the only live file after the idempotent commit, not just that it succeeded. - AllVoidSpecReplacesWholeTable: also assert added-data-files == 1 so the summary fully pins the all-void file as the sole survivor. - ReplaceCleansUpDeletionVectors: assert the exact DV path before and its absence after, instead of a raw live-delete count. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 8c71a01 commit c9db9b7

1 file changed

Lines changed: 24 additions & 11 deletions

File tree

src/iceberg/test/replace_partitions_test.cc

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -180,9 +180,9 @@ class ReplacePartitionsTest : public UpdateTestBase {
180180
return paths;
181181
}
182182

183-
// Number of live delete files (position/equality/DV) in the current snapshot.
184-
Result<int> LiveDeleteFileCount() {
185-
int count = 0;
183+
// Live (non-deleted) delete-file paths in the current snapshot's manifests.
184+
Result<std::vector<std::string>> LiveDeleteFilePaths() {
185+
std::vector<std::string> paths;
186186
ICEBERG_ASSIGN_OR_RAISE(auto snapshot, table_->current_snapshot());
187187
SnapshotCache cache(snapshot.get());
188188
ICEBERG_ASSIGN_OR_RAISE(auto manifests, cache.DeleteManifests(file_io_));
@@ -192,9 +192,13 @@ class ReplacePartitionsTest : public UpdateTestBase {
192192
ICEBERG_ASSIGN_OR_RAISE(auto reader,
193193
ManifestReader::Make(manifest, file_io_, schema_, spec));
194194
ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->LiveEntries());
195-
count += static_cast<int>(entries.size());
195+
for (const auto& entry : entries) {
196+
if (entry.data_file) {
197+
paths.push_back(entry.data_file->file_path);
198+
}
199+
}
196200
}
197-
return count;
201+
return paths;
198202
}
199203

200204
Result<std::unique_ptr<ReplacePartitions>> NewReplace() {
@@ -426,6 +430,13 @@ TEST_F(ReplacePartitionsTest, NoValidationSamePartitionConcurrentReplaceCommits)
426430
op->AddFile(MakeDataFile("/data/replacement_x1.parquet", /*partition_x=*/1L));
427431
op->ValidateFromSnapshot(first_id); // no ValidateNoConflicting* calls
428432
EXPECT_THAT(op->Commit(), IsOk());
433+
434+
EXPECT_THAT(table_->Refresh(), IsOk());
435+
// The concurrent file is replaced along with the original; only the
436+
// replacement is live in partition 1.
437+
ICEBERG_UNWRAP_OR_FAIL(auto live, LiveDataFilePaths());
438+
EXPECT_THAT(live, ::testing::UnorderedElementsAre(table_location_ +
439+
"/data/replacement_x1.parquet"));
429440
}
430441

431442
// An all-void spec (a partition field using the void transform) is unpartitioned,
@@ -442,7 +453,9 @@ TEST_F(ReplacePartitionsTest, AllVoidSpecReplacesWholeTable) {
442453

443454
EXPECT_THAT(table_->Refresh(), IsOk());
444455
ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot());
445-
// Both partitioned files replaced by the single all-void file.
456+
// The single all-void file is added and both partitioned files are replaced,
457+
// leaving it as the only live file.
458+
EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDataFiles), "1");
446459
EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedDataFiles), "2");
447460
EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kTotalDataFiles), "1");
448461
}
@@ -494,17 +507,17 @@ TEST_F(ReplacePartitionsV3Test, ReplaceCleansUpDeletionVectors) {
494507
row_delta->AddDeletes(dv);
495508
EXPECT_THAT(row_delta->Commit(), IsOk());
496509
EXPECT_THAT(table_->Refresh(), IsOk());
497-
ICEBERG_UNWRAP_OR_FAIL(auto before, LiveDeleteFileCount());
498-
EXPECT_EQ(before, 1);
510+
ICEBERG_UNWRAP_OR_FAIL(auto before, LiveDeleteFilePaths());
511+
EXPECT_THAT(before, ::testing::ElementsAre(dv->file_path));
499512

500513
ICEBERG_UNWRAP_OR_FAIL(auto op, NewReplace());
501514
op->AddFile(MakeDataFile("/data/file_a_new.parquet", /*partition_x=*/1L));
502515
EXPECT_THAT(op->Commit(), IsOk());
503516
EXPECT_THAT(table_->Refresh(), IsOk());
504517

505-
// The DV in the replaced partition is dropped along with the data.
506-
ICEBERG_UNWRAP_OR_FAIL(auto after, LiveDeleteFileCount());
507-
EXPECT_EQ(after, 0);
518+
// The DV referencing file_a in the replaced partition is dropped with the data.
519+
ICEBERG_UNWRAP_OR_FAIL(auto after, LiveDeleteFilePaths());
520+
EXPECT_THAT(after, ::testing::IsEmpty());
508521
}
509522

510523
} // namespace iceberg

0 commit comments

Comments
 (0)