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: [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]
