sunchao commented on code in PR #6826:
URL: https://github.com/apache/datafusion-comet/pull/6826#discussion_r4253039520
##########
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:
[P2] Preserve failure for extreme negative-scale column casts. With
`spark.sql.ansi.enabled=true` and
`spark.sql.legacy.allowNegativeScaleOfDecimal=true`, read a Parquet string
column `s` containing `'1e-2147483647'` and evaluate
`df.select(col("s").cast(DecimalType(10, -2))).collect()`. Spark throws
`ArithmeticException: Underflow`. The widened subtraction now produces
`-2147483649`, reaches the zero-return branch, and silently yields zero. The
base returned an error under ANSI, so this materially worsens existing
behavior. The new literal fallback does not protect column inputs. Please
retain the widened arithmetic but detect unsupported rescaling and preserve
Spark execution or failure, with a negative-scale column regression test.
Evidence: On OpenJDK 21.0.12.1, Spark 3.5.9, 4.1.3 and 4.2.0 all threw
`ArithmeticException: Underflow` for this bound string-column cast in both
interpreted and generated execution. Release-mode reproductions using verified
base/head parser bodies and the unchanged Arrow decimal conversion loop
produced base `Err(CastInvalidValue)` versus head `Decimal128(10, -2) [0,
null]` for input `["1e-2147483647", null]` under ANSI.
`CometCast.canCastFromString` marks this target compatible, and `spark_cast`
routes it to this kernel. Reproduction:
`/tmp/pr6826-review-current-pfnzv_nz/bench/src/bin/decimal.rs`. No end-to-end
Comet query was run.
##########
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:
[P2] Avoid a second full scan for ordinary floating-point strings. This
`.all(...)` runs for every numeric column value before `parse::<F>()` scans it
again, slowing the existing `STRING` to `FLOAT`/`DOUBLE` path. In repeated
release-mode Arrow-batch measurements, ordinary double strings increased from
about 29.6 to 40.0 ns/value, and long numeric strings from about 75 to 147
ns/value. Please reject unhandled special values using a bounded prefix check,
such as the first byte after an optional sign, and leave remaining numeric
syntax validation to Rust. A disposable version of that guard retained the
special-value tests and removed most of the measured overhead.
Evidence: Verified both parser bodies and `cast_string_to_float_impl`
against the exact base/head sources. Benchmarked Arrow 59.3.0 batches of 8,192
varied strings with release settings matching Comet, seven alternating samples
of 150 batches, and two complete runs. Ordinary double inputs were generated
with `format!("{}.{:013}", i + 1000, i * 7 + 1)`. Second-run medians were 29.67
ns/value at base, 40.03 at head, and 31.30 with the prefix guard. FLOAT,
scientific notation and nullable batches reproduced the slowdown. Both head and
candidate special-value tests passed. Harness:
`/tmp/pr6826-review-current-pfnzv_nz/bench/src/main.rs`. These are
conversion-kernel measurements, not end-to-end query timings.
--
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]
