snmvaughan opened a new pull request, #7605:
URL: https://github.com/apache/datafusion-comet/pull/7605
## Which issue does this PR close?
Follow-up to #7578, for the review comments @comphead left on it after the
merge. There is no issue to close.
## Rationale for this change
The tests added in #7578 have a stale comment, a closure that copies the
code it checks, and several tests and SQL cases that assert the same strings.
Once #7576 and #7581 change the error for the shapes they track, every repeat
is one more place to edit.
## What changes are included in this PR?
Tests only. No behavior changes.
- `string.rs`:
- `test_parse_string_to_decimal_boundary` asserted `is_err()` for 39 nines
under a comment saying the digits overflow an i128. The string reaches the
integral-digit limit first, so the test now asserts
`NUMERIC_OUT_OF_SUPPORTED_RANGE` and the comment says so.
- `test_integral_digit_limit_bound_agrees_with_the_digit_count` takes the
expected value from `counted_digits_exceed_integral_digit_limit`. It had a
closure with a copy of that formula, which would pass a mistake in the formula.
- `test_cast_string_to_decimal_integral_digit_limit` keeps what only it
checks, the error to NULL mapping in each eval mode and the `Decimal256`
wiring, with one string for each error class. The parser tests and the SQL
files cover the other strings. The `6E+38` and 40-digit shapes that are pinned
to #7576 and #7581 are now pinned only in
`test_parse_string_to_decimal_skip_integral_digit_limit`.
- SQL file tests take the place of the two `castTest` cases in
`CometNativeCastSuite`:
- `cast_string_to_decimal_integral_digits_ansi.sql` gains the zero strings
that only the Scala cases had (`+0e83` and `0e5887677`). After the setting, it
also queries the zero strings with `try_cast` and a `DECIMAL(38,10)` target.
- The new `cast_string_to_decimal_integral_digits.sql` has the results
without ANSI mode: NULL for a string over the limit whatever the target type,
and the values and NULLs after `SET
spark.sql.legacy.allowNegativeScaleOfDecimal=true`. It is a second file, as
`cast_string_to_decimal_unicode_digits.sql` and its `_ansi` twin are, because
`-- Config` sets the mode of a whole file.
- The two `castTest` cases are removed. The test of the proto option
stays, because a SQL file cannot see the field that carries the setting.
## How are these changes tested?
- `cargo test -p datafusion-comet-spark-expr` passes (1,327 unit tests), and
`cargo clippy` and `cargo fmt --check` are clean.
- I could not run the SQL files, because the environment I used cannot start
a Spark session. I checked their queries another way. The 18 strings of the two
files went to four target types (`DECIMAL(10,2)`, `(38,10)`, `(38,0)` and
`(38,38)`), with and without the setting, in legacy, try and ANSI mode (432
cases), through Spark 4.1.3's `Decimal.fromString` and `fromStringANSI`
followed by `changePrecision`, and through Comet's `spark_cast`. The results
are identical except for the ANSI error under the setting for the shapes pinned
to #7576 and #7581 (`CAST_INVALID_INPUT` against `NUMERIC_VALUE_OUT_OF_RANGE`),
and no query in the files asks for those.
- Spotless is clean on `CometNativeCastSuite`. CI runs the SQL files.
#7522 adds an argument to `parse_string_to_decimal`, so whichever of the two
merges second has a one-line conflict in
`test_parse_string_to_decimal_boundary`.
--
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]
