schenksj commented on PR #4827:
URL:
https://github.com/apache/datafusion-comet/pull/4827#issuecomment-6631504275
@andygrove, replying to your Sep 17 comment on soundness:
Thanks for writing this up. You're right, and I've restricted the detour the
way you suggested.
The detour no longer lives in `exprToProtoInternal`. The new
`CometJvmDetour.exprToProtoOrDetour`
is called only by `CometProjectExec` (once per projection item) and
`CometFilterExec` (on the whole
condition). It tries the normal conversion first. If that fails, it sends
the whole item to the
dispatcher: the `Alias` child, or the full filter condition. It never sends
a node below the root.
So no Rust-implemented ancestor in the expression consumes a detoured value.
The projection only
outputs the value, and the filter only keeps rows where it is true. That
makes the rule checkable
instead of something we reason about per expression.
Tests (in `CometPartialProjectFallbackSuite`):
- Your reproducer plus the `map_contains_key` form, with `element_at` forced
to decline through a
new test-only config (`spark.comet.testing.unsupportedExpressions`). The
test asserts the result
matches Spark, that `element_at` went through the dispatcher, and that
neither `array_contains`
nor `map_keys` is reported as Rust-implemented.
- "the detour takes the whole projection item": `concat(upper(s),
cast(length(s) AS STRING))` with
`length` declined compiles one kernel, and `concat` and `upper` are not
reported as
Rust-implemented. There is also a filter version.
To check that the tests discriminate, I put the old node-level hook back
temporarily, without
committing it, and reran the suite. Six tests failed: both whole-item tests,
the ancestor test, the
disabled-expression test, the reason-rollup test, and the scoping test. One
caveat: on current
`main`, the old hook gave the right answer for your reproducer. The ancestor
test caught it only
through the structural check (a Rust-implemented `array_contains` over a
detoured `element_at`), not
through a wrong result. I didn't track down which change on main fixed the
`-0.0` comparison. The
structural assertion holds whatever the Rust kernels do.
I removed the "outermost unsupported node" wording and the test that
asserted supported ancestors
stay in Rust, since both described the old behaviour.
--
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]
