andygrove commented on PR #5603:
URL:
https://github.com/apache/datafusion-comet/pull/5603#issuecomment-5608248739
@sunchao I think you are right, and the evidence is stronger than the
summary states, so I prototyped it rather than argue from the source.
### Nothing in the native path resolves a struct child by name
That is the property the native route needs, and it holds:
- `CometGetStructField` serializes `setOrdinal(expr.ordinal)`, and the
planner builds `GetStructField::new(child, expr.ordinal as usize)`. Purely
positional on both sides.
- There is no `column_by_name` anywhere in `native/spark-expr/src` or
`native/core/src` outside tests.
- Native `to_json` iterates `fields()` positionally and emits each field's
own name, so a duplicate-name struct produces both keys, which is what Spark
does.
Field access by name is not even reachable. `named_struct('x', a, 'x', b).x`
is rejected by Spark's own analyzer before Comet sees it:
```
[AMBIGUOUS_REFERENCE_TO_FIELDS] Ambiguous reference to the field `x`. It
appears 2 times in the schema.
at ExtractValue$.findField(complexTypeExtractors.scala:161)
```
So Spark itself guarantees the only way into a duplicate-name struct is by
ordinal.
### The prototype works
I removed the four lines of `getSupportLevel` on this branch, leaving
everything else in the PR in place, and ran ten shapes under both shuffle modes
with `checkSparkAnswer`, asserting no dispatcher note in the extended explain:
```
PROBE[jvm] OK flat / computed children / case-distinct + dup / nested
dup /
to_json / array of dup / map value dup / shuffled / sorted
/ grouped
PROBE[native] OK (same ten)
```
Twenty for twenty, all native, all matching Spark. `CometCodegenSuite` still
passes 94 of 94 and the new `create_named_struct.sql` fixture still passes with
the guard gone, so the PR's own routing tests do not depend on the rejection
either.
### The two positions are not in conflict
The native route depends on this PR's Arrow-boundary work, and that is the
part to keep. A native `named_struct` produces a duplicate-name `StructArray`
that then has to survive shuffle IPC, broadcast coalescing, the per-batch
import factory and a cache round trip. `CometArrowStreamReader`,
`ColumnarBatchArrowReader` and the runtime field allocation are what make that
hold, and the dispatcher route needs them just as much, since the JVM result
crosses the same boundaries. Nothing in the PR is wasted by switching the
primary path.
@RRXXZZYY the shape I would suggest, if you agree: keep the boundary fixes
exactly as they are, replace the four-line `getSupportLevel` rejection with
`Compatible()`, and let `CodegenDispatchFallback` stay as the fallback for
whatever native declines for other reasons rather than as the primary path for
duplicates. The `getUnsupportedReasons` text and the generated `expressions.md`
entry then need updating, since they currently promise the dispatcher.
One caveat on my own earlier review: my roughly fifty divergence probes were
run against the dispatcher route, so they do not carry over unchanged. The ten
above are a start, not a replacement. The shapes I would want re-run natively
are dictionary-encoded inputs, an all-null and a zero-row batch, empty field
names, a struct straddling `spark.sql.codegen.maxFields`, and a `df.cache()`
round trip, since those are the ones where the two routes could differ.
Separately, my main request from last time is addressed: the JVM
columnar-shuffle duplicate-name guard is gone and the former fallback test is
now a query-level regression carrying a duplicate-name struct through a JVM
repartition. Thanks for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
