andygrove opened a new pull request, #5185:
URL: https://github.com/apache/datafusion-comet/pull/5185
## Which issue does this PR close?
N/A
## Rationale for this change
Comet has a hand-written native kernel for casting `boolean` to `decimal`.
This is not worth
carrying:
- **Nobody uses it.** Casting a boolean to a decimal is a genuine edge case.
There is no
realistic query shape where a user needs `1`/`0` as a scaled decimal
rather than as an
integer or a double.
- **It is disproportionately complex to implement.** The native kernel has
to reproduce
Spark's full `Decimal.toPrecision` semantics: pick the unscaled value for
the target scale,
decide whether it fits the target precision, and then distinguish non-ANSI
(return NULL),
ANSI (throw a Spark-compatible `NUMERIC_VALUE_OUT_OF_RANGE` error), and
`try_cast` (return
NULL) behavior. Overflow is value-dependent -- `false` scales to `0`,
which fits every
decimal type, while `true` overflows anything that cannot hold `1`
(`decimal(1,1)`,
`decimal(2,2)`, `decimal(38,38)`). That is a lot of surface area, and a
lot of ways to
silently diverge from Spark, for a cast with no real users.
Since `CometCast` mixes in `CodegenDispatchFallback`, marking the cast
unsupported does not
push the plan back to Spark. The JVM codegen dispatcher runs Spark's own
generated code for
the cast inside the Comet pipeline, so the enclosing projection still
executes natively and
the results are Spark-compatible by construction.
## What changes are included in this PR?
- `CometCast.canCastFromBoolean` no longer reports `DecimalType` as
`Compatible`, so
boolean -> decimal is routed through the codegen dispatcher.
- Removed the native `cast_boolean_to_decimal` kernel, its dispatch arm in
`cast_array`, and
its Criterion benchmark.
- The Rust unit test now asserts that the native cast rejects boolean ->
decimal, pinning the
fact that the planner must not send it to the native path.
- `CometCastSuite`: the `cast BooleanType to DecimalType(10,2)` test is
ignored to satisfy the
matrix-consistency assertion in `all valid cast combinations covered`, and
a new test pins
the `Unsupported` support level. The `DecimalType(14,4)` and
`DecimalType(30,0)` tests stay
active and now exercise the dispatch path.
- New SQL file tests `cast_boolean_to_decimal.sql` and
`cast_boolean_to_decimal_ansi.sql`
covering both modes. Non-ANSI covers the long fast path (`precision <=
18`) and the
`BigDecimal` path (`precision > 18`), the tightest representable types,
overflow returning
NULL, literals, `try_cast`, predicates, and aggregates. The ANSI file
covers the edge cases:
overflow throwing `NUMERIC_VALUE_OUT_OF_RANGE` for `decimal(1,1)`,
`decimal(2,2)`, and
`decimal(38,38)`; overflow raised from inside an aggregate and from inside
a filter
predicate; `false` and `NULL` inputs *not* throwing even for those types;
and `try_cast`
suppressing the error. The non-error queries double as sentinels -- they
assert fully native
execution, so a silent fallback to Spark would fail the test rather than
let the
`expect_error` queries pass vacuously.
## How are these changes tested?
New SQL file tests:
```
./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite
cast_boolean_to_decimal" -Dtest=none
```
Both files pass, which also confirms the cast stays inside the Comet
pipeline via the codegen
dispatcher rather than falling the plan back to Spark. Rust unit tests in
`conversion_funcs::boolean` also pass.
--
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]
