snmvaughan opened a new pull request, #7578:
URL: https://github.com/apache/datafusion-comet/pull/7578
## Which issue does this PR close?
Closes #7188.
## Rationale for this change
`Decimal.fromString` and `fromStringANSI` reject a string as soon as they
have parsed it if its `BigDecimal` has more than 38 integral digits (`precision
- scale`), before they check the value against the target type. Under ANSI that
raises `NUMERIC_OUT_OF_SUPPORTED_RANGE`, whatever the target type, and
otherwise the cast returns NULL. A value with up to 38 integral digits that
does not fit the target type raises `NUMERIC_VALUE_OUT_OF_RANGE` instead.
Comet returns NULL for these strings too, but under ANSI it reports another
error, because its parser fails on the digits before any digit count exists:
| Input, cast to `DECIMAL(10,2)` under ANSI | Spark | Comet before this
change |
|---|---|---|
| `'6E+38'` | `NUMERIC_OUT_OF_SUPPORTED_RANGE` | `CAST_INVALID_INPUT` (the
scale adjustment is beyond 38) |
| `'1'` and 39 zeros (40 digits) | `NUMERIC_OUT_OF_SUPPORTED_RANGE` |
`CAST_INVALID_INPUT` (the mantissa does not fit an i128) |
| `'1'` and 38 zeros (39 digits) | `NUMERIC_OUT_OF_SUPPORTED_RANGE` |
`NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` (the rescaled value overflows) |
| `'12345678901'` (11 digits, for comparison) |
`NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` |
`NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` |
With `spark.sql.legacy.allowNegativeScaleOfDecimal=true` Spark skips the
limit, for zero too. Those strings are then ordinary values that are too large
for the target type and fail with `NUMERIC_VALUE_OUT_OF_RANGE`, and a zero with
a large exponent is zero. A fix that applied the limit unconditionally would
turn the inputs that match today under that setting into mismatches, so the
setting has to reach the Rust side.
## What changes are included in this PR?
- `parse_decimal_str` applies the limit itself, as soon as it has the scale
and before the errors for digits that do not fit an i128, so a string of any
length gets the same error as one of 39 digits. The new
`exceeds_integral_digit_limit` counts the integral digits as `precision -
scale` of the `BigDecimal` that Spark parses, so leading zeros do not count and
trailing zeros do. The digits are at most those before the point, shifted by
the exponent, and that bound settles nearly every string with one comparison.
Only a string that may be over the limit has its digits counted.
- The zero rules from SPARK-60118 moved into that function unchanged. A
non-zero value is counted in an i64 on every release, as the releases with
SPARK-60118 do, because the int Spark used before wraps for an exponent near
the int limit and then fails in `BigDecimal.setScale` with a raw
`ArithmeticException` that has no counterpart here.
- A new `Cast` proto field, `decimal_skip_integral_digit_limit`, named for
the behavior and not for the setting, like `decimal_zero_any_exponent`.
`CometCast` sets it from `spark.sql.legacy.allowNegativeScaleOfDecimal`, and
`parse_string_to_decimal` skips the limit when it is set. The field number is 9
because 8 is used by #7522, which changes the same function and arguments;
whichever merges second needs a mechanical rebase.
- Docs: the cast audit entry and the cast compatibility page.
What this does not change: the error still does not reach Spark's
`SparkArithmeticException` type, because no shim converts `NumericOutOfRange`
(#7216), and a value with up to 38 integral digits whose scale adjustment to
the target type is beyond 38 (`'1e37'` to `DECIMAL(10,2)`) still reports
`CAST_INVALID_INPUT` under ANSI where Spark reports
`NUMERIC_VALUE_OUT_OF_RANGE` (#7576).
## How are these changes tested?
The expected results come from running Spark 4.1.3's `Decimal.fromString`
and `fromStringANSI`, followed by `changePrecision`, on the same inputs, with
and without the setting.
- Rust unit tests in `string.rs`:
- `test_parse_string_to_decimal_integral_digit_limit` covers 15 strings
over the limit, which fit an i128 or do not and include an exponent at the top
of the int range, against five target types and both values of
`zero_any_exponent`. It also covers the strings within the limit that parse,
the ones that do not fit the target type, and the exponents and scales outside
the int range that stay invalid inputs.
- `test_parse_string_to_decimal_skip_integral_digit_limit` covers the
setting: the same strings fail the precision check, a zero with a large
exponent is zero, and a string within the limit is unchanged.
- `test_cast_string_to_decimal_integral_digit_limit` runs the cast for
`Decimal128` and `Decimal256` in legacy, try and ANSI mode.
- `test_integral_digit_limit_bound_agrees_with_the_digit_count` compares
`exceeds_integral_digit_limit` with a count of the digits for strings of every
shape around the limit: all zeros, leading zeros and none, before and after the
point, with exponents on both sides of zero and near the ends of the int range.
The shortcut must never hide a string that is over the limit.
- `CometNativeCastSuite`:
- `cast StringType to DecimalType sets the integral-digit limit option
from the legacy config` checks the proto field for both values of the setting.
- `cast StringType to DecimalType - more than 38 integral digits` and `...
with negative scale` compare Comet with Spark in every mode with `castTest`,
one value per ANSI case so that each error is compared.
- `cast_string_to_decimal_integral_digits_ansi.sql` covers the errors under
ANSI, `try_cast`, and the same strings after `SET
spark.sql.legacy.allowNegativeScaleOfDecimal=true`.
The string-to-decimal path is hot, so I compared it with `main` using the
`cast_string_to_decimal` benchmarks in `benches/cast_from_string.rs`. I built
the benchmark binary from `main` and from this branch and ran them alternately
for five rounds, so that drift in the machine affects both (an Apple M5 Max,
macOS, rustc 1.97.1, release profile). The median of the per-round medians is
1.005 to 1.009 times `main` for `decimal_38_10` and `decimal_18_2` in legacy,
try and ANSI mode, which is about 0.2 ns per row, and the per-round ratios run
from 0.98 to 1.03. Counting the digits of every string was 4 to 8% slower on
these benchmarks, which is why the bound exists. The unicode shapes take a path
this change does not touch, and their rounds differ from each other by up to 5%.
The new tests compare Comet's error text with Spark's, and that text differs
between Spark versions. The PR tier runs the default profile only, so could a
maintainer apply `run-all-spark-profiles`? I can't add labels from a fork.
--
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]
