snmvaughan commented on code in PR #7578:
URL: https://github.com/apache/datafusion-comet/pull/7578#discussion_r4541932157
##########
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:
You're right, the string reaches the limit first and `is_err()` hid it. The
follow-up asserts `NUMERIC_OUT_OF_SUPPORTED_RANGE` there and rewrites the
comment. It isn't open yet; I'll link it here when it is.
##########
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:
Yes, that was one reason: `castTest` compares the whole error message on 3.4
and the first 40 characters on 4.x, where `expect_error` matches the error
class in both messages. It also ran the legacy and try modes and checked that a
Comet operator ran, which `query` and `try_cast` do in the SQL files. Spark 3.4
is gone since #7521, and the class is what tells these cases apart, so the SQL
files are enough.
The follow-up moves what only the Scala cases had into the SQL files.
`+0e83` and `0e5887677`, and the zero strings under `try_cast` and a
`DECIMAL(38,10)` target, go into
`cast_string_to_decimal_integral_digits_ansi.sql`. A new
`cast_string_to_decimal_integral_digits.sql` has the results without ANSI mode,
with and without the legacy setting. I made it a second file, as
`cast_string_to_decimal_unicode_digits.sql` and its `_ansi` twin are, because
`-- Config` sets the mode for the whole file. The two `castTest` cases are
removed and the proto-option test is the only Scala test left.
##########
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:
Agreed. The follow-up keeps the `Err` to NULL mapping for each eval mode and
the `Decimal256` wiring, with one string for each error class: `6E+38` for the
limit, and the 39-digit string under the setting for
`NUMERIC_VALUE_OUT_OF_RANGE`. The `6E+38` and 40-digit shapes from #7576 and
#7581 are then pinned only in
`test_parse_string_to_decimal_skip_integral_digit_limit`, so they change in one
place when those 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:
Done in the follow-up: the test calls
`counted_digits_exceed_integral_digit_limit` and the closure is gone.
--
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]
