From 4d1ef6e466b1c9c93490048b8f3196ad20ef1c08 Mon Sep 17 00:00:00 2001 From: Zhao Junwang Date: Wed, 21 Oct 2026 23:49:21 +0800 Subject: [PATCH] refactor: keep snapshot update operation hooks protected Align operation override visibility across merge append, delete, overwrite, replace partitions, rewrite files, and row delta updates. Validate operation classification through committed snapshot summaries in the existing tests. --- src/iceberg/test/overwrite_files_test.cc | 23 ++++++++++++++------- src/iceberg/test/replace_partitions_test.cc | 7 ++++++- src/iceberg/update/delete_files.h | 2 +- src/iceberg/update/merge_append.h | 1 + src/iceberg/update/overwrite_files.h | 2 +- src/iceberg/update/replace_partitions.h | 2 +- src/iceberg/update/rewrite_files.h | 2 +- src/iceberg/update/row_delta.h | 2 +- 8 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/iceberg/test/overwrite_files_test.cc b/src/iceberg/test/overwrite_files_test.cc index 6cac62e97d..7f55759b36 100644 --- a/src/iceberg/test/overwrite_files_test.cc +++ b/src/iceberg/test/overwrite_files_test.cc @@ -167,13 +167,12 @@ TEST_F(OverwriteFilesTest, DeleteAndAddCommit) { ICEBERG_UNWRAP_OR_FAIL(auto op, NewOverwrite()); op->DeleteFile(file_a_); op->AddFile(file_b_); - const std::string expected_operation = op->operation(); EXPECT_THAT(op->Commit(), IsOk()); EXPECT_THAT(table_->Refresh(), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); - EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kOperation), expected_operation); - EXPECT_EQ(expected_operation, DataOperation::kOverwrite); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kOperation), + DataOperation::kOverwrite); EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDataFiles), "1"); EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedDataFiles), "1"); } @@ -196,7 +195,6 @@ TEST_F(OverwriteFilesTest, EmptyCommit) { CommitFileA(); ICEBERG_UNWRAP_OR_FAIL(auto op, NewOverwrite()); - EXPECT_EQ(op->operation(), DataOperation::kOverwrite); EXPECT_THAT(op->Commit(), IsOk()); EXPECT_THAT(table_->Refresh(), IsOk()); @@ -283,11 +281,12 @@ TEST_F(OverwriteFilesTest, BulkDeleteCommit) { ICEBERG_UNWRAP_OR_FAIL(auto op, NewOverwrite()); op->DeleteFiles(data_files, delete_files); op->AddFile(file_b_); - EXPECT_EQ(op->operation(), DataOperation::kOverwrite); EXPECT_THAT(op->Commit(), IsOk()); EXPECT_THAT(table_->Refresh(), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kOperation), + DataOperation::kOverwrite); EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedDataFiles), "1"); EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDataFiles), "1"); } @@ -623,18 +622,26 @@ TEST_F(OverwriteFilesTest, OperationMatrix) { int index = 0; for (const auto& c : cases) { + SCOPED_TRACE("case index " + std::to_string(index)); + auto file_to_delete = + MakeDataFile("/data/tt_del" + std::to_string(index) + ".parquet", 1L); + if (c.delete_file || c.row_filter) { + CommitFastAppend(file_to_delete); + } ICEBERG_UNWRAP_OR_FAIL(auto op, NewOverwrite()); if (c.add) { op->AddFile(MakeDataFile("/data/tt_add" + std::to_string(index) + ".parquet", 1L)); } if (c.delete_file) { - op->DeleteFile( - MakeDataFile("/data/tt_del" + std::to_string(index) + ".parquet", 1L)); + op->DeleteFile(file_to_delete); } if (c.row_filter) { op->OverwriteByRowFilter(Expressions::Equal("x", Literal::Long(1L))); } - EXPECT_EQ(op->operation(), c.expected) << "case index " << index; + ASSERT_THAT(op->Commit(), IsOk()); + ASSERT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kOperation), c.expected); ++index; } } diff --git a/src/iceberg/test/replace_partitions_test.cc b/src/iceberg/test/replace_partitions_test.cc index aea5228224..fe319b063b 100644 --- a/src/iceberg/test/replace_partitions_test.cc +++ b/src/iceberg/test/replace_partitions_test.cc @@ -247,7 +247,12 @@ class ReplacePartitionsTest : public UpdateTestBase { TEST_F(ReplacePartitionsTest, OperationIsOverwrite) { ICEBERG_UNWRAP_OR_FAIL(auto op, NewReplace()); - EXPECT_EQ(op->operation(), DataOperation::kOverwrite); + op->AddFile(file_a_); + ASSERT_THAT(op->Commit(), IsOk()); + ASSERT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kOperation), + DataOperation::kOverwrite); } // A replace created from a transaction commits with the rest of the transaction. diff --git a/src/iceberg/update/delete_files.h b/src/iceberg/update/delete_files.h index 18756ff486..fabafcb836 100644 --- a/src/iceberg/update/delete_files.h +++ b/src/iceberg/update/delete_files.h @@ -88,9 +88,9 @@ class ICEBERG_EXPORT DeleteFiles : public MergingSnapshotUpdate { /// \return This DeleteFiles for method chaining. DeleteFiles& ValidateFilesExist(); + protected: std::string operation() override; - protected: Status Validate(const TableMetadata& current_metadata, const std::shared_ptr& snapshot) override; diff --git a/src/iceberg/update/merge_append.h b/src/iceberg/update/merge_append.h index 18503b3c17..51cca4b9fb 100644 --- a/src/iceberg/update/merge_append.h +++ b/src/iceberg/update/merge_append.h @@ -66,6 +66,7 @@ class ICEBERG_EXPORT MergeAppend : public MergingSnapshotUpdate { /// \return This MergeAppend for method chaining. MergeAppend& AppendManifest(const ManifestFile& manifest); + protected: std::string operation() override; private: diff --git a/src/iceberg/update/overwrite_files.h b/src/iceberg/update/overwrite_files.h index f76263cad2..85568a4fb7 100644 --- a/src/iceberg/update/overwrite_files.h +++ b/src/iceberg/update/overwrite_files.h @@ -177,9 +177,9 @@ class ICEBERG_EXPORT OverwriteFiles : public MergingSnapshotUpdate { /// \return This OverwriteFiles for method chaining. OverwriteFiles& CaseSensitive(bool case_sensitive); + protected: std::string operation() override; - protected: Status Validate(const TableMetadata& current_metadata, const std::shared_ptr& snapshot) override; diff --git a/src/iceberg/update/replace_partitions.h b/src/iceberg/update/replace_partitions.h index ee57b61247..027764ce60 100644 --- a/src/iceberg/update/replace_partitions.h +++ b/src/iceberg/update/replace_partitions.h @@ -114,9 +114,9 @@ class ICEBERG_EXPORT ReplacePartitions : public MergingSnapshotUpdate { /// \return Reference to this for method chaining ReplacePartitions& ValidateNoConflictingDeletes(); + protected: std::string operation() override; - protected: Status Validate(const TableMetadata& current_metadata, const std::shared_ptr& snapshot) override; diff --git a/src/iceberg/update/rewrite_files.h b/src/iceberg/update/rewrite_files.h index 283091c131..7cb5003c51 100644 --- a/src/iceberg/update/rewrite_files.h +++ b/src/iceberg/update/rewrite_files.h @@ -170,9 +170,9 @@ class ICEBERG_EXPORT RewriteFiles : public MergingSnapshotUpdate { /// \return this for method chaining RewriteFiles& ValidateFromSnapshot(int64_t snapshot_id); + protected: std::string operation() override; - protected: Status Validate(const TableMetadata& current_metadata, const std::shared_ptr& snapshot) override; diff --git a/src/iceberg/update/row_delta.h b/src/iceberg/update/row_delta.h index f6faf8c92e..474683b2a0 100644 --- a/src/iceberg/update/row_delta.h +++ b/src/iceberg/update/row_delta.h @@ -154,9 +154,9 @@ class ICEBERG_EXPORT RowDelta : public MergingSnapshotUpdate { /// \return This RowDelta for method chaining. RowDelta& ValidateNoConflictingDeleteFiles(); + protected: std::string operation() override; - protected: Status Validate(const TableMetadata& current_metadata, const std::shared_ptr& snapshot) override;