andygrove commented on PR #5136: URL: https://github.com/apache/datafusion-comet/pull/5136#issuecomment-5145724074
@mbutrovich thanks for the review — all five comments are addressed in 069b1e0c1, with replies inline on each thread. Summary: - `float_to_decimal128` uses a plain `shift as u32` cast instead of the fallible `try_into().ok()?`. - The HALF_UP-by-power-of-ten division is now a single `div_round_half_up_i128` next to `pow10_i128` in `string.rs`, called from both `float_to_decimal128` and `parse_string_to_decimal`. Collapsing them also removed a dead `checked_div`/zero-divisor guard in the string path. I left the `i256` copy in `wide_decimal_binary_expr.rs` alone as a different integer width — say the word if you want it unified here too. - `parse_decimal_notation` is rebuilt on the existing `digits_to_i128` helper, splitting off the sign and the `.` the way `parse_string_to_decimal` does. It returns `Option` so the (unreachable) overflow propagates rather than wrapping, and a new test round-trips every shape ryu emits, including `f64::MAX` and the smallest subnormal. - Float/double to decimal now reports `Compatible(Some(...))` carrying the JDK < 19 tie-rounding caveat, and the cast audit doc records the same note (it had the same gap). Verification: `cargo test -p datafusion-comet-spark-expr` 550 passed / 0 failed, clippy and `cargo fmt --check` clean, and the full `CometCastSuite` is 165 passed / 0 failed / 7 ignored. One unrelated gotcha I hit, in case it saves you time: running a filtered subset of `CometCastSuite` (e.g. `-Dsuites="…CometCastSuite DecimalType"`) makes `cast StringType to DecimalType(38,10) high precision` fail with a Spark-side `java.lang.ArithmeticException: BigInteger would overflow supported range`. `DataGenerator.DEFAULT` is seeded once and shared across the suite, so filtering shifts which random strings that test gets. It reproduces identically without any of my changes and passes in a full-suite run. -- 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]
