mbutrovich commented on code in PR #4924:
URL: https://github.com/apache/datafusion-comet/pull/4924#discussion_r3591134045
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -1424,5 +1495,14 @@ mod tests {
assert_eq!(fmt(1, -2), "1E+2");
assert_eq!(fmt(123, -2), "1.23E+4");
assert_eq!(fmt(-123, -2), "-1.23E+4");
+
+ // values needing more than one 64-bit digit group
+ assert_eq!(fmt(10_000_000_000_000_000_000, 0), "10000000000000000000");
+ assert_eq!(fmt(i128::MAX, 0), i128::MAX.to_string());
+ assert_eq!(fmt(i128::MIN, 0), i128::MIN.to_string());
+ assert_eq!(
+ fmt(99_999_999_999_999_999_999_999_999_999_999_999_999, 10),
+ "9999999999999999999999999999.9999999999"
Review Comment:
`native/spark-expr/src/conversion_funcs/numeric.rs:1499-1506` (new tests)
covers `i128::MAX`, `i128::MIN`, and the max-precision plain-notation
coefficient, but every multi-group case is either non-negative or in plain
notation. The sign push and the coefficient-splitting for scientific notation
(`&coeff[..1]` / `&coeff[1..]`) are the parts most likely to regress if someone
later refactors `render_digits`, and neither is exercised on a coefficient that
spans two 64-bit groups.
Suggested change: add cases such as
```rust
// multi-group coefficient in scientific notation, both signs
assert_eq!(fmt(i128::MAX, 45),
"1.70141183460469231731687303715884105727E-7");
assert_eq!(fmt(-i128::MAX, 45),
"-1.70141183460469231731687303715884105727E-7");
```
(compute the expected strings from `BigDecimal(unscaled, scale).toString()`
or the pre-PR implementation so they are anchored to Spark, not to the new
code).
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -1424,5 +1495,14 @@ mod tests {
assert_eq!(fmt(1, -2), "1E+2");
assert_eq!(fmt(123, -2), "1.23E+4");
assert_eq!(fmt(-123, -2), "-1.23E+4");
+
+ // values needing more than one 64-bit digit group
+ assert_eq!(fmt(10_000_000_000_000_000_000, 0), "10000000000000000000");
Review Comment:
`native/spark-expr/src/conversion_funcs/numeric.rs:1500` tests
`10_000_000_000_000_000_000` (scale 0), which is the smallest two-group value,
but no test covers a value like `10^19 + 5` where the lower group is
`0000000000000000005` and the emitted leading zeros of that group are
load-bearing. This is exactly the case the code comment at `render_digits`
calls out ("emitting its leading zeroes is correct"), so it deserves a guard
test.
Suggested change:
```rust
assert_eq!(fmt(10_000_000_000_000_000_005, 0), "10000000000000000005");
```
--
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]
