Skip to content

test: teach CometCastSuite about the codegen dispatch path - #5187

Draft
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:cast-suite-dispatch-expectations
Draft

test: teach CometCastSuite about the codegen dispatch path#5187
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:cast-suite-dispatch-expectations

Conversation

@andygrove

Copy link
Copy Markdown
Member

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 ignored 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 ignored 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 ignores are removed. All eight pass:

  • cast FloatType to DecimalType(10,2) and cast DoubleType to DecimalType(10,2) -- Incompatible
    for rounding differences (Make cast from float/double to decimal compatible with Spark #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 (Implement Spark-compatible CAST from timestamp to numeric types #352, Cast timestamp to decimal is unsupported #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.

`CometCast` mixes in `CodegenDispatchFallback`, so an `Incompatible` or
`Unsupported` support level no longer takes the plan out of Comet. The suite
still assumed the old compatible-or-fallback binary.

- Replace the `hasIncompatibleType` boolean with a `CastExecution` expectation
  (`Native`, `Dispatched`, `SparkFallback`) and assert the declared path from
  the verbose extended explain, not just that the plan stayed in Comet.
- Rewrite `all valid cast combinations covered` around those three outcomes: it
  now pins that no pair in the matrix leaves Comet, and rejects ignored tests
  instead of requiring them for every non-`Compatible` pair.
- Un-ignore the eight stale tests that the dispatcher now handles.
@andygrove
andygrove marked this pull request as draft August 1, 2026 02:19
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.

Revisit CometCastSuite assumptions now that Cast has a codegen dispatch fallback

1 participant