Skip to content

test: add helpers to assert whether an expression ran natively or via codegen dispatch - #5610

Open
andygrove wants to merge 1 commit into
mainfrom
test/expression-impl-assertions
Open

test: add helpers to assert whether an expression ran natively or via codegen dispatch#5610
andygrove wants to merge 1 commit into
mainfrom
test/expression-impl-assertions

Conversation

@andygrove

@andygrove andygrove commented Sep 1, 2026

Copy link
Copy Markdown
Member

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 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 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 on BinaryType and stay native on StringType. Nothing pinned either split. lower.sql and upper.sql open with a comment saying the fixture exists to exercise the dispatcher route, and nothing checked that it did.

ExtendedExplainInfo already exposes getNativeExpressions and getCodegenDispatchExpressions, and CometCodegenSuite already 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 underlying assertExpressionImpl split 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.
  • Two SQL file harness query modes, expect_dispatch(<names>) and expect_native(<names>), accepting a comma-separated list. Both check results and coverage like a plain query first, then delegate to the same assertion.
  • Documentation in the Comet SQL Tests guide (a section per mode, a tip on when to reach for them, and a step in "Adding a new test") and in the new-expression guide (the Scala helper alongside checkSparkAnswerAndOperator, and a tip in the SQL test list).
  • round.sql, lower.sql and upper.sql annotated as worked examples.

Deliberately not included: making the assertion mandatory. Having the default query mode 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, round on float and double, the mask family) 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 SqlFileTestParserSuite covers the two directives: single name, comma-separated list, surrounding whitespace, empty names dropped, and that the new patterns do not shadow expect_fallback / expect_error / spark_answer_only / tolerance= / ignore. Pure text parsing, so no Spark session. Registered in both pr_build_linux.yml and pr_build_macos.yml.

CometCodegenSuite gains a test that the helper actually fails. Against SELECT 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:

Suite Result
CometSqlFileTestSuite 467 succeeded, 0 failed
CometExpressionSuite 141 succeeded, 0 failed, 4 ignored
CometCodegenSuite, SqlFileTestParserSuite, CometMathExpressionSuite, CometStringExpressionSuite 134 succeeded, 0 failed

dev/ci/check-suites.py passes 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) reported round in both sets, because round(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 (CometRound and CometCaseConversionBase branch on argument type and config, not on version), so no divergence is expected there.

Note for reviewers

While writing this I noticed lower_enabled.sql and upper_enabled.sql set spark.comet.expression.Lower.allowIncompatible=true to reach the native path, but CometCaseConversionBase reports Compatible and branches on spark.comet.caseConversion.enabled instead, 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.

… 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)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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) =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test helpers to assert whether an expression ran natively or through the codegen dispatcher

2 participants