andygrove commented on issue #5574:
URL:
https://github.com/apache/datafusion-comet/issues/5574#issuecomment-5482050612
Prototyped the `.orElse` variant and measured it. **It is unsound as
specified — I'd drop it in this form.** Details below, since the failure mode
is more interesting than the result.
## What I built
`handler.convert(...).orElse { CometScalaUDF.emitJvmCodegenDispatch(expr,
inputs, binding) }` in the `Compatible` arm, behind an internal off-by-default
flag. Deliberately *not* gated on `CodegenDispatchFallback`: the mixin marks a
serde's declared unsupported cases, whereas reaching here means the serde
promised it could convert and then couldn't, which is exactly the case no serde
has declared. That ungated form is what would subsume #5575.
## What fired
132 rescues across `CometExpressionSuite`, the array / map / string /
temporal / json / math expression suites, `CometSqlFileTestSuite` and
`CometCodegenSuite`:
| count | expression |
| --- | --- |
| 86 | `alias` |
| 33 | `staticinvoke` |
| 3 | `element_at` |
| 2 each | `size`, `precisetimestampconversion`, `cast`, `casewhen`, `add` |
The 33 `staticinvoke` rescues confirm #5575 is subsumed. The 86 `alias`
rescues are the whole-projection case — `CometAlias.convert` just delegates to
its child, so any projection whose tree fails anywhere gets its entire
top-level expression dispatched.
## The problem
35 test failures. About 33 are tests asserting a fallback that no longer
happens — expected, and they'd need updating. **Two are wrong answers**, both
`map_contains_key` over a nested map with floating-point or collated keys:
Spark returns `true`, Comet returns `false`.
The mechanism generalises, which is why this matters. For
```
array_contains(map_keys(element_at(map(1, map(0.0, 7)), k)), -0.0)
```
`CometElementAt.getSupportLevel` sees an outer key type of `IntegerType` and
says `Compatible`. `convert` then fails anyway, because the nested map
*literal* is unsupported. The rescue dispatches `element_at` alone, which
faithfully produces the inner map — and then `map_keys` and `array_contains`
run **natively** on top of it, where `array_contains` does not normalise `-0.0`
to `+0.0` the way Spark does. Hence `false`.
So the decline was not protecting the node that declined. It was protecting
that node's *ancestors*, which are only Spark-compatible as long as the whole
subtree falls back together. Rescuing at the failing node converts a
whole-operator fallback into a local dispatch and silently removes that
protection.
That is a general soundness hazard, not a quirk of this one expression: any
time a native ancestor's compatibility depends on a descendant having declined,
a node-local rescue breaks it. Nothing in the serde API records that dependency
today, so there is no way to detect it at the point of rescue.
## Where that leaves the design
- Node-local rescue: unsound, drop it.
- Root-level-only rescue (rescue at the `Alias` / top of the projection,
never below): plausible, since the entire expression then runs Spark's own
code. But it needs an explicit rule that a rescue may only happen where no
native ancestor will consume the result, and the 86 `alias` hits suggest that
is the common case anyway — so most of the upside may survive.
- Either way this does **not** subsume #5575 for free. `staticinvoke` sits
mid-tree as often as not, so the `StaticInvoke` catch-all is better done as its
own targeted change with the type gate doing the safety work.
Practical consequence for the epic: I'd stop treating #5574 as the item that
might make the per-expression mixin issues unnecessary. The mixin route reports
`Unsupported` from `getSupportLevel`, which routes through `dispatchIfFallback`
*before* any ancestor has converted, so it does not have this hazard. The
per-expression issues (#5575–#5591) should proceed on their own.
Prototype is on a local branch; happy to push it if anyone wants to
reproduce.
--
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]
