wgtmac commented on PR #560:
URL: https://github.com/apache/iceberg-cpp/pull/560#issuecomment-3959033157
## Review Report: PR #560
### 📄 File: `src/iceberg/util/snapshot_util.cc` &
`src/iceberg/util/snapshot_util_internal.h`
**Java Counterpart:**
`core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java`
* **Parity Check:** ⚠️
* **Discrepancy in `IsAncestorOf`**: The new implementation introduces
an early return `if (snapshot_id == ancestor_snapshot_id) return true;` before
verifying if `snapshot_id` actually exists.
* In Java, `isAncestorOf` delegates to `ancestorsOf(snapshotId, ...)`,
which strictly evaluates `Preconditions.checkArgument(start != null, "Cannot
find snapshot: %s", snapshotId);` before anything else.
* *Impact*: In C++, if queried about a non-existent `snapshot_id`,
`IsAncestorOf(invalid_id, invalid_id, lookup)` will silently return `true`,
whereas Java throws an `IllegalArgumentException`.
* *Fix*: Perform the lookup and validation of `snapshot_id` *before*
the equality check.
* **Style Check:** ✅
* The transition away from `std::ranges::any_of` on a fully
constructed `std::vector` of ancestors to an iterative, early-exit `while` loop
in `IsAncestorOf` and `IsParentAncestorOf` is excellent. It avoids unnecessary
heap allocations and redundant O(N) snapshot lookups.
* Adding overloads for `TableMetadata` cleans up the dependency chain
nicely.
* **Logic Check:** ✅
* Returning `nullptr` from the lambda within `AncestorsBetween`
perfectly aligns with Java's `Iterables.filter` early-termination behavior and
cleanly exits the `AncestorsOf` traversal loop without throwing errors.
* Loop safety (`current != nullptr`) and `NotFound` error forwarding
via `ICEBERG_ACTION_FOR_NOT_FOUND` are correctly implemented.
---
## Summary & Recommendation
* **Request Changes**: The PR is highly optimized and well-structured, but
the missing validation for `snapshot_id` in `IsAncestorOf` breaks strict parity
with Java and could mask invalid snapshot queries. Please move the `lookup`
validation to occur before the `snapshot_id == ancestor_snapshot_id` check.
--
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]
