viirya commented on code in PR #6826:
URL: https://github.com/apache/datafusion-comet/pull/6826#discussion_r4253231874
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -616,9 +626,9 @@ fn parse_string_to_decimal(input_str: &str, precision: u8,
scale: i8) -> SparkRe
return Ok(Some(0));
}
- // scale adjustment
- let target_scale = scale as i32;
- let scale_adjustment = target_scale - exponent;
+ // scale adjustment, in i64 so that an exponent near the i32 bounds cannot
wrap
+ let target_scale = scale as i64;
+ let scale_adjustment = target_scale - exponent as i64;
Review Comment:
Good catch, the widening removed an accidental failure.
`BigDecimal.setScale`, which Spark rescales with, fails when the difference
between the parsed scale and the target scale does not fit in an int (its
`checkScale` throws `Underflow`/`Overflow`), so 6e04baad8 treats that as
invalid input instead of rounding to zero. This restores the base behavior for
your example. Added a Rust case and a column regression test with
`allowNegativeScaleOfDecimal=true` that asserts both Spark and Comet fail.
There is a remaining gap that predates this PR: when the scale difference
fits in an int but is roughly above 6e8, Spark fails with `BigInteger would
overflow supported range` while the native cast rounds to zero, as it did
before this PR. The exact threshold depends on the JDK's BigInteger limits, and
below it Spark rounds to zero only after computing a huge power of ten. Spark
raises a raw `ArithmeticException` in every eval mode here, including
`try_cast`, which looks like a Spark-side issue, so I'd prefer not to encode
JDK-specific thresholds in this PR.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -254,6 +255,15 @@ where
} else {
s
};
+ // Every special value Spark accepts is handled above. Java only allows
the suffix after a
+ // decimal number, so reject anything else before Rust's parser, which
also accepts
+ // `nan`, `inf` and `infinity` in any case (e.g. `NaNF` or `-nan`).
+ if !pruned_float_str
+ .bytes()
+ .all(|b| b.is_ascii_digit() || matches!(b, b'.' | b'e' | b'E' | b'+' |
b'-'))
Review Comment:
Thanks, confirmed with a release-mode microbenchmark: 11.1 → 16.3 ns/value
for ordinary strings and 31.0 → 45.9 for long ones. 6e04baad8 checks only the
first byte after the optional sign, since `nan`, `inf` and `infinity` are the
only non-numeric forms Rust's parser accepts and they all start with a letter.
That brings it to 11.5 and 31.5 ns/value. The special-value tests still 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]
