andygrove commented on PR #5083:
URL:
https://github.com/apache/datafusion-comet/pull/5083#issuecomment-5146647344
@mbutrovich all four threads addressed in 815cc8a6f, details inline. Summary:
**The wrong-value finding was real and would have failed CI.** I ran the new
test on `spark-3.5` before touching anything:
```
- cast BooleanType to DecimalType where 10^scale overflows precision ***
FAILED ***
"...ALUE_OUT_OF_RANGE] 1[0] cannot be represent..." did not equal
"...ALUE_OUT_OF_RANGE] 1[] cannot be represent..."
```
Exactly your diagnosis. Fixed by reporting the pre-scale logical value,
matching `cast_int_to_decimal128_internal`.
**Helper extracted, with one signature change.** `SparkResult<Option<i128>>`
taking an `i128` does not fit the float site, which reports an `f64` — coercing
it would alter the reported string and break message parity there — and the
`Option` would always be `None` since the helper only ever errors or nulls. So
it is `decimal_overflow_or_null<T: Display>(value, precision, scale, eval_mode)
-> SparkResult<()>` in `conversion_funcs/utils.rs`, generic over the displayed
value, with the pre-scale convention documented in one place. All three sites
call it; each keeps its own scan strategy, and boolean's two-valued precheck
stays outside the loop as you asked. It also let the two identical branches in
`cast_int_to_decimal128_internal` collapse into one.
**Test comment** now names the ANSI coverage and notes that the 3.4/3.5
exact comparison is what pins the reported value.
**Verified on spark-3.5:** all four `cast BooleanType to DecimalType*` tests
pass; 62 `conversion_funcs` unit tests pass; clippy `--all-targets` clean.
One unrelated thing I hit while regression-testing the refactor: `cast
StringType to DecimalType(38,10) high precision` fails when the `to
DecimalType` tests run as a batch, but passes in isolation, and it fails
identically on the base commit (base: 2 failures in that batch, with this
change: 1, the difference being the boolean test this PR fixes). The exception
comes from Spark's own `Decimal.changePrecision` → `BigInteger.pow`, so it
looks like a pre-existing order dependency, most likely a leaked
`allowNegativeScaleOfDecimal` conf from a neighbouring test. Untouched here —
worth a separate issue if it is not already known.
--
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]
