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:
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:
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.
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
doGenCodecompiled 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.
checkSparkAnswerAndOperatorand the defaultquerymode 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 onBinaryTypeand stay native onStringType. Nothing pins either split.lower.sqlandupper.sqlopen 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.
ExtendedExplainInfoexposesgetNativeExpressionsandgetCodegenDispatchExpressions, andCometCodegenSuiteuses 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:and matching SQL file harness query modes, so the assertion is available at the layer where most expression coverage actually lives:
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
querymode 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,roundon float/double, themaskfamily), 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.