Skip to content

test: cover ambiguous exact nested Parquet field matches - #5751

Merged
sunchao merged 1 commit into
apache:mainfrom
peterxcli:test/parquet-ambiguous-exact-field-match
Sep 9, 2026
Merged

test: cover ambiguous exact nested Parquet field matches#5751
sunchao merged 1 commit into
apache:mainfrom
peterxcli:test/parquet-ambiguous-exact-field-match

Conversation

@peterxcli

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #5707.

Rationale for this change

Comet can retain DataFusion's generic CastExpr for 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 of s: struct<id: bigint> is ambiguous. DataFusion's generic cast can select the exact id field, whereas Spark and Comet's Parquet converter reject the duplicate match. The same problem applies to non-ASCII names such as CAFÉ and café, 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?

  • Add a native regression covering ASCII and non-ASCII sibling names with an exact requested match, directly in a struct and inside nested structs and lists.
  • Check both the narrowing predicate and the actual expression-adapter rewrite. Case-insensitive ambiguity must select CometCastColumnExpr; case-sensitive exact matches must retain DataFusion's CastExpr so valid pruning remains enabled.
  • Parameterize the existing native-reader regression to request both mixed-case Café and exact-case café from CAFÉ/café siblings. Both reads must raise a duplicate-field error, and the test asserts that the plan uses CometNativeScanExec.

How are these changes tested?

  • 14 native structural-narrowing tests passed.
  • 2 focused CometNativeReaderSuite tests passed on Spark 4.1.
  • Rust formatting, Maven Spotless, and git diff --check passed.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sunchao
sunchao merged commit 17f54da into apache:main Sep 9, 2026
74 checks passed
@sunchao

sunchao commented Sep 9, 2026

Copy link
Copy Markdown
Member

Merged, thanks @peterxcli !

@peterxcli
peterxcli deleted the test/parquet-ambiguous-exact-field-match branch September 9, 2026 03:30

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up. I verified the premise before reading the tests: the resolver_matches == 1 guard did land in #5262 itself, so this really is test-only and #5707 describes the pre-fix code. git log -S resolver_matches confirms #5262 is the only commit that ever touched it.

I also checked that the new coverage is load-bearing rather than decorative. Reverting the guard to resolver_matches >= 1 fails the new Rust test and nothing else in schema_adapter (46 of 47 still pass), and on the Scala side it fails the new café parameter while the pre-existing Café one still passes. That is exactly the gap the issue asked to close, so this does what it says.

Two things I would like to see, neither blocking.

The fixture writes named_struct('CAFÉ', id, 'café', id), so both siblings hold the same value. That makes it impossible for any value assertion to tell which sibling was selected, now or later. Could you change the second to id + 100? It costs nothing and makes the fixture able to fail.

That matters because of the other half of the new Rust test. The case_sensitive = true branch asserts DataFusion's CastExpr survives, which is the right structural check, but nothing verifies that the retained cast then picks the right sibling. That is the direction where a mistake is silent: when the predicate denies, CometCastColumnExpr raises a duplicate-field error, but when it allows, a wrong pick just quietly returns the other column. Would you add a caseSensitive = true read requesting café exactly and assert the values are the id + 100 ones? I ran that against this branch locally and it passes in both directions and for the ASCII ID/id pair too, so it should go green as pure regression coverage.

Some smaller points.

On the duplicate-field assertion, contains("duplicate field") would still pass if Comet named only one of the two matched siblings, which is the property this PR exists to pin down. I compared against vanilla Spark locally and the messages are byte-identical, Found duplicate field(s) "café": [CAFÉ, café] in case-insensitive mode., so asserting the matched-field list would tighten it for free.

The two generated test names differ only in the case of one letter, which will be hard to tell apart in a CI failure list, on a test that is specifically about names differing only by case. Naming them by intent would read better, something like (non-ASCII, mixed-case request) and (non-ASCII, exact-case request). That also keeps the (non-ASCII) marker that the top-level duplicate test above still uses.

In the Rust test the assert_eq! names the failing configuration, which is what made the mutation above easy to diagnose. The two assert!s below it do not, so a rewrite-side failure in the twelve-configuration loop would report only a line number. Reusing the same "{physical:?} -> {target:?}, case_sensitive={case_sensitive}" suffix on both would keep them consistent.

Separately, probing the case-sensitive fall-through in the same match arm turned up a real bug that is not yours: a struct with byte-identical duplicate field names makes the native scan return N times the rows. Filed as #5783.

Approving, since none of the above blocks and the coverage is a clear improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nested Parquet cast retention accepts an ambiguous case-insensitive field match

3 participants