andygrove opened a new pull request, #5610: URL: https://github.com/apache/datafusion-comet/pull/5610
## 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. `CometSqlFileTestSuite` on Spark 4.1: 467 succeeded, 0 failed. The annotations found something 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. Still to do before this leaves draft: `CometExpressionSuite`, `CometStringExpressionSuite` and `CometMathExpressionSuite` have not been run yet, and this has only been exercised on the default Spark 4.1 profile. ## 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. -- 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]
