andygrove commented on PR #4827:
URL:
https://github.com/apache/datafusion-comet/pull/4827#issuecomment-5719962779
I've read through this properly now, and I want to flag a soundness problem
before it gets more review effort, because it is not obvious from the diff.
To be clear about what this PR is and isn't: it isn't reimplementing the
dispatcher, it reuses `emitJvmCodegenDispatch` as it stands, and the gap it
identifies is real. `CodegenDispatchFallback` is opt-in per serde and
`dispatchIfFallback` is only reachable from the `Unsupported` and non-opt-in
`Incompatible` arms, so an expression with no serde entry at all never gets a
dispatch attempt and takes its operator down with it. Generalising that is a
reasonable thing to want, and the thread-local scoping is a nicer job than I
would have expected.
The problem is the shape of the catch-all. I prototyped essentially this —
`handler.convert(...).orElse { emitJvmCodegenDispatch(...) }` behind an
off-by-default flag — on #5574 at the end of August and measured it across the
expression suites, `CometSqlFileTestSuite` and `CometCodegenSuite`. It produced
132 rescues and two wrong answers. The reproducer was
```
array_contains(map_keys(element_at(map(1, map(0.0, 7)), k)), -0.0)
```
where Spark returns `true` and the rescued plan returns `false`.
`CometElementAt.getSupportLevel` sees an outer key of `IntegerType` and reports
`Compatible`; `convert` then fails anyway because the nested map literal is
unsupported; the rescue dispatches `element_at` on its own, and `map_keys` and
`array_contains` then run natively on top of it, where `array_contains` does
not normalise `-0.0` the way Spark does.
The general form is what matters: that decline was not protecting the node
that declined, it was protecting that node's *ancestors*, which are only
Spark-compatible for as long as the whole subtree falls back together. Nothing
in the serde API records that dependency, so there is no way to detect it at
the point of rescue. Any time a native ancestor's compatibility rests on a
descendant having declined, a node-local rescue silently removes it. I wrote
that up on #5574 and #5572 carries it as a prerequisite; this PR predates the
measurement by a couple of months, which is presumably why neither is
referenced here.
This PR is in that node-local form. The comment on the `.orElse` states the
hazardous property as the intent — "Supported ancestors keep converting
natively and reference this detour's output" — and
`CometPartialProjectFallbackSuite` has a test called "detour fires at the
outermost unsupported node, keeping supported ancestors native" that asserts
it. Neither of the two guards closes the hole: my reproducer sits inside a
projection, so `withJvmDetour` scoping does not exclude it, and my prototype
was off by default too. If anything the surface here is wider than what I
measured, since my version only touched the
`Compatible`-then-`convert`-returns-`None` arm and this one also catches
`Unsupported` and the no-handler case.
The way I'd salvage it is the alternative I sketched on that issue: allow
the detour only where no native ancestor will consume the result, which in
practice means rescuing at the top of the projection list — the `Alias` — and
never below it. In my run 86 of the 132 rescues were `alias`, so most of the
upside should survive the restriction, and it turns the rule into something
checkable rather than something we have to reason about per expression. The
filter predicate would need the same treatment at the root of the condition.
@schenksj, would you be up for restricting the detour that way? If it helps
I can push the prototype branch so you can reproduce the two failures directly.
Worth adding the `map_contains_key` case to the suite either way — it is the
shape that will catch a regression here.
--
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]
