test: cover ambiguous exact nested Parquet field matches - #5751
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
The existing production guard already requires an unambiguous resolver match before retaining DataFusion's CastExpr; this PR closes the missing exact-name regression coverage without changing that guard. With CAFÉ and café siblings, requesting café remains ambiguous in case-insensitive mode even though one sibling matches exactly. I checked ParquetReadSupport.clipParquetGroupFields and its duplicate-field error against maintained Spark branch-3.5-openai (5947fd6e74a1) and branch-4.0-openai (03f28fc43180): both group names with Locale.ROOT lowercasing and reject multiple matches, while case-sensitive matching uses the exact name.
The Rust test covers 12 configurations: ASCII/non-ASCII names, direct struct/nested struct/list-of-struct, and both case-sensitivity settings. It checks the predicate and the actual expression-adapter rewrite: ambiguity selects CometCastColumnExpr, while valid case-sensitive narrowing retains CastExpr. The Scala parameterization preserves the prior mixed-case request and adds the exact-case request, asserting CometNativeScanExec before executing the duplicate-error check. The fixtures use unchanged Int64 leaves and no field IDs; they do not introduce numeric conversion, overflow, null-handling or field-ID behavior changes. I found no actionable correctness issue in the added tests.
Validation and scope
CI ran merge b08959a6fa5a, whose parents are the assigned base bc74cc79fcf5 and head 0dfc87bca0d7; its entire source tree equals the reviewed head. The Rust job passed the new regression (1,180 tests passed, four skipped). Both nested duplicate-field cases passed in the Spark 4.0 scans job and Spark 4.1 scans job. Their native artifact ID and download digest match the build producer at that same CI commit. The snapshot contains 65 successful checks and nine skipped checks.
I did not run local builds or tests. The Rust test validates adapter selection without evaluating rows, and the Scala assertion checks duplicate-field text rather than full error-class/parameter parity or a fresh vanilla-Spark comparison. Maintained Spark 3.4/4.1 source was unavailable; the Spark 4.1 CI result does not fill that source-compatibility gap.
Performance
Production execution and dependencies are unchanged. The positive case-sensitive assertions protect the existing cast route that permits nested pruning, but these tests do not measure bytes read or speedup. The added work is limited to 12 small adapter configurations and one additional three-row Parquet integration case; there is no material new runtime cost or performance claim requiring a benchmark in this test-only PR.
Design
Separating adapter selection from an actual native-reader error check is appropriate here. The unit matrix isolates the recursive uniqueness condition, while the reader test exercises the previously missing exact-name request through file I/O and native execution. Keeping the mixed-case case alongside it prevents the parameterization from losing existing coverage. The implementation fits the existing test helpers and suite; I found no design change needed for this scope.
Abstraction & complexity
The change reuses struct_type, list_type, and rewrite_events_column, and uses a two-value Scala parameterization rather than duplicating the reader setup. It adds no production abstraction or dependency. The nesting corresponds directly to the three independent test dimensions and remains small enough to inspect; no actionable simplification is needed.
|
Merged, thanks @peterxcli ! |
Which issue does this PR close?
Closes #5707.
Rationale for this change
Comet can retain DataFusion's generic
CastExprfor pure structural narrowing of nested Parquet columns, allowing DataFusion's nested leaf pruning to read only the requested fields. Retaining that cast is safe only when DataFusion's exact-name lookup agrees with Spark's configured field-name resolver.An exact match alone is insufficient: with case-insensitive resolution, a file containing
s: struct<ID: bigint, id: bigint>and a requested schema ofs: struct<id: bigint>is ambiguous. DataFusion's generic cast can select the exactidfield, whereas Spark and Comet's Parquet converter reject the duplicate match. The same problem applies to non-ASCII names such asCAFÉandcafé, and to structs nested inside other structs or lists.The implementation merged in #5262 already requires exactly one source match under Spark's configured resolver at each nested struct level, in addition to an exact-name match. Ambiguous narrowing therefore falls back to
CometCastColumnExpr, preserving the existing duplicate-field error. This PR adds the missing regression coverage for that behavior; it does not change production code.What changes are included in this PR?
CometCastColumnExpr; case-sensitive exact matches must retain DataFusion'sCastExprso valid pruning remains enabled.Caféand exact-casecaféfromCAFÉ/cafésiblings. Both reads must raise a duplicate-field error, and the test asserts that the plan usesCometNativeScanExec.How are these changes tested?
CometNativeReaderSuitetests passed on Spark 4.1.git diff --checkpassed.