test: verify MergingSnapshotUpdate commit reports - #884
Conversation
Co-authored-by: Codex <codex@openai.com>
There was a problem hiding this comment.
Pull request overview
This PR removes an outdated comment implying merge-based snapshot updates don’t emit commit events, and adds a focused regression test asserting that MergingSnapshotUpdate emits exactly one CommitReport containing key commit metadata and summary-derived metrics.
Changes:
- Removed stale TODO in
MergingSnapshotUpdateheader about lacking event/reporting support. - Added a new test that captures emitted
MetricsReportinstances and validates theCommitReportfields (table name, operation, snapshot/sequence IDs, and added file/record counts).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/iceberg/update/merging_snapshot_update.h | Removes an outdated TODO that no longer reflects the current metrics reporting behavior. |
| src/iceberg/test/merging_snapshot_update_test.cc | Adds regression coverage verifying merge-append snapshot commits emit a correct CommitReport. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// 6. Merge data manifests (via data_merge_manager_) | ||
| /// 7. Merge delete manifests (via delete_merge_manager_) | ||
| /// | ||
| /// TODO(Guotao): Java MergingSnapshotProducer overrides updateEvent() to return a |
There was a problem hiding this comment.
MetricsReporter covers the commit reporting use case, but it is not exactly the same as Java's event listener mechanism. Java handles event notification and metrics reporting separately. If we don't plan to support a general event listener in C++, removing this TODO is fine.
| EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedRecords), "100"); | ||
| } | ||
|
|
||
| TEST_F(MergingSnapshotUpdateTest, CommitReportsCreatedSnapshot) { |
What changed
MergingSnapshotUpdateevent-reporting TODOCommitReportWhy
The TODO predates commit metrics integration and still says C++ has no equivalent reporting mechanism. Snapshot updates now report successful commits through
MetricsReporter, butMergingSnapshotUpdatedid not have direct regression coverage for that behavior.This keeps reporting centralized in
SnapshotUpdatewhile testing the contract for merge-based updates.Validation
cmake --build build --target table_update_test -j2build/src/iceberg/test/table_update_test --gtest_filter='MergingSnapshotUpdateTest.*'(79 tests passed)clang-format --dry-run --Werror src/iceberg/test/merging_snapshot_update_test.cc src/iceberg/update/merging_snapshot_update.hgit diff --check