fix(array): scale decimal-to-float scalar casts across the whole scale domain - #9883
Open
jackylee-ch wants to merge 1 commit into
Open
jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
…e domain `DecimalScalar::cast` computed the scale factor as `10_i128.pow(self.decimal_type.scale() as u32)`. `DecimalDType` admits any `i8` scale up to `MAX_SCALE` (76), so that expression is wrong at both ends: - a scale above 38 overflows `i128`, which in release wraps to a garbage factor — `decimal(76,39)` storing 1 cast to `-4.8e-38` instead of `1e-39`; - a negative scale reaches `pow` as a wrapped `u32` exponent, because `-5i8 as u32` is 4294967291 — `decimal(5,-5)` storing 1 cast to `inf` instead of `100000.0`. A negative scale should scale the value up, not divide it. Take the factor in `f64` and branch on the sign, the way the array kernel `arrays::decimal::compute::cast::cast_to_f64` already does. The widest factor the domain needs is `10^128`, well inside `f64` range. Division is kept for non-negative scale rather than multiplying by the inverse, so every scale up to 22 — where `10^scale` is exactly representable — returns the same bits as before. Signed-off-by: jackylee-ch <qcsd2011@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DecimalScalar::castcomputed the scale factor for the float target as10_i128.pow(self.decimal_type.scale() as u32)(
vortex-array/src/scalar/typed_view/decimal/scalar.rs:98).DecimalDType::try_newbounds scaleabove at
MAX_SCALE(76) and not at all below, so that expression is wrong at both ends. Measuredin release:
decimal(76,39)storing1→f64-4.796830079047167e-381e-39decimal(5,-5)storing1→f64inf100000.0Above scale 38 the factor overflows
i128and wraps. A negative scale reachespowas a wrappedexponent, since
-5i8 as u32is4294967291— and semantically a negative scale should scale thevalue up, not divide by anything.
Three places in the same crate already get this right:
DecimalValue::rescale_i256(
dvalue.rs:127) widens the scale toi16before branching on its sign and takes the factorthrough the fallible
decimal_scale_factor, and the array kernelarrays::decimal::compute::cast::cast_to_f64(cast.rs:196) takes it as10f64.powi(-scale).Only the scalar float branch did not.
The extreme scales are already pinned for the integer target, on both sides:
cast_decimal_to_integer_policy(cast.rs:508) has cases at scale-2,76and-128andasserts the array and scalar casts agree, and
cast_decimal_to_integer_wide_storage(:691) doesthe same for
decimal(76,40). That target goes throughDecimalToIntegerCast, which is fine. Thefloat target had no equivalent.
Fix
Take the factor in
f64— the widest the-128..=76domain needs is10^128, well inside range —and branch on the sign.
Division is kept for non-negative scale rather than multiplying by the inverse, so every scale up
to 22, where
10^scaleis exactly representable, returns the same bits as before. Multiplying bythe inverse instead would shift
12345 / 100by an ulp.The
i128funnel above (to_i128, which errors for ani256value wider thani128where thearray kernel yields a float) is a third scalar/array divergence, left alone here: turning that
Errinto anOkis a semantics change, not a bug fix. The neighbouringTODO(connor)stillapplies to the remaining
ascasts.Tests
cargo test --release -p vortex-array --no-fail-fast: 3535 + 73 + 1 passed, 3526 + 73 + 1 before.Two
arrays::listview- should panictests fail identically before and after — they rely ondebug_assert, which is off in release.Restoring the old expression fails all five scale-domain cases and the
f32case while leaving thethree exactness cases green, which is what shows the common path is untouched.
Scale 23..=38 can shift by up to ~2 ulp, since
powiaccumulates rounding where the old exacti128factor did not. NoDecimalDTypein the tree uses a scale in that band: the literal scalesacross the workspace are
{-128, -67, -19, -11, -10, -8, -5, -4, -2, 0..6, 10, 20, 40}.AI assistance
Written with agentic AI assistance; I reproduced both wrong values before changing anything, and
the first version of the fix (multiply by the inverse, matching the array kernel exactly) was
rejected by its own exactness test.