Skip to content

refactor: protect merging snapshot update hooks - #885

Draft
manuzhang wants to merge 1 commit into
apache:mainfrom
manuzhang:agent/protect-merging-snapshot-update-hooks
Draft

refactor: protect merging snapshot update hooks#885
manuzhang wants to merge 1 commit into
apache:mainfrom
manuzhang:agent/protect-merging-snapshot-update-hooks

Conversation

@manuzhang

Copy link
Copy Markdown
Member

Summary

  • move MergingSnapshotUpdate's Apply, CleanUncommitted, and Summary overrides from public to protected
  • keep the destructor public
  • explicitly re-expose the protected hooks only in test subclasses that need white-box access

Why

These methods implement internal extension hooks that are already protected in SnapshotUpdate. Declaring the overrides public unnecessarily widens the MergingSnapshotUpdate API. Keeping them protected aligns the derived class with the base-class contract while preserving access for subclasses and virtual dispatch from SnapshotUpdate.

External code that directly called these internal hooks through a concrete merging update will need to use the public snapshot-update workflow instead. The virtual function layout and runtime behavior are unchanged.

Validation

  • clang-format --dry-run --Werror src/iceberg/update/merging_snapshot_update.h src/iceberg/test/merging_snapshot_update_test.cc
  • cmake --build build --target table_update_test -j2
  • build/src/iceberg/test/table_update_test --gtest_filter='MergingSnapshotUpdateTest.*:MergingSnapshotUpdateV1Test.*' (79 tests passed)
  • git diff --check upstream/main...HEAD

Co-authored-by: Codex <codex@openai.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant