0lai0 commented on PR #5050:
URL:
https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5558329269
Thanks @andygrove and @comphead.
## Where `Unsupported` routes these casts
`CometCast` gained `CodegenDispatchFallback` after this PR branched, so
these casts route through the JVM codegen dispatcher rather than falling back
to Spark, my three `checkSparkAnswerAndFallbackReason` assertions failed after
the rebase for exactly that reason (`Accelerated expressions: 0 native, 1
codegen dispatch`).
It is not moving the problem, though. The dispatcher delegates to Spark's
own `doGenCode` (`Decimal`/`BigDecimal`), and Arrow carries the unscaled value
with the scale as metadata, `writeBigDecimalToArrowBuf` just calls
`value.unscaledValue()`. No power-of-ten arithmetic on that path. Confirmed on
unmodified `main` via `Boolean -> Decimal`, which is already dispatcher-routed:
passes for both `Decimal(10,-1)` and `Decimal(20,-5)` with no panic. `main`'s
own `round on negative-scale decimal` test relies on the same mechanism.
Tests now assert `assertCodegenRan { checkSparkAnswerAndOperator(...) }`,
and I have corrected the PR description. The arithmetic half is a genuine Spark
fallback, those five serdes don't mix the trait in.
## Parameterising over `Decimal(20,-5)` caught a real bug
```
!== Spark Answer == == Comet Answer ==
![1000000.0] [999999.9999999999]
```
`Decimal(neg) -> Double` has no Comet arm in `cast.rs`, so arrow's kernel
computes `unscaled / 10f64.powi(scale)`. For scale `-5` the divisor `1e-5` is
not exactly representable; the Comet values reproduce bit-for-bit as `10/1e-5`
etc. Spark's `BigDecimal.doubleValue()` is exact. Positive scales are fine
(`10^s` exact for `s <= 22`).
So my claim that all other directions were "unaffected" was wrong, they
avoid the *panic*, but float/double was silently wrong. `canCastFromDecimal`
now guards those two as well.
The `v * v` pin also covers `Decimal(20,-5) * Decimal(20,-5)` now, which
takes `WideDecimalBinaryExpr`'s multiply branch and yields `Decimal(38,-10)` —
a negative *output* scale.
## Other items
- **`Boolean -> Decimal(neg)`** — already fixed on `main` independently;
nothing needed after the rebase.
- **`planner.rs` underflow** — rewritten in `i16`.
- **Other rescaling sites** — all safe as written, no changes:
`Round`/`Ceil`/`Floor` already guarded; `sum_decimal.rs` has no `pow()` and
never rescales; `avg_decimal.rs`'s exponent is `(s+4) - s = 4` regardless of
sign; `decimal_rescale_check.rs` uses signed `i8` + `unsigned_abs()` and
rejects `abs_delta > 38`.
- **Docs** — new `cast.md` section (the generated matrix only samples
`Decimal(10,2)`), and the five arithmetic serdes now override
`getUnsupportedReasons()` for `math.md`.
## Consolidating the negative-scale check
Agreed, including @comphead's nested-type point
(`ArrayType(DecimalType(10,-1))` isn't matched today) and your
`exprToProtoInternal` + `SupportLevel.containsType` suggestion. It would touch
the pre-existing `Ceil`/`Floor`/`Round` guards too, so I'd rather file it as a
follow-up than grow this PR, happy to pick it up if that works.
--
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]
