comphead commented on code in PR #7578:
URL: https://github.com/apache/datafusion-comet/pull/7578#discussion_r4532817652
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -2446,16 +2563,16 @@ mod tests {
fn test_parse_string_to_decimal_boundary() {
// 38-digit integral parses (fits i128).
let s38 = "9".repeat(38);
- assert!(parse_string_to_decimal(&s38, 38, 0, false)
+ assert!(parse_string_to_decimal(&s38, 38, 0, false, false)
.unwrap()
.is_some());
// 39-digit integral overflows i128, so returns the
invalid_decimal_cast error.
Review Comment:
This comment is stale now. A 39-digit integral part hits the new limit and
returns `NUMERIC_OUT_OF_SUPPORTED_RANGE`, so it never reaches the i128 overflow
in `digits_to_i128`. `is_err()` still passes, so nothing flags it. Since the
same 39-nines case is in `over_integral_digit_limit()`, could this assertion be
dropped, or pinned to the error class with the comment updated?
##########
spark/src/test/scala/org/apache/comet/CometNativeCastSuite.scala:
##########
@@ -1555,6 +1555,58 @@ class CometNativeCastSuite
Some(SparkFixes.castsZeroWithAnyExponentToDecimal(org.apache.spark.SPARK_VERSION)))
}
+ test(
+ "cast StringType to DecimalType sets the integral-digit limit option from
the legacy config") {
+ val a = AttributeReference("a", StringType)()
+ Seq(true, false).foreach { allowNegativeScale =>
+ withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" ->
allowNegativeScale.toString) {
+ val proto = CometCast.convert(Cast(a, DecimalType(38, 10)), Seq(a),
binding = false)
+ assert(proto.map(_.getCast.getDecimalSkipIntegralDigitLimit) ==
Some(allowNegativeScale))
+ }
+ }
+ }
+
+ // Spark rejects a string whose integral part has more than 38 digits as
soon as it has parsed
+ // it, before it checks the value against the target type. So ANSI mode
raises
+ // NUMERIC_OUT_OF_SUPPORTED_RANGE whatever the target type, where a value
with up to 38 integral
+ // digits that does not fit raises NUMERIC_VALUE_OUT_OF_RANGE, and the other
modes return NULL.
+ // See https://github.com/apache/datafusion-comet/issues/7188.
+ test("cast StringType to DecimalType - more than 38 integral digits") {
Review Comment:
The values in these two `castTest` cases are already in
`cast_string_to_decimal_integral_digits_ansi.sql` (ids 1 to 7 and 20/21, the
ANSI errors, `try_cast`, and the same strings after `SET
spark.sql.legacy.allowNegativeScaleOfDecimal=true`). As far as I can see the
Scala versions add the non-ANSI `cast` returning NULL, the exact error text on
3.4, and `0e5887677`/`+0e83`. A `SET spark.sql.ansi.enabled=false` section in
the SQL file plus those values would cover most of that, leaving the
proto-option test as the only Scala addition. Is the exact message comparison
on 3.4 the reason to keep them here?
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -2804,19 +2921,331 @@ mod tests {
("0e2147483646", 10, 2),
("0.0e2147483647", 10, 2),
] {
- let err = parse_string_to_decimal(s, precision, scale,
false).unwrap_err();
+ let err = parse_string_to_decimal(s, precision, scale, false,
false).unwrap_err();
assert!(
err.to_string().contains("NUMERIC_OUT_OF_SUPPORTED_RANGE"),
"{s}: {err}"
);
assert_eq!(
- parse_string_to_decimal(s, precision, scale, true).unwrap(),
+ parse_string_to_decimal(s, precision, scale, true,
false).unwrap(),
Some(0),
"{s}"
);
}
}
+ /// Strings whose integral part has more than 38 digits, which Spark
rejects as soon as it has
+ /// parsed them. It counts `precision - scale` of the `BigDecimal`, so
leading zeros do not
+ /// count, and neither the digits nor the exponent has to fit an i128 for
the count to apply.
+ fn over_integral_digit_limit() -> Vec<String> {
+ let one_and_zeros = |zeros: usize| format!("1{}", "0".repeat(zeros));
+ vec![
+ "6E+38".to_string(),
+ "1e38".to_string(),
+ "+1e39".to_string(),
+ "-1.5e50".to_string(),
+ "0.1e40".to_string(),
+ format!("{}1e38", "0".repeat(40)),
+ // 39 digits. The first two fit in an i128 and the third does not.
+ one_and_zeros(38),
+ format!("-{}", one_and_zeros(38)),
+ "9".repeat(39),
+ // 40 digits and more, which do not fit in an i128
+ one_and_zeros(39),
+ format!("{}.5", "9".repeat(40)),
+ format!("{}.{}", "9".repeat(39), "9".repeat(10)),
+ // an exponent at the top of the int range, which Spark counts in
a long since
+ // SPARK-60118
+ "1e2147483647".to_string(),
+ "1.5e2147483647".to_string(),
+ "12e2147483647".to_string(),
+ ]
+ }
+
+ /// Without `spark.sql.legacy.allowNegativeScaleOfDecimal`, Spark rejects
such a string before it
+ /// rescales it, whatever the target type, so the error is
NUMERIC_OUT_OF_SUPPORTED_RANGE and
+ /// not the one the digits would give when the cast goes on: an invalid
input for digits or a
+ /// scale adjustment that do not fit an i128, or
NUMERIC_VALUE_OUT_OF_RANGE for a value that
+ /// does not fit the target type. A string with up to 38 integral digits
goes on as before.
+ #[test]
+ fn test_parse_string_to_decimal_integral_digit_limit() {
+ for s in over_integral_digit_limit() {
+ for (precision, scale) in [(10, 2), (38, 10), (38, 0), (38, 38),
(1, 0)] {
+ for zero_any_exponent in [false, true] {
+ let err =
+ parse_string_to_decimal(&s, precision, scale,
zero_any_exponent, false)
+ .unwrap_err();
+ assert!(
+
err.to_string().contains("NUMERIC_OUT_OF_SUPPORTED_RANGE"),
+ "{s} to DECIMAL({precision},{scale}): {err}"
+ );
+ // The error reports the string as given.
+ assert!(err.to_string().contains(&s), "{s}: {err}");
+ }
+ }
+ }
+
+ // 38 integral digits are within the limit. Leading zeros do not
count, and trailing
+ // zeros do, so 39 digits with a scale of 38 are one integral digit.
+ let nines = "9".repeat(38);
+ let one_and_37_zeros = format!("1{}", "0".repeat(37));
+ let one_and_38_zeros = format!("1{}", "0".repeat(38));
+ let padded = format!("{}1", "0".repeat(50));
+ let trailing_zeros = format!("{one_and_38_zeros}e-38");
+ for (s, precision, scale, expected) in [
+ (nines.as_str(), 38, 0, Some(10_i128.pow(38) - 1)),
+ (one_and_37_zeros.as_str(), 38, 0, Some(10_i128.pow(37))),
+ ("1e37", 38, 0, Some(10_i128.pow(37))),
+ (padded.as_str(), 10, 2, Some(100)),
+ (trailing_zeros.as_str(), 10, 2, Some(100)),
+ ("0.000001", 10, 2, Some(0)),
+ ("1e-50", 10, 2, Some(0)),
+ ] {
+ assert_eq!(
+ parse_string_to_decimal(s, precision, scale, false,
false).unwrap(),
+ expected,
+ "{s} to DECIMAL({precision},{scale})"
+ );
+ }
+ // A value that is within the limit but does not fit the target type
is a different error.
+ for (s, precision, scale) in [
+ ("12345678901", 10, 2),
+ (nines.as_str(), 10, 2),
+ (one_and_37_zeros.as_str(), 10, 2),
+ ("1e30", 10, 2),
+ ] {
+ let err = parse_string_to_decimal(s, precision, scale, false,
false).unwrap_err();
+ assert!(
+ err.to_string().contains("NUMERIC_VALUE_OUT_OF_RANGE"),
+ "{s} to DECIMAL({precision},{scale}): {err}"
+ );
+ }
+
+ // An exponent or scale outside the int range is not a number to
`BigDecimal`, so it is
+ // an invalid input and not a value with too many digits.
+ for s in ["1e2147483648", "1e-2147483649", "1.5e-2147483648"] {
+ let err = parse_string_to_decimal(s, 10, 2, false,
false).unwrap_err();
+ assert!(err.to_string().contains("CAST_INVALID_INPUT"), "{s}:
{err}");
+ }
+ }
+
+ /// With `spark.sql.legacy.allowNegativeScaleOfDecimal`, Spark skips the
limit. A value that
+ /// has too many integral digits for the target type then fails the
precision check like any
+ /// other, and a zero with a large exponent is zero, whether or not Spark
has SPARK-60118.
+ #[test]
+ fn test_parse_string_to_decimal_skip_integral_digit_limit() {
+ let one_and_38_zeros = format!("1{}", "0".repeat(38));
+ for (s, precision, scale) in [
+ (one_and_38_zeros.as_str(), 10, 2),
+ (one_and_38_zeros.as_str(), 38, 0),
+ ] {
+ let err = parse_string_to_decimal(s, precision, scale, false,
true).unwrap_err();
+ assert!(
+ err.to_string().contains("NUMERIC_VALUE_OUT_OF_RANGE"),
+ "{s} to DECIMAL({precision},{scale}): {err}"
+ );
+ }
+ for s in ["0e38", "0E73", "+0e83", "0e5887677", "0.0e2147483647"] {
+ for zero_any_exponent in [false, true] {
+ assert_eq!(
+ parse_string_to_decimal(s, 10, 2, zero_any_exponent,
true).unwrap(),
+ Some(0),
+ "{s} zero_any_exponent={zero_any_exponent}"
+ );
+ }
+ }
+ // Two shapes that are too large for the target type but still do not
report
+ // NUMERIC_VALUE_OUT_OF_RANGE as Spark does: a scale adjustment over
38 returns `None`,
+ // which the cast reports as CAST_INVALID_INPUT under ANSI (#7576),
and more digits than
+ // an i128 holds are rejected as an invalid input (#7581).
+ assert_eq!(
+ parse_string_to_decimal("6E+38", 10, 2, false, true).unwrap(),
+ None
+ );
+ let forty_digits = format!("1{}", "0".repeat(39));
+ let err = parse_string_to_decimal(&forty_digits, 10, 2, false,
true).unwrap_err();
+ assert!(err.to_string().contains("CAST_INVALID_INPUT"), "{err}");
+ // The scale adjustment of "6E+38" to a negative-scale target is 8, so
it is a value.
+ assert_eq!(
+ parse_string_to_decimal("6E+38", 10, -30, false, true).unwrap(),
+ Some(600_000_000)
+ );
+ // The setting changes nothing for a value within the limit.
+ for s in ["123", "-1.005", "1e-50", "12345678901", "1e30"] {
+ for zero_any_exponent in [false, true] {
+ let with_setting = parse_string_to_decimal(s, 10, 2,
zero_any_exponent, true);
+ let without = parse_string_to_decimal(s, 10, 2,
zero_any_exponent, false);
+ assert_eq!(
+ with_setting.map_err(|e| e.to_string()),
+ without.map_err(|e| e.to_string()),
+ "{s}"
+ );
+ }
+ }
+ }
+
+ /// `exceeds_integral_digit_limit` settles most strings from an upper
bound on their integral
+ /// digits and counts the digits only for the rest. The bound must never
hide a string that is
+ /// over the limit, so compare the function 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 it and near the ends of the int range.
+ #[test]
+ fn test_integral_digit_limit_bound_agrees_with_the_digit_count() {
+ // The integral digits are `precision - scale` of the parsed
`BigDecimal`.
+ let reference = |integral: &str, fractional: &str, scale: i32,
zero_any_exponent: bool| {
+ let digits = format!("{integral}{fractional}")
+ .trim_start_matches('0')
+ .len();
+ if digits == 0 {
+ !zero_any_exponent && 1i32.wrapping_sub(scale) > 38
+ } else {
+ digits as i64 - scale as i64 > 38
+ }
+ };
+ let shapes = |len: usize| -> Vec<String> {
+ let mut shapes = vec!["0".repeat(len)];
+ if len > 0 {
+ shapes.push(format!("{}1", "0".repeat(len - 1)));
+ shapes.push(format!("1{}", "0".repeat(len - 1)));
+ }
+ shapes
+ };
+ let exponents = (-50..=50).chain([i32::MIN + 1000, i32::MAX - 1000]);
+ let mut over = 0;
+ let mut within = 0;
+ for exponent in exponents {
+ for integral_len in 0..=44 {
+ for fractional_len in [0, 1, 2, 10, 37, 38, 39, 40, 44] {
+ if integral_len + fractional_len == 0 {
+ continue;
+ }
+ let scale = i32::try_from(fractional_len as i64 - exponent
as i64).unwrap();
+ for integral in shapes(integral_len) {
+ for fractional in shapes(fractional_len) {
+ for zero_any_exponent in [false, true] {
+ let expected =
+ reference(&integral, &fractional, scale,
zero_any_exponent);
+ let actual = exceeds_integral_digit_limit(
+ integral.as_bytes(),
+ fractional.as_bytes(),
+ exponent,
+ scale,
+ zero_any_exponent,
+ );
+ assert_eq!(
+ actual, expected,
+ "{integral}.{fractional}e{exponent}
zero_any_exponent={zero_any_exponent}"
+ );
+ if expected {
+ over += 1;
+ } else {
+ within += 1;
+ }
+ }
+ }
+ }
+ }
+ }
+ }
+ // Both outcomes are common enough for the comparison to mean
something.
+ assert!(
+ over > 10_000 && within > 10_000,
+ "over={over} within={within}"
+ );
+ }
+
+ /// The limit reaches both decimal widths from `SparkCastOptions`: NULL
for a string over it in
+ /// legacy and try mode and NUMERIC_OUT_OF_SUPPORTED_RANGE under ANSI,
unless
+ /// `decimal_skip_integral_digit_limit` is set. Each string is its own
array because ANSI mode
+ /// stops at the first row that fails.
+ #[test]
+ fn test_cast_string_to_decimal_integral_digit_limit() ->
DataFusionResult<()> {
Review Comment:
`test_parse_string_to_decimal_integral_digit_limit`,
`test_parse_string_to_decimal_skip_integral_digit_limit` and this test assert
the same 39-digit string under the setting and the `6E+38` and 40-digit shapes
pinned to #7576 and #7581. The SQL file and the Scala suite cover them too.
What this test adds is the `Err` to NULL mapping per eval mode and the
`Decimal256` wiring. Could it keep only that, with one string per error class?
That also leaves fewer places to edit when #7576 and #7581 land.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -2804,19 +2921,331 @@ mod tests {
("0e2147483646", 10, 2),
("0.0e2147483647", 10, 2),
] {
- let err = parse_string_to_decimal(s, precision, scale,
false).unwrap_err();
+ let err = parse_string_to_decimal(s, precision, scale, false,
false).unwrap_err();
assert!(
err.to_string().contains("NUMERIC_OUT_OF_SUPPORTED_RANGE"),
"{s}: {err}"
);
assert_eq!(
- parse_string_to_decimal(s, precision, scale, true).unwrap(),
+ parse_string_to_decimal(s, precision, scale, true,
false).unwrap(),
Some(0),
"{s}"
);
}
}
+ /// Strings whose integral part has more than 38 digits, which Spark
rejects as soon as it has
+ /// parsed them. It counts `precision - scale` of the `BigDecimal`, so
leading zeros do not
+ /// count, and neither the digits nor the exponent has to fit an i128 for
the count to apply.
+ fn over_integral_digit_limit() -> Vec<String> {
+ let one_and_zeros = |zeros: usize| format!("1{}", "0".repeat(zeros));
+ vec![
+ "6E+38".to_string(),
+ "1e38".to_string(),
+ "+1e39".to_string(),
+ "-1.5e50".to_string(),
+ "0.1e40".to_string(),
+ format!("{}1e38", "0".repeat(40)),
+ // 39 digits. The first two fit in an i128 and the third does not.
+ one_and_zeros(38),
+ format!("-{}", one_and_zeros(38)),
+ "9".repeat(39),
+ // 40 digits and more, which do not fit in an i128
+ one_and_zeros(39),
+ format!("{}.5", "9".repeat(40)),
+ format!("{}.{}", "9".repeat(39), "9".repeat(10)),
+ // an exponent at the top of the int range, which Spark counts in
a long since
+ // SPARK-60118
+ "1e2147483647".to_string(),
+ "1.5e2147483647".to_string(),
+ "12e2147483647".to_string(),
+ ]
+ }
+
+ /// Without `spark.sql.legacy.allowNegativeScaleOfDecimal`, Spark rejects
such a string before it
+ /// rescales it, whatever the target type, so the error is
NUMERIC_OUT_OF_SUPPORTED_RANGE and
+ /// not the one the digits would give when the cast goes on: an invalid
input for digits or a
+ /// scale adjustment that do not fit an i128, or
NUMERIC_VALUE_OUT_OF_RANGE for a value that
+ /// does not fit the target type. A string with up to 38 integral digits
goes on as before.
+ #[test]
+ fn test_parse_string_to_decimal_integral_digit_limit() {
+ for s in over_integral_digit_limit() {
+ for (precision, scale) in [(10, 2), (38, 10), (38, 0), (38, 38),
(1, 0)] {
+ for zero_any_exponent in [false, true] {
+ let err =
+ parse_string_to_decimal(&s, precision, scale,
zero_any_exponent, false)
+ .unwrap_err();
+ assert!(
+
err.to_string().contains("NUMERIC_OUT_OF_SUPPORTED_RANGE"),
+ "{s} to DECIMAL({precision},{scale}): {err}"
+ );
+ // The error reports the string as given.
+ assert!(err.to_string().contains(&s), "{s}: {err}");
+ }
+ }
+ }
+
+ // 38 integral digits are within the limit. Leading zeros do not
count, and trailing
+ // zeros do, so 39 digits with a scale of 38 are one integral digit.
+ let nines = "9".repeat(38);
+ let one_and_37_zeros = format!("1{}", "0".repeat(37));
+ let one_and_38_zeros = format!("1{}", "0".repeat(38));
+ let padded = format!("{}1", "0".repeat(50));
+ let trailing_zeros = format!("{one_and_38_zeros}e-38");
+ for (s, precision, scale, expected) in [
+ (nines.as_str(), 38, 0, Some(10_i128.pow(38) - 1)),
+ (one_and_37_zeros.as_str(), 38, 0, Some(10_i128.pow(37))),
+ ("1e37", 38, 0, Some(10_i128.pow(37))),
+ (padded.as_str(), 10, 2, Some(100)),
+ (trailing_zeros.as_str(), 10, 2, Some(100)),
+ ("0.000001", 10, 2, Some(0)),
+ ("1e-50", 10, 2, Some(0)),
+ ] {
+ assert_eq!(
+ parse_string_to_decimal(s, precision, scale, false,
false).unwrap(),
+ expected,
+ "{s} to DECIMAL({precision},{scale})"
+ );
+ }
+ // A value that is within the limit but does not fit the target type
is a different error.
+ for (s, precision, scale) in [
+ ("12345678901", 10, 2),
+ (nines.as_str(), 10, 2),
+ (one_and_37_zeros.as_str(), 10, 2),
+ ("1e30", 10, 2),
+ ] {
+ let err = parse_string_to_decimal(s, precision, scale, false,
false).unwrap_err();
+ assert!(
+ err.to_string().contains("NUMERIC_VALUE_OUT_OF_RANGE"),
+ "{s} to DECIMAL({precision},{scale}): {err}"
+ );
+ }
+
+ // An exponent or scale outside the int range is not a number to
`BigDecimal`, so it is
+ // an invalid input and not a value with too many digits.
+ for s in ["1e2147483648", "1e-2147483649", "1.5e-2147483648"] {
+ let err = parse_string_to_decimal(s, 10, 2, false,
false).unwrap_err();
+ assert!(err.to_string().contains("CAST_INVALID_INPUT"), "{s}:
{err}");
+ }
+ }
+
+ /// With `spark.sql.legacy.allowNegativeScaleOfDecimal`, Spark skips the
limit. A value that
+ /// has too many integral digits for the target type then fails the
precision check like any
+ /// other, and a zero with a large exponent is zero, whether or not Spark
has SPARK-60118.
+ #[test]
+ fn test_parse_string_to_decimal_skip_integral_digit_limit() {
+ let one_and_38_zeros = format!("1{}", "0".repeat(38));
+ for (s, precision, scale) in [
+ (one_and_38_zeros.as_str(), 10, 2),
+ (one_and_38_zeros.as_str(), 38, 0),
+ ] {
+ let err = parse_string_to_decimal(s, precision, scale, false,
true).unwrap_err();
+ assert!(
+ err.to_string().contains("NUMERIC_VALUE_OUT_OF_RANGE"),
+ "{s} to DECIMAL({precision},{scale}): {err}"
+ );
+ }
+ for s in ["0e38", "0E73", "+0e83", "0e5887677", "0.0e2147483647"] {
+ for zero_any_exponent in [false, true] {
+ assert_eq!(
+ parse_string_to_decimal(s, 10, 2, zero_any_exponent,
true).unwrap(),
+ Some(0),
+ "{s} zero_any_exponent={zero_any_exponent}"
+ );
+ }
+ }
+ // Two shapes that are too large for the target type but still do not
report
+ // NUMERIC_VALUE_OUT_OF_RANGE as Spark does: a scale adjustment over
38 returns `None`,
+ // which the cast reports as CAST_INVALID_INPUT under ANSI (#7576),
and more digits than
+ // an i128 holds are rejected as an invalid input (#7581).
+ assert_eq!(
+ parse_string_to_decimal("6E+38", 10, 2, false, true).unwrap(),
+ None
+ );
+ let forty_digits = format!("1{}", "0".repeat(39));
+ let err = parse_string_to_decimal(&forty_digits, 10, 2, false,
true).unwrap_err();
+ assert!(err.to_string().contains("CAST_INVALID_INPUT"), "{err}");
+ // The scale adjustment of "6E+38" to a negative-scale target is 8, so
it is a value.
+ assert_eq!(
+ parse_string_to_decimal("6E+38", 10, -30, false, true).unwrap(),
+ Some(600_000_000)
+ );
+ // The setting changes nothing for a value within the limit.
+ for s in ["123", "-1.005", "1e-50", "12345678901", "1e30"] {
+ for zero_any_exponent in [false, true] {
+ let with_setting = parse_string_to_decimal(s, 10, 2,
zero_any_exponent, true);
+ let without = parse_string_to_decimal(s, 10, 2,
zero_any_exponent, false);
+ assert_eq!(
+ with_setting.map_err(|e| e.to_string()),
+ without.map_err(|e| e.to_string()),
+ "{s}"
+ );
+ }
+ }
+ }
+
+ /// `exceeds_integral_digit_limit` settles most strings from an upper
bound on their integral
+ /// digits and counts the digits only for the rest. The bound must never
hide a string that is
+ /// over the limit, so compare the function 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 it and near the ends of the int range.
+ #[test]
+ fn test_integral_digit_limit_bound_agrees_with_the_digit_count() {
+ // The integral digits are `precision - scale` of the parsed
`BigDecimal`.
+ let reference = |integral: &str, fractional: &str, scale: i32,
zero_any_exponent: bool| {
Review Comment:
The `reference` closure repeats the formula of
`counted_digits_exceed_integral_digit_limit` (`1i32.wrapping_sub(scale)` for
zero, `digits as i64 - scale` otherwise), so a mistake in that formula would
pass in both. Since the goal is to check the shortcut against the count, could
the test call `counted_digits_exceed_integral_digit_limit` directly and drop
the closure?
--
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]
