andygrove opened a new issue, #5609:
URL: https://github.com/apache/datafusion-comet/issues/5609
### Is your feature request related to a problem or challenge?
Comet evaluates a scalar expression one of three ways: natively (a
DataFusion expression), through the JVM codegen dispatcher (Spark's own
`doGenCode` compiled into an Arrow batch kernel), or not at all (the enclosing
operator falls back to Spark).
Our tests can see the third and not the first two.
`checkSparkAnswerAndOperator` and the default `query` mode in the SQL file
harness both assert "no fallback", which is a real assertion, but native and
codegen-dispatched execution are indistinguishable to them: both produce
Spark-matching results by construction.
That leaves a class of change invisible to the test suite. A serde that
widens from native to codegen dispatch silently gives up the native kernel; one
that narrows from dispatch to native silently gives up Spark-exact semantics.
Neither changes a single result, so every existing assertion stays green.
This bites in practice on the growing set of expressions whose mechanism
depends on the argument type. `CometRound` (#5600) dispatches on float/double
and stays native on decimal/integral. `CometLength` / `CometBitLength` /
`CometOctetLength` (#5607) will dispatch on `BinaryType` and stay native on
`StringType`. Nothing pins either split. `lower.sql` and `upper.sql` open with
a comment saying the fixture exists to exercise the dispatcher route, and
nothing checks that it does.
The pieces to assert this already exist. `ExtendedExplainInfo` exposes
`getNativeExpressions` and `getCodegenDispatchExpressions`, and
`CometCodegenSuite` uses them. There is just no reusable helper, so writing the
assertion is enough friction that nobody does.
### Describe the solution you'd like
A helper on `CometTestBase`:
```scala
checkSparkAnswerAndImpl(df, native = Seq("round"), dispatched = Seq.empty)
```
and matching SQL file harness query modes, so the assertion is available at
the layer where most expression coverage actually lives:
```sql
query expect_dispatch(bit_length)
SELECT bit_length(b) FROM test_bit_length_binary
query expect_native(bit_length)
SELECT bit_length(s) FROM test_bit_length
```
Naming an expression asserts both that it ran through the expected mechanism
and that it did not run through the other one, so the assertion is a claim
rather than a hint.
### Describe alternatives you've considered
Making the mechanism assertion mandatory: have the default `query` mode
assert that the dispatched-expression set matches a file-level declaration
defaulting to empty. That catches the whole regression class across every
fixture permanently rather than only where someone remembered to annotate.
It needs a one-time annotation pass over every fixture that already
dispatches (`rlike`, `regexp_replace`, `split`, `lower`, `upper`, `round` on
float/double, the `mask` family), and support levels differ by Spark version,
so some fixtures would need version-conditional declarations. Worth doing, but
after the opt-in form is in use and the annotation cost is understood. Tracked
as a follow-up rather than built here.
### Additional context
Came out of review discussion on #5607.
--
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]
