fix: decline structs with duplicate field names before they reach Java Arrow

[dwsmith1983]

Which issue does this PR close?

Closes #5605.

Rationale for this change

Native shuffle accepted a struct column with duplicate field names, and the task then died at the JVM Arrow FFI import because Java Arrow keys a struct vector's children by name and collapses the two into one. The issue's second reproduction, a local table scan of the same struct, showed the shuffle predicate is not the only gap: the scan builds the struct through Java Arrow as well, so even with the shuffle on Spark the plan fails in columnar-to-row when it reads the second child of a one-child vector. Any Comet operator that turns Spark rows into Arrow on the JVM has the same limitation.

What changes are included in this PR?

• The shared type gate in DataTypeSupport declines a struct with duplicate field names at any depth, with the fallback reason struct with duplicate field names. CometLocalTableScanExec and CometSparkToColumnarExec use it, so neither builds the struct any more and both reproductions in the issue fall back to Spark with a correct answer.
• The native shuffle type predicate gains the distinct-name clause the columnar predicate already has. With the expression serde, the local table scan and row-to-columnar all declining the shape first, no plan reaches this clause today; it is kept so the two shuffle predicates agree, which Centralize data-type support predicates so new types don't have to be added in multiple places #5021 tracks consolidating.

How are these changes tested?

Four tests in CometNativeShuffleSuite:

• the local table scan reproduction from the issue, which before the change chose a Comet exchange and then crashed, now falls back with the type-gate reason;
• a unit test that both row conversion sinks decline a top-level duplicate struct with that reason, that the local scan also declines array<struct<a, a>> and map<_, struct<a, a>> through the gate's recursion, and that a struct whose names differ only by case stays supported;
• a unit test that feeds a synthetic native child with a duplicate struct straight into shuffleSupported, which reports unsupported shuffle data type, while the case-distinct struct is accepted as native shuffle; this test fails when only the shuffle clause is reverted;
• the cached-relation reproduction, which now plans zero Comet exchanges and returns Spark's answer.

Native shuffle and both concrete columnar shuffle suites pass (138 tests), as do the exec and native reader suites (252 tests) on Spark 3.5 against the rebuilt library.

[sunchao]

Correctness

Reviewed 2d5ffa95363e against f29a236128b3.

Spark permits a struct such as named_struct('a', id, 'a', id + 1) and preserves both values by ordinal. Before this change, local-table and cached-row conversion could admit that schema into Java Arrow, whose default struct conflict policy replaces same-named children. Rejecting the shape before those conversions makes the affected plans fall back to Spark instead of failing while writing or importing the struct.

I checked the recursive gate and its actual callers, including local-table conversion, Spark-to-columnar conversion, required-schema scans, and native/columnar shuffle. The check is local to each struct and compares names exactly: a and A remain distinct, repeated names in separate structs remain valid, and duplicates beneath a struct, array, or map are declined where those containers are otherwise supported. Existing empty-struct and unsupported-type behavior is preserved. Checking field types rather than the outer schema's column names also preserves duplicate top-level columns. The existing expression and codegen-dispatch gates already decline duplicate struct inputs/outputs; disabling dispatch retains Spark fallback.

This agrees with the inspected maintained Spark 3.5 and 4.0 implementations: struct construction retains fields in order in interpreted and generated execution, while name-based extraction separately reports ambiguity according to the configured resolver. I found no new P1/P2 issue in the three-file change.

Validation

Reviewed all four added tests, including the synthetic native child that exercises the shuffle predicate independently of the earlier conversion gate. Validation here is source inspection, not a local Spark/JNI/native test run. The author reports 138 shuffle-suite and 252 exec/reader tests on Spark 3.5; I have not independently reproduced those results. At the September 11, 16:35 UTC check, product CI is action_required, so there is no completed product-test result to credit. Maintained Spark 3.4/4.1 branches were unavailable locally; compatibility conclusions are limited to the inspected 3.5/4.0 sources.

Performance

The additional work walks struct field names during schema admission and shuffle planning, with temporary storage proportional to the number of sibling fields. These checks inspect schemas, not individual row values. The intended benefit is avoiding invalid execution; no throughput or latency improvement was measured. I found no material performance issue introduced by this change.

Design

Placing the rejection in the shared type gate addresses both reported row-conversion entry points before Arrow allocates the incompatible struct. Keeping the separate native-shuffle predicate consistent with its existing columnar counterpart is appropriate, and the direct predicate test guards that rule even when normal plans are rejected earlier. The fix preserves Spark fallback without changing struct names or values.

Abstraction & complexity

The change reuses the existing recursive type checks and fallback reporting, adding no new abstraction or execution path. The short duplicate-name condition is proportionate to the fix; consolidating all type predicates is a separate design change and is not needed for this correction.

[sunchao]

Re-reviewed 77a0fdb7814c against 8b818b53bd6a. The authored patch, including both duplicate-field guards and all four tests, is byte-for-byte unchanged from the reviewed 2d5ffa95363e. The head update consists entirely of the rebase over #5794.

I rechecked the conversion and shuffle boundaries, recursive struct/array/map checks, exact-case names, duplicate root columns, and test routing against the new base. No new or remaining P1/P2 concern was found. The existing approval stands.

Validation remains source inspection and exact source-equivalence checks, with no local Spark/JNI/native execution. At the September 11, 17:25 UTC refresh, current product CI still reports action_required. The author's test counts remain unverified, and maintained Spark compatibility comparisons remain limited to 3.5/4.0 because the 3.4/4.1 branches are unavailable.

[sunchao]

Re-reviewed 5b307a0f27e4 against db790673d0a2. The authored patch, including both duplicate-field guards and all four tests, is byte-for-byte unchanged from 77a0fdb7814c; this update is a rebase. I rechecked the conversion/shuffle boundaries and relevant base interactions. No new or remaining P1/P2 concern was found. The existing approval stands.

Validation has advanced: all four duplicate-name regressions pass in the Spark 3.4, 3.5, 4.0, and 4.1 shuffle jobs. Their logs show checkout 3e6ce49f8425, whose parents are the reviewed base/head and whose tree exactly matches the head. This evidence does not cover the subsequently regenerated merge preview bab738445c9b. No local Spark/JNI test run was performed; maintained-source comparisons remain limited to Spark 3.5/4.0 because the 3.4/4.1 branches are unavailable.

[andygrove]

This looks right to me and it is a clean, contained fix for a crash that is reachable on main with default settings.

One thing worth flagging: #5603 is heading in the opposite direction. It makes the Java Arrow boundaries duplicate-safe by giving struct children unique runtime names, and it removes the columnar shuffle distinct-name clause plus the CometCreateNamedStruct guard. If this PR merges first, #5603 will need to also drop the new DataTypeSupport clause and the native shuffle clause added here, and the four tests here would flip to asserting native execution. Could you add a note in the PR description pointing at #5603 so whoever rebases second knows the two gates are meant to go away together?

[andygrove]

The description says no plan reaches this clause today. I think that stops being true as soon as any native operator can emit a duplicate-named struct, which #5603 would do for named_struct. Might be worth rewording the comment to say this is the backstop for native-produced structs rather than framing it purely as parity with the columnar predicate, so nobody removes it as dead code later.