fix: narrow Iceberg complex-null scan fallback to struct columns - #5732
fix: narrow Iceberg complex-null scan fallback to struct columns#5732ErikBPF wants to merge 2 commits into
Conversation
The scan-rule check that declines Iceberg scans carrying IS NULL / IS NOT NULL predicates on complex-type columns predates the current iceberg-rust pin. iceberg-rust's Arrow predicate visitor now evaluates null checks on list and map columns through arrow native is_null / is_not_null; only struct columns remain unsupported (project_column rejects Struct). The check was declining list-column null checks that Spark pushes below Generate (e.g. isnotnull(arr) under explode), forcing the whole scan back to Spark and cascading into JVM-side aggregate fallbacks. On a 24-query derived TPC-H benchmark (SF1, 3 runs, cold caches), removing the list false-positives eliminates 123 scan declines and restores native execution: Comet/Iceberg total 20.6s -> 16.9s (-17.9%), closing the gap to raw Parquet to within 4%.
Adds an end-to-end scan-rule regression test: IS NULL/IS NOT NULL pushed on list and map columns must keep the scan native (iceberg-rust Arrow predicate visitor projects them), while the same predicate on a struct column must still fall back to Spark (project_column rejects Struct).
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed 68f2f47eabee08ee10c9ef09caf7735907c343b7 against authoritative base b5a39c33a1b3bbe3693a0cf630007cd4c784358c. The change removes list/map columns from an existing whole-scan fallback for complex-column null predicates. It retains the top-level struct guard and adds a query test requiring native list/map scans and struct fallback. One P2 remains: the existing CometIcebergNativeSuite still asserts fallback for five affected list/map queries, including both direct IS NULL cases. Those expectations need to change with the behavior.
The actual safety path is different from the PR rationale. At pinned iceberg-rust 665c64e48e8d33797ecb1a421f327edd9b024879, schema accessors are still not built for list/map fields, and Reference::bind requires an accessor. Comet catches that bind failure and skips the residual. Thus this restores native scanning while the filter above the scan enforces the predicate. It does not establish working list/map predicate evaluation in iceberg-rust's Arrow visitor. Nested dotted references are also omitted by the existing residual serializer. I found no additional data-correctness defect in this narrowing.
The maintained Spark 3.5 and 4.0 branches evaluate IS NULL/IS NOT NULL from the value's nullness. Empty containers and containers holding null elements are non-null. Their InferFiltersFromGenerate rule does introduce IS NOT NULL for eligible array/map generator inputs. The matching Iceberg Java dependencies retain post-scan filters unless partition selection fully guarantees them, and include filter columns in the read projection. Normal list/map reads preserve the container arrays, including nested struct validity, through the unchanged reader path. Null struct parents, nested field access, field-ID projection, missing optional columns, partition metadata, and unsupported-type/default guards gain no new transformation in this PR. The retained direct-struct fallback is conservative and pre-existing. No new ANSI, overflow, or coercion branch is introduced. Maintained Spark 3.4 and 4.1 sources were unavailable, so source-level compatibility is not claimed for those versions.
Validation
The new test executes Spark and Comet queries, compares results, and inspects the executed native scan plan. Its three queries cover IS NOT NULL for a nullable list, map, and struct, with one populated row and one all-null row. It does not test IS NULL, empty containers, nested null parents, schema evolution, or prove native residual pushdown. Existing direct IS NULL and element/key tests provide useful coverage once their obsolete fallback assertions are corrected.
At the complete discussion/CI cutoff of 2026-09-06 02:39:49 UTC, there were no reviews, inline comments, or executed checks. CI, CodeQL, and Delta Contrib Build Gate were all action_required. I ran no local build, Scala/JNI tests, or benchmark and did not approve or rerun workflows. The merge commit has the assigned head/base parents and identical reviewed/supporting file blobs, but there is no executed CI checkout to credit.
Performance
The gate remains a plan-time schema/filter check, with a smaller candidate-column set and no new per-row work. Restoring native scans can avoid the broader fallback chain reported in issue #5731. The reported SF1 derived TPC-H improvement from 20.63 s to 16.93 s is author evidence that I did not reproduce or independently qualify. List/map residuals can still be skipped during binding, so native scan eligibility should not be presented as improved native predicate pruning. No new expression kernel warrants a generic expression microbenchmark.
Design
The localized gate change restores scan eligibility without altering filter enforcement, file-task planning, or reader ownership. Keeping the existing binding-failure path is essential to the source-level safety argument. The change should be paired with the existing integration tests' new expected behavior. The retained struct guard and string-based predicate matching are existing conservatism, not newly introduced abstractions.
Abstraction & complexity
The production change adds no helper, registry, or type hierarchy. A top-level StructType check is simple to follow, and recursive rejection of every struct inside a list/map would incorrectly conflate container nullness with element nullness. The new test reuses the established Spark-comparison and native-plan helpers. I found no additional abstraction or complexity issue.
| // evaluate through arrow's native is_null/is_not_null and are supported. | ||
| val complexColumns = readSchema | ||
| .filter(field => isComplexType(field.dataType)) | ||
| .filter(field => field.dataType.isInstanceOf[StructType]) |
There was a problem hiding this comment.
Correctness
[P2] Update the existing Iceberg fallback tests
Could you update the affected cases in CometIcebergNativeSuite along with this gate? The existing array/map IS NULL tests still call checkIcebergNativeScanFallback (lines 2358 and 2469), whose helper explicitly asserts that no CometIcebergNativeScanExec exists. Those assertions now reject the native scans this change enables. The array-element, whole-array equality, and map-key cases also retain fallback assertions based on the implicit NOT NULL restriction being removed here. This suite runs alongside CometFuzzIcebergSuite in the reader CI group, so adding the new test does not replace the conflicting expectations. Please convert the affected cases to native-scan assertions while retaining their Spark-result comparisons, and run that suite. This is a source-verified assertion conflict, not an observed CI failure: the current workflows are still action_required.
3 participants
Which issue does this PR close?
Closes #5731
Rationale for this change
The Iceberg scan-rule check that declines scans carrying
IS NULL/IS NOT NULLpredicates on complex-typed columns was written against an older iceberg-rust pin (#2528). At the current pin (#5651), iceberg-rust's Arrow predicate visitor evaluates null checks on list and map columns via arrow-nativeis_null/is_not_null; only struct columns are unsupported (project_columnrejectsDataType::Struct).The check therefore produces false positives on a very common pattern: Spark pushes
isnotnull(arr)belowGenerateforexplode(arr), which declined whole scans and cascaded into JVM-side aggregate fallbacks.What changes are included in this PR?
CometScanRule: the complex-null-check scan predicate now declines only struct-typed columns; list and map columns are admitted. Comment and fallback message updated to describe the actual iceberg-rust limitation (project_column).isComplexTypeimport.How are these changes tested?
CometScanRuleSuitepasses unchanged.array<struct>table, SF1, 3 runs, cold caches): eliminating the list false-positives removes 123 scan declines and restores native execution — Comet/Iceberg total 20.63 s → 16.93 s (−17.9%), with parity vs vanilla Spark verified on all queries (details in Iceberg scan falls back to Spark on IS NULL/IS NOT NULL over list/map columns (stale complex-type check) #5731).