[PR] test: teach CometCastSuite about the codegen dispatch path [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove opened a new pull request, #5187:
URL: https://github.com/apache/datafusion-comet/pull/5187

   ## Which issue does this PR close?
   
   Closes #5186.
   
   ## Rationale for this change
   
   `CometCastSuite` was written when a cast was either implemented natively or fell back to Spark
   for the whole plan. `CometCast` now mixes in `CodegenDispatchFallback`, so an `Incompatible` or
   `Unsupported` support level no longer means the plan leaves Comet -- the JVM codegen dispatcher
   runs Spark's own generated code for the cast inside the Comet pipeline, and the results are
   Spark-compatible by construction.
   
   Three assumptions in the suite were stale:
   
   1. `all valid cast combinations covered` required an `ignore`d test for every pair that is not
      `Compatible`, on the grounds that the test would otherwise fail. For a codegen-dispatched cast
      the test now *passes*, so the rule forced us to ignore working coverage.
   2. Several `ignore`d tests carried comments about wrong results or Arrow errors from the native
      path. Those casts are dispatched rather than executed natively now, and all of them produce
      correct results today.
   3. `castTest`'s `hasIncompatibleType` flag meant "do not assert native execution", which cannot
      distinguish "no Rust kernel but the plan stays in Comet" from "the plan falls back to Spark".
   
   ## What changes are included in this PR?
   
   **A three-way `CastExecution` expectation replaces `hasIncompatibleType`.** `Native` means Comet's
   own kernel evaluates the cast, `Dispatched` means the JVM codegen dispatcher compiles Spark's
   `Cast.doGenCode` into the pipeline, and `SparkFallback` means the plan leaves Comet. The first two
   both assert a fully native plan; `SparkFallback` only compares answers, as before.
   
   `castTest` now also pins *which* of the first two happened, rather than only that the plan stayed
   in Comet. It enables `explain.codegen.enabled` plus the verbose explain format so
   `CometExecRule`'s `JVM codegen dispatcher: <names>` roll-up is visible, then asserts the cast is
   (or is not) named there. That assertion has teeth in both directions -- flipping a `Native` test
   to `Dispatched` locally fails with "Expected the cast to be routed through the JVM codegen
   dispatcher, but the plan does not report it".
   
   Only the non-ANSI `cast()` is checked that precisely. The ANSI block enables `allowIncompatible`,
   which pushes an `Incompatible` cast back onto the native path, so there the assertion stays at
   "the plan is fully Comet".
   
   The `SparkFallback` cases in this suite are all about the *input* type rather than the cast: the
   byte/short tests when `COMET_PARQUET_UNSIGNED_SMALL_INT_CHECK` is on, and the `cast ArrayType to
   StringType` element types that `CometScanTypeChecker` rejects. Both now say so by name
   (`smallIntScanExecution`, `scanExecution`) instead of via a boolean called `hasIncompatibleType`.
   
   **`all valid cast combinations covered` is rebuilt around the three outcomes.** It computes the
   expected path per pair from `CometCast.isSupported` plus `CometBatchKernelCodegen
   .isSupportedDataType`, and now asserts a genuinely new invariant: no pair in the matrix resolves to
   `SparkFallback` -- every cast between `CometCast.supportedTypes` stays inside Comet, either
   natively or through the dispatcher. Ignored tests are then rejected outright, with an
   outcome-specific explanation, instead of being *required* for every non-`Compatible` pair.
   
   **Eight stale `ignore`s are removed.** All eight pass:
   
   - `cast FloatType to DecimalType(10,2)` and `cast DoubleType to DecimalType(10,2)` -- `Incompatible`
     for rounding differences (#1371), so the non-ANSI cast dispatches. The existing
     `- allow incompat` variants still cover the native kernel, and are now the demonstration of the
     `Native` / `Dispatched` distinction rather than a workaround for an ignored test.
   - `cast TimestampType to BooleanType / ByteType / ShortType / IntegerType / FloatType / DoubleType
     / DecimalType(10,2)` -- `Unsupported` (#352, #1280), so they dispatch. Spark's ANSI and TRY modes
     reject timestamp -> boolean at analysis time (`Cast.canAnsiCast`), so that one keeps
     `testAnsi = false, testTry = false`; the numeric targets are allowed in ANSI/TRY and exercise
     both, including the overflow errors Spark raises, which now come from Spark's own code running
     in-pipeline.
   
   The stale issue links and "expected X, actual null" notes are replaced with a description of why
   there is no native kernel.
   
   ## How are these changes tested?
   
   ```
   ./mvnw test -Pspark-3.5 -Pscala-2.12 -Dtest=none -Dsuites="org.apache.comet.CometCastSuite"
   ./mvnw test -Pspark-4.0 -Pscala-2.13 -Dtest=none -Dsuites="org.apache.comet.CometCastSuite"
   ```
   
   170/171 and 171/172 pass respectively. The one failure, `cast StringType to TimestampType`, is
   pre-existing and unrelated: it fails identically on `main` in this environment because Spark's
   row-at-a-time and Comet's vectorized execution report different offending values from the fuzz
   data in the ANSI error message. Verified by stashing this change and re-running that test alone.
   
   Note on ordering: #5185 ignores `cast BooleanType to DecimalType(10,2)` purely to satisfy the old
   matrix rule. If that PR lands first, the ignore has to become
   `expectedExecution = CastExecution.Dispatched` on rebase; the matrix test here will say so.
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] test: teach CometCastSuite about the codegen dispatch path [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove closed pull request #5187: test: teach CometCastSuite about the codegen dispatch path
URL: https://github.com/apache/datafusion-comet/pull/5187


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org