test: add helpers to assert whether an expression ran natively or via codegen dispatch - #5610
test: add helpers to assert whether an expression ran natively or via codegen dispatch#5610andygrove wants to merge 1 commit into
Conversation
… codegen dispatch Comet evaluates a scalar expression natively, through the JVM codegen dispatcher, or not at all. Only the third is visible to our tests: checkSparkAnswerAndOperator and the default sql-file query mode assert "no fallback", but native and dispatched execution both produce Spark-matching results by construction, so a serde that swaps one for the other passes every existing assertion unchanged. Add checkSparkAnswerAndImpl to CometTestBase and expect_native / expect_dispatch query modes to the sql-file harness, both backed by the same assertion over ExtendedExplainInfo's native and codegen-dispatch expression sets. Naming an expression asserts both that it ran through the expected mechanism and that it did not run through the other one. Annotate round.sql, lower.sql and upper.sql as worked examples. Doing so split one round query: round(NULL, 0) implicitly casts the untyped null to double, so it dispatches while the decimal literals beside it stay native. Closes #5609.
| s"native=[${actualNative.mkString(", ")}] " + | ||
| s"codegen-dispatched=[${actualDispatched.mkString(", ")}]" | ||
| native.foreach { name => | ||
| if (actualDispatched.contains(name)) { |
There was a problem hiding this comment.
[P2] Account for expressions nested inside a dispatched subtree
Could the exclusion check account for descendants of a dispatched expression? With non-null Double columns in a nonempty Parquet table, Comet projection and codegen dispatch enabled, SELECT abs(a), hypot(abs(b), c) FROM t lowers the first abs natively but serializes the whole hypot(abs(b), c) tree to the JVM. Dispatch tags only hypot, so native = Seq("abs"), dispatched = Seq("hypot") can pass even though the nested abs runs in the JVM kernel. The new assertion therefore misses a composed case where the named expression uses both mechanisms. A nested-expression regression case would cover this missing classification. This is source-derived, not an executed reproduction.
| checkSparkAnswerAndOperatorWithTolerance(sql, tol) | ||
| case ExpectFallback(reason) => | ||
| checkSparkAnswerAndFallbackReason(sql, reason) | ||
| case ExpectDispatch(names) => |
There was a problem hiding this comment.
[P2] Accept the new implementation modes as positive sentinels
Could requireSentinelForCodegenExpectError also recognize ExpectDispatch and ExpectNative? Both branches call checkSparkAnswerAndImpl, which first performs the same answer/operator checks as a plain query. For a file with spark.comet.exec.scalaUDF.codegen.enabled=true and an expect_error record, upgrading its last plain positive query to either new mode makes preflight reject the file as missing a sentinel before any SQL runs. The stronger assertion should be able to serve as that successful control query without requiring a redundant plain query.
Which issue does this PR close?
Closes #5609.
Rationale for this change
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 dispatched execution are indistinguishable to them: both produce Spark-matching results by construction.So a serde that widens from native to codegen dispatch silently gives up the native kernel, and one that narrows from dispatch to native silently gives up Spark-exact semantics. Neither changes a result, so every existing assertion stays green.
This matters on the growing set of expressions whose mechanism depends on the argument type.
CometRound(#5600) dispatches on float and double and stays native on decimal and integral.CometLength/CometBitLength/CometOctetLength(#5607) will dispatch onBinaryTypeand stay native onStringType. Nothing pinned either split.lower.sqlandupper.sqlopen with a comment saying the fixture exists to exercise the dispatcher route, and nothing checked that it did.ExtendedExplainInfoalready exposesgetNativeExpressionsandgetCodegenDispatchExpressions, andCometCodegenSuitealready uses them. There was just no reusable helper, so writing the assertion was enough friction that nobody did.What changes are included in this PR?
CometTestBase.checkSparkAnswerAndImpl(df, native, dispatched), plus the underlyingassertExpressionImplsplit out so callers holding a plan can reuse it. Naming an expression asserts both that it ran through the expected mechanism and that it did not run through the other one, so a name is a claim rather than a hint.expect_dispatch(<names>)andexpect_native(<names>), accepting a comma-separated list. Both check results and coverage like a plainqueryfirst, then delegate to the same assertion.checkSparkAnswerAndOperator, and a tip in the SQL test list).round.sql,lower.sqlandupper.sqlannotated as worked examples.Deliberately not included: making the assertion mandatory. Having the default
querymode assert against a file-level declaration would catch this class of regression everywhere rather than only where someone annotated, but it needs a one-time pass over every fixture that already dispatches (rlike,regexp_replace,split,lower,upper,roundon float and double, themaskfamily) and some would need version-conditional declarations. Worth doing once the opt-in form is in use; the alternatives section of #5609 records it.How are these changes tested?
New
SqlFileTestParserSuitecovers the two directives: single name, comma-separated list, surrounding whitespace, empty names dropped, and that the new patterns do not shadowexpect_fallback/expect_error/spark_answer_only/tolerance=/ignore. Pure text parsing, so no Spark session. Registered in bothpr_build_linux.ymlandpr_build_macos.yml.CometCodegenSuitegains a test that the helper actually fails. AgainstSELECT abs(a), hypot(a, b)(a known native/dispatched pair) it asserts the correct claim passes and that four wrong claims are rejected: each mechanism swapped, and a name the query does not contain, which is what a fixture typo looks like.End to end via the annotated fixtures. Run against Spark 4.1:
CometSqlFileTestSuiteCometExpressionSuiteCometCodegenSuite,SqlFileTestParserSuite,CometMathExpressionSuite,CometStringExpressionSuitedev/ci/check-suites.pypasses with the new suite registered in both workflow files.The annotations earned their keep on the first run.
SELECT round(123.456, 2), round(2.5, 0), round(3.5, 0), round(-2.5, 0), round(NULL, 0)reportedroundin both sets, becauseround(NULL, 0)implicitly casts the untyped null to double and so dispatches while the decimal literals beside it stay native. The query is now split, with the reason in a comment.Only the default Spark 4.1 profile has been exercised locally; the 3.4 / 3.5 / 4.0 profiles are left to CI. The annotated splits do not depend on Spark version (
CometRoundandCometCaseConversionBasebranch on argument type and config, not on version), so no divergence is expected there.Note for reviewers
While writing this I noticed
lower_enabled.sqlandupper_enabled.sqlsetspark.comet.expression.Lower.allowIncompatible=trueto reach the native path, butCometCaseConversionBasereportsCompatibleand branches onspark.comet.caseConversion.enabledinstead, so that config is a no-op and both fixtures currently exercise the dispatcher despite their names. Left alone here rather than folded in. Happy to file it separately.