Skip to content

feat: remove native cast from boolean to decimal - #5185

Open
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:remove-native-bool-to-decimal-cast
Open

feat: remove native cast from boolean to decimal#5185
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:remove-native-bool-to-decimal-cast

Conversation

@andygrove

Copy link
Copy Markdown
Member

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.

Boolean -> Decimal is an edge case that nobody uses in practice, and the
native implementation has to reproduce Spark's precision/scale and
overflow semantics for it. That is not worth the complexity, so mark the
cast unsupported in `CometCast` and let the `CodegenDispatchFallback`
mixin route it through Spark's own generated code inside the Comet
pipeline. The projection still runs natively; only the cast itself is
evaluated by Spark's codegen.

Adds SQL file tests covering non-ANSI and ANSI behavior, including the
value-dependent overflow edge cases.
// `CometCast.isSupported` reports it as unsupported and the matrix-consistency test above
// requires this one to be ignored. Execution coverage, including the ANSI overflow edge cases,
// lives in `sql-tests/expressions/cast/cast_boolean_to_decimal{,_ansi}.sql`.
ignore("cast BooleanType to DecimalType(10,2)") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this ignored test? are we planning to switch it back after future changes?

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.

2 participants