shangxinli commented on PR #776: URL: https://github.com/apache/iceberg-cpp/pull/776#issuecomment-4784771856
**Force-pushed a redesign in response to a codex review pass.** The first revision extended `SnapshotUpdate` directly and reimplemented half of the merging pipeline by hand. Codex flagged two real bugs: 1. **P0:** `ManifestFilterManager` was only constructed for `kData`, and `Apply()` returned `new_data + filtered_data`. Existing delete manifests from the parent snapshot were silently dropped. Any table with positional / equality deletes outside the replaced partitions would lose them on the first `ReplacePartitions` commit — data corruption, not a style nit. 2. **P3:** `Summary()` cleared `summary_` without replaying `custom_summary_properties_`, so caller-supplied summary entries vanished on commit retry. Both stem from the same root cause: choosing `SnapshotUpdate` as the base. Java's `ReplacePartitions` extends `MergingSnapshotProducer`, which gives you data+delete filter managers, merge managers, the cached `custom_summary_properties_` map, and access to the protected `ValidateAddedDataFiles` / `ValidateNoNewDeleteFiles` helpers — exactly what we need. Rewrote `ReplacePartitions` to extend `MergingSnapshotUpdate`. Net **-123 LOC**, both bugs gone, `Validate()` is now actually wired (no longer a TODO stub). One small companion change: `MergingSnapshotUpdate::SetSummaryProperty` is promoted from `private` to `protected` so derived classes can stash custom summary entries through the cached-rebuild path. No behavioral change to existing subclasses. Local: clean build, `table_update_test` passes (17.89s). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
