Repository navigation
Conversation
sunchao
left a comment
There was a problem hiding this comment.
No introduced P1/P2 issues found within this review.
Reviewed the entire four-file diff at 3a2a1f593555fdb5413a6eb774f1544c5959b801 against 3355602e268819750884aa0ce0dd47002c6ea580. The PR is not a draft and contains no stacked prerequisites. The snapshot and live discussion checks contained no reviews, issue comments, inline comments, or review threads.
Summary
- Prior state and problem: A primitive equality-delete key nested inside a struct passed Comet’s validation, but the pinned
iceberg-rusttransformer could not resolve it and failed withfield not found. - Design approach: Collect top-level field IDs from the current schema and schema history, then fall back when a resolved equality-delete key is absent from that set.
- Correctness: Verified that
Schema.columns()returns top-level fields whilefindField(id)resolves nested fields. The native loader’s top-level lookup confirms the need for this guard. Focused checks distinguished nested keys from top-level keys, including dropped fields and mixed deletes. No introduced P1/P2 correctness issue was identified. - Compatibility analysis: Checked Iceberg Java’s schema and Spark reader implementations in 1.5.2, 1.8.1, 1.10.0, and 1.11.0, covering the dependencies used by Comet’s Spark 3.4–4.2 profiles. The added calls use existing reflection helpers. Compared with latest release branch
branch-1.1at89c729a9697467e8f8d89e3eadf1797a75e2328c: the affected validation previously admitted nested keys, so this is an intended fallback change. - Key design decisions: Checking field IDs avoids confusing same-named fields. Including historical schemas preserves native eligibility for dropped top-level keys. Existing unsupported-type and unresolved-ID checks remain effective.
- Implementation sketch: Eleven production lines extend
deleteFileTypesSupported. The test helper accepts explicit equality-field IDs while preserving its previous default. The regression writes a genuine nested-key delete, checks the result and fallback, and updates the user guide. - Performance: Scans with delete files gain one traversal of current and historical top-level columns and set lookups for equality keys. This adds planning work, including for positional-only deletes, but no per-row processing. No evidence-backed P1/P2 performance regression was identified. Planning overhead was not benchmarked.
- Design: Extending the existing validation block is a straightforward way to keep an unsupported case out of native execution until the upstream reader supports it.
- Abstraction & complexity: Existing reflection helpers provide the necessary schema inspection. The optional test-helper argument enables nested-key fixtures without introducing a production abstraction or changing existing callers.
- Behavioral changes worth calling out: Nested-key equality deletes now use Spark’s reader. Top-level equality keys and positional deletes retain their previous eligibility. Spark/Iceberg’s existing limitation when the projection omits the nested equality key remains. The regression deliberately projects that key.
- Suggested improvements: No additional change met the P1/P2 reporting bar. Full integration validation remains outstanding as described below.
Routed skills: review-comet-pr. No sibling skill applies to this scan-planning fallback. Repository AGENTS.md and the relevant contributor guidance were read.
Exact-head CI: Comet CI, Check PR Title, and CodeQL report action_required. The label job remains queued. There is no completed build or test verdict for this head.
Validation: Compiled the verbatim base/head delete-validation blocks with the current reflection implementation and modified test helper in a disposable harness. Eleven cases passed on each of Iceberg 1.10.0 and 1.11.0, with a base/head regression control. A real Parquet equality-delete reproduction on Spark 4.1.3/Iceberg 1.11.0 returned [90,420,4470]. Its planned delete files were accepted by the base guard and rejected by the head guard with the expected reason. git diff --check passed.
Limits: These focused checks did not execute the complete CometIcebergNativeSuite, the full Comet fallback plan, or the Spark/Iceberg integration matrix. No fresh native build or performance benchmark was run. Project source remained unchanged, and nothing was published to GitHub.
andygrove
left a comment
There was a problem hiding this comment.
Thanks for the fix @yunchipang. This is a clean way to keep the nested-key case out of the native scan until iceberg-rust#3453 lands.
I compared against branch-1.1. The nested-key failure is the same there, so this is an intended change from a native field not found error to a fallback that returns Spark's answer. Top-level keys, dropped top-level keys and struct or variant keys behave as before. Checking the schema history for top-level ids is the right call. If reflection fails, the key looks nested and the scan falls back, so that failure mode is safe.
The test arithmetic checks out (90, 420 and 4470) and projecting the key is the right way to get a valid comparison with Spark.
One request: could we get the run-iceberg-tests label on this once the workflows are approved, so the Iceberg version matrix confirms it before merge?
|
@andygrove thanks! i don't think i have the permission to add labels though. could you add |
Which issue does this PR close?
Closes #6782.
Rationale for this change
If an Iceberg equality delete is keyed on a nested field such as
s.k, every native scan of the table fails withfield not found. iceberg-rust only looks up top-level columns. The upstream fix is apache/iceberg-rust#3453. Until Comet picks it up, this case should fall back to Spark.What changes are included in this PR?
CometScanRule: fall back when an equality delete's key is not a top-level column in any of the table's schemas.CometEqualityDeletes: a test helper option to key a delete on a nested field.iceberg.md: list this case as a fallback instead of a known failure.How are these changes tested?
A new test in
CometIcebergNativeSuitekeys a delete ons.k. It checks that the result matches Spark and that the scan falls back. It fails without the fix. The existing equality delete tests still pass.