mbutrovich commented on code in PR #4916:
URL: https://github.com/apache/datafusion-comet/pull/4916#discussion_r3591105426
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -469,6 +469,71 @@ fn normalize_fullwidth_digits(s: &str) -> String {
unsafe { String::from_utf8_unchecked(out) }
}
+/// Powers of ten that fit in an `i128` (`10^0` through `10^38`).
+const POW10_I128: [i128; 39] = {
+ let mut table = [1i128; 39];
+ let mut i = 1;
+ while i < 39 {
+ table[i] = table[i - 1] * 10;
+ i += 1;
+ }
+ table
+};
+
+/// `10^exp`, using the precomputed table for the range that fits in an `i128`.
+#[inline]
+fn pow10_i128(exp: u32) -> i128 {
+ match POW10_I128.get(exp as usize) {
+ Some(v) => *v,
+ None => 10_i128.pow(exp),
+ }
+}
+
+/// Accumulate an ASCII-digit slice into an `i128`, returning `None` on
overflow.
+///
+/// The first 38 digits always fit (`i128::MAX` is ~1.7e38), so only the
digits past
+/// them need the per-digit overflow checks.
+#[inline]
+fn digits_to_i128(digits: &[u8]) -> Option<i128> {
Review Comment:
`digits_to_i128` at `native/spark-expr/src/conversion_funcs/string.rs` (new
helper) splits the digit slice at 38 and skips overflow checks on the first 38
digits, relying on the fact that 38 nines (about 9.99e37) fit under `i128::MAX`
(about 1.7e38). The reasoning is correct, and leading zeros are handled
correctly because zeros in the head accumulate to 0 and the tail uses
`checked_mul`/`checked_add`. But this boundary is exactly where a future edit
could regress, and the current Scala fuzz tests
(`CometCastSuite.scala:958-961`) cap the generated digit count at 38, so they
never exercise a 39-plus-digit integral part.
Suggested change: add a Rust unit test in the `#[cfg(test)]` block of
`string.rs` asserting parse results for a 38-digit value (parses), a 39-digit
value (overflows to the `invalid_decimal_cast` error / NULL in non-ANSI), and a
40-digit value with leading zeros that reduce to a small value (parses). This
locks the boundary that the optimization now depends on.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -469,6 +469,71 @@ fn normalize_fullwidth_digits(s: &str) -> String {
unsafe { String::from_utf8_unchecked(out) }
}
+/// Powers of ten that fit in an `i128` (`10^0` through `10^38`).
+const POW10_I128: [i128; 39] = {
+ let mut table = [1i128; 39];
+ let mut i = 1;
+ while i < 39 {
+ table[i] = table[i - 1] * 10;
+ i += 1;
+ }
+ table
+};
+
+/// `10^exp`, using the precomputed table for the range that fits in an `i128`.
+#[inline]
+fn pow10_i128(exp: u32) -> i128 {
Review Comment:
`pow10_i128` falls back to `10_i128.pow(exp)` on a table miss (`exp >= 39`).
Two of the three call sites are guarded by `scale_adjustment > 38` /
`abs_scale_adjustment > 38` checks, so they never miss. The
`checked_mul(pow10_i128(fractional_scale as u32))` combine step at the end of
`parse_decimal_str`, however, is not bounded: `fractional_scale` is
`fractional_part.len()`, so an input like `"0." + "0"*40` calls
`pow10_i128(40)`, misses the table, and evaluates `10_i128.pow(40)`, which
panics in debug and wraps in release. This is identical to the old code
(`10_i128.pow(fractional_scale as u32)`), so the PR introduces no regression,
but the new named helper is the natural place to make the fallback total.
Suggested change: make `pow10_i128` return `Option<i128>` (or add a
`checked_pow10` variant) and thread it through the combine step so an over-long
fraction produces the `invalid_decimal_cast` error path instead of a panic. If
you would rather keep this PR scoped to performance, leave a one-line comment
on `pow10_i128` noting that callers must keep `exp <= 38` and that the
`10_i128.pow` fallback is unreachable in current callers, so it is not silently
masking a bug.
##########
native/spark-expr/benches/cast_string_to_decimal.rs:
##########
@@ -0,0 +1,93 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{builder::StringBuilder, RecordBatch};
+use arrow::datatypes::{DataType, Field, Schema};
+use criterion::{criterion_group, criterion_main, Criterion};
+use datafusion::physical_expr::{expressions::Column, PhysicalExpr};
+use datafusion_comet_spark_expr::{Cast, EvalMode, SparkCastOptions};
+use rand::rngs::StdRng;
+use rand::{RngExt, SeedableRng};
+use std::hint::black_box;
+use std::sync::Arc;
+
+/// A batch of decimal strings covering the shapes Spark sees in practice:
plain
+/// integers, fixed-point values, negatives, and scientific notation.
+fn create_decimal_string_batch(size: usize) -> RecordBatch {
+ let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Utf8,
true)]));
+ let mut rng = StdRng::seed_from_u64(42);
+ let mut b = StringBuilder::new();
+ for i in 0..size {
+ if i % 10 == 0 {
+ b.append_null();
+ } else {
+ match i % 5 {
+ 0 => b.append_value(format!(
+ "{}.{}",
+ rng.random_range(0..1_000_000u32),
+ rng.random_range(0..100_000u32)
+ )),
+ 1 => b.append_value(format!(
+ "{}.{}E{}",
+ rng.random_range(0..10u32),
+ rng.random_range(0..100u32),
+ rng.random_range(0..10u32)
+ )),
+ 2 => b.append_value(format!(
+ "-{}.{}",
+ rng.random_range(0..1_000_000u32),
+ rng.random_range(0..100_000u32)
+ )),
+ 3 => b.append_value(format!("{}",
rng.random_range(-1_000_000..1_000_000i32))),
+ _ => b.append_value(format!("0.{:05}",
rng.random_range(0..100_000u32))),
+ }
+ }
+ }
+ RecordBatch::try_new(schema, vec![Arc::new(b.finish())]).unwrap()
+}
+
+fn criterion_benchmark(c: &mut Criterion) {
+ let batch = create_decimal_string_batch(8192);
+ let expr = Arc::new(Column::new("a", 0));
+
+ for (mode, mode_name) in [
+ (EvalMode::Legacy, "legacy"),
+ (EvalMode::Ansi, "ansi"),
+ (EvalMode::Try, "try"),
+ ] {
+ let mut group =
c.benchmark_group(format!("cast_string_to_decimal/{mode_name}"));
Review Comment:
`native/spark-expr/benches/cast_string_to_decimal.rs:72` creates group
`cast_string_to_decimal/{mode_name}` with a `decimal_38_10` bench function. The
existing `native/spark-expr/benches/cast_from_string.rs:124-131` already
creates the identical group `cast_string_to_decimal/{mode_name}` with an
identical `decimal_38_10` function, over the same three eval modes and the same
string shapes (plain, fixed-point, negative, scientific). Criterion keys saved
results by `group/function`, so the two benches write to and overwrite the same
`target/criterion/cast_string_to_decimal/legacy/decimal_38_10` directory.
Whichever bench runs last clobbers the other's baseline, which makes the
reported before/after comparison unreliable.
The only thing the new file adds over the existing one is the `decimal_18_2`
variant and a seeded `StdRng` (the existing bench uses unseeded `rand::random`,
so its numbers are not reproducible run to run).
Suggested change: do not add a second benchmark file. Fold the improvements
into the existing `create_decimal_cast_string_batch` path in
`cast_from_string.rs`: switch it to a seeded `StdRng` and add the
`decimal_18_2` data type to the existing loop at `cast_from_string.rs:116-133`.
Delete `benches/cast_string_to_decimal.rs` and the corresponding `[[bench]]`
entry in `Cargo.toml`. If you keep a separate file, rename its group to
something distinct like `parse_string_to_decimal/...` so it does not overwrite
the existing baseline.
--
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]
