This is an automated email from the ASF dual-hosted git repository.
wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new 0aaed8d5 refactor: keep snapshot update operation hooks protected
(#1049)
0aaed8d5 is described below
commit 0aaed8d5236cf197590f86a1c854fb625e71c7fa
Author: Junwang Zhao <[email protected]>
AuthorDate: Fri Oct 23 15:43:42 2026 +0800
refactor: keep snapshot update operation hooks protected (#1049)
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 6cac62e9..7f55759b 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 aea52282..fe319b06 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 18756ff4..fabafcb8 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>& snapshot) override;
diff --git a/src/iceberg/update/merge_append.h
b/src/iceberg/update/merge_append.h
index 18503b3c..51cca4b9 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 f76263ca..85568a4f 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>& snapshot) override;
diff --git a/src/iceberg/update/replace_partitions.h
b/src/iceberg/update/replace_partitions.h
index ee57b612..027764ce 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>& snapshot) override;
diff --git a/src/iceberg/update/rewrite_files.h
b/src/iceberg/update/rewrite_files.h
index 283091c1..7cb5003c 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>& snapshot) override;
diff --git a/src/iceberg/update/row_delta.h b/src/iceberg/update/row_delta.h
index f6faf8c9..474683b2 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>& snapshot) override;
