andygrove opened a new issue, #6623:
URL: https://github.com/apache/datafusion-comet/issues/6623
Part of #6615.
## What / Why
This issue moves cast coverage for **string sources** to Comet SQL tests
under `sql-tests/expressions/cast/`. The sources are:
- the `cast StringType to ...` tests in `CometNativeCastSuite`, including
whitespace-trim parity and string to timestamp/date/NTZ;
- all of `CometDateTimeUtilsSuite`, which despite its name is eight
answer-only string-to-timestamp/date/NTZ cast tests;
- `CometExpressionSuite`'s string-to-boolean test.
Numeric and temporal/complex sources have their own issues. Static analysis
on `main` at 3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometNativeCastSuite` | 0 | 38 | 1 | 0 | 0 | 39 |
| `CometDateTimeUtilsSuite` | 0 | 8 | 0 | 0 | 0 | 8 |
| `CometExpressionSuite` | 0 | 1 | 0 | 0 | 0 | 1 |
| **Total** | **0** | **47** | **1** | **0** | **0** | **48** |
Nothing is covered today, and the existing fixtures are weaker than they
look:
- **Literal casts never run native code.** `CometCast.convert` evaluates a
cast of a `Literal` on the JVM at planning time, so these never touch the
native string parser:
- every string-to-timestamp format check in `cast.sql:43-99`;
- the literal block of `cast_timestamp_ntz.sql:69-116`;
- all of `cast_timestamp_ntz_ansi.sql`. Its `expect_error` queries pass on
Spark's own planning-time error.
New fixtures must cast **columns read from Parquet**.
- **Two fixtures defer to the Scala tests this issue deletes.**
`cast_string_to_date_ansi.sql:22-23` defers per-string parity, and
`cast_string_trim.sql:25-26` defers per-codepoint, all-mode parity. Their
coverage has to be added before deletion.
Follow the conventions in the numeric-cast issue for this work:
- a non-ANSI file with `cast` + `try_cast`, plus an ANSI companion;
- a plain-`query` sentinel per type pair in each ANSI file;
- one failing value per `expect_error` query, with a value-specific
substring (for example `[CAST_INVALID_INPUT] The value 'tru' of the type
"STRING" cannot be cast to "BOOLEAN"`);
- ANSI outcomes pinned by running Spark.
Deleting the per-pair Scala tests is blocked on replacing the cast-pair
coverage meta-test (L132), as described in that issue.
## Work
Line numbers refer to `CometNativeCastSuite` unless marked.
- `cast_string_to_boolean.sql` + `_ansi.sql` (L1027)
- `CometExpressionSuite` L2497's 31 spellings go in too, as a `CHAR(20)`
column: the true/false aliases, the Arrow-only spellings, and invalid values.
- `cast_string_trim.sql` (existing) + `cast_string_trim_ansi.sql` (new)
(L1055-L1082)
- Extend the 8 pads to the 45 in `trimPadding`: chr(0)..chr(32), chr(127),
and the non-ASCII ones via `cast(X'..' AS string)` (U+0085, U+00A0, U+1680,
U+2000, U+2005, U+200A, U+2028, U+2029, U+202F, U+205F, U+3000).
- Add leading-only, trailing-only and interior forms for every target,
plus `try_cast`.
- In the ANSI file, add a separate `expect_error` for each of the 10
"trims to nothing" values (L1082).
- `cast_string_to_integral.sql` + `_ansi.sql` (L1127-L1148): all 30
`castStringToIntegralInputs`, verbatim, plus numeric-pattern stand-ins.
- `cast_string_to_float.sql` + `_ansi.sql` (L1155, L1160, L1190, L1196):
include the 24 `specialValues` verbatim.
- `cast_string_to_decimal.sql` + `_ansi.sql` (L1203-L1300)
- `'d11307' || chr(10)`, the 4-space-prefixed ints, zero-mantissa
exponents, NUL bytes and fullwidth digits.
- The 23 basic values, and the scientific-notation set cast to
decimal(23,8).
- `cast_string_to_binary.sql` (L1318).
- `cast_string_to_date.sql` (new) + `cast_string_to_date_ansi.sql`
(existing) (L1322, `CometDateTimeUtilsSuite` L266)
- All 23 valid and 38 invalid dates, plus the invalid calendar dates.
- Don't insert 7-digit years, or 6-digit years above 262142; both are
outside Comet's supported range.
- `cast_string_to_timestamp.sql` + `_ansi.sql`
- Covers L1419-L1734 and `CometDateTimeUtilsSuite` L84, L118, L178 and
L277.
- **Data:**
- the 16 basic shapes;
- the 42 offset strings;
- the 23 invalid strings (non-ANSI only);
- `'2019-10-31T10:59:23Z:::'`;
- the segment-rule, whitespace-before-zone-suffix and
numeric-offset-validation sets;
- the out-of-range years, for both TIMESTAMP and TIMESTAMP_NTZ.
- **Zones:** a ConfigMatrix over UTC, America/Los_Angeles and
Asia/Kathmandu. The zone-suffix tests (L1716, L1734) need UTC plus
America/Los_Angeles, so use a separate file if the matrices differ.
- **Assertions:** use a plain `query`. String to timestamp is Compatible,
so that is stronger than the Scala answer-only checks.
- `cast_string_to_timestamp_ansi_spark35.sql` (`MaxSparkVersion: 3.5`) and
`_spark40.sql` (`MinSparkVersion: 4.0`): the ANSI halves of L1521 and L1566.
Leading-whitespace `T` forms are valid on 3.x and invalid on 4.0+.
- `cast_string_to_timestamp_range.sql` (`CometDateTimeUtilsSuite` L212):
UTC, non-ANSI, the extreme-year and int-overflow strings, and `CAST(.. AS
BIGINT)` for the two Long-boundary strings.
- `cast_string_to_timestamp_dst.sql` (`CometDateTimeUtilsSuite` L284):
New_York, an ANSI false/true matrix, and the six DST strings.
- `cast_timestamp_ntz.sql` + `cast_timestamp_ntz_ansi.sql` (existing)
(L1587, `CometDateTimeUtilsSuite` L310)
- Add the 19 values to `test_str_to_ntz`, `'2021-11-22'` and
`'2021-11-22T10:54:27.123456'`, plus `try_cast`.
- Widen the zones from 4 to the 19 in `representativeTimezones`, including
Europe/London with NTZ rows at its gap and overlap.
- Replace the literal-only ANSI queries with column queries.
**ANSI coverage the SQL suite would otherwise drop.** `CometTestBase` does
not set ANSI, so the four `CometDateTimeUtilsSuite` tests that leave it unset
(L84, L118, L284, L310) ran with ANSI on under Spark 4.x. The SQL suite forces
it off, so their conversions need an ANSI run of the valid values.
**Blocked**
- **#6617: L1399** ("(ansi) cast error message should include SQL query",
#2215).
- It uses `checkSparkError`, which checks native plan, error-class,
SQLSTATE and exception-class parity, plus that the message contains the SQL
text.
- Interim: only the message part fits today, as `expect_error(...)` with a
sentinel.
## Notes on the Scala tests
- **L1203** is a strict subset of L1208.
- **SPARK-37326 (`CometDateTimeUtilsSuite` L310)** has a wrong comment: "Has
offset — null". Spark's Cast uses `stringToTimestampWithoutTimeZone(s,
allowTimeZone = true)` and keeps the local time.
## Done when
- Every test in the table below is accounted for. Either a fixture covers it
(the same inputs, configs and assertion strength, or stronger), the fixture
that already covers it is cited, or it stays in Scala for the reason given
above.
- Each converted Scala test is deleted in the PR that adds its coverage. A
suite left empty is deleted and removed from the suite lists in
`.github/workflows/pr_build_linux.yml` and `pr_build_macos.yml`.
- The new and changed fixtures pass on every Spark profile. Apply
`run-all-spark-profiles`, because pull requests run only the default profile.
<details><summary>Every Scala test in scope, with its verdict and target
fixture</summary>
| Suite | Line | Test | Verdict | Target |
|---|---|---|---|---|
| `CometDateTimeUtilsSuite` | 84 | string to timestamp - basic date and
datetime formats | convert | cast/cast_string_to_timestamp.sql (new);
cast/cast_string_to_timestamp_ansi.sql (new) |
| `CometDateTimeUtilsSuite` | 118 | string to timestamp - embedded timezone
offsets | convert | cast/cast_string_to_timestamp.sql (new);
cast/cast_string_to_timestamp_ansi.sql (new) |
| `CometDateTimeUtilsSuite` | 178 | string to timestamp - invalid formats
return null | convert | cast/cast_string_to_timestamp.sql (new) |
| `CometDateTimeUtilsSuite` | 212 | SPARK-35780: full range of timestamp
string | convert | cast/cast_string_to_timestamp_range.sql (new) |
| `CometDateTimeUtilsSuite` | 266 | SPARK-15379: invalid calendar dates in
string to date cast | convert | cast/cast_string_to_date.sql (new);
cast/cast_string_to_timestamp.sql (new) |
| `CometDateTimeUtilsSuite` | 277 | trailing characters while converting
string to timestamp | convert | cast/cast_string_to_timestamp.sql (new) |
| `CometDateTimeUtilsSuite` | 284 | DST spring-forward gap and fall-back
overlap | convert | cast/cast_string_to_timestamp_dst.sql (new) |
| `CometDateTimeUtilsSuite` | 310 | SPARK-37326: cast string to
TIMESTAMP_NTZ rejects timezone offsets | convert | cast/cast_timestamp_ntz.sql;
cast/cast_timestamp_ntz_ansi.sql |
| `CometExpressionSuite` | 2497 | test cast utf8 to boolean as compatible
with Spark | convert | cast/cast.sql |
| `CometNativeCastSuite` | 1027 | cast StringType to BooleanType | convert |
cast/cast_string_to_boolean.sql (new); cast/cast_string_to_boolean_ansi.sql
(new) |
| `CometNativeCastSuite` | 1055 | cast StringType to BooleanType -
whitespace trim parity | convert | cast/cast_string_trim.sql;
cast/cast_string_trim_ansi.sql (new) |
| `CometNativeCastSuite` | 1059 | cast StringType to integral types -
whitespace trim parity | convert | cast/cast_string_trim.sql;
cast/cast_string_trim_ansi.sql (new) |
| `CometNativeCastSuite` | 1065 | cast StringType to floating point and
decimal types - whitespace trim parity | convert | cast/cast_string_trim.sql;
cast/cast_string_trim_ansi.sql (new) |
| `CometNativeCastSuite` | 1071 | cast StringType to DateType - whitespace
trim parity | convert | cast/cast_string_trim.sql;
cast/cast_string_trim_ansi.sql (new) |
| `CometNativeCastSuite` | 1075 | cast StringType to timestamp types -
whitespace trim parity | convert | cast/cast_string_trim.sql;
cast/cast_string_trim_ansi.sql (new) |
| `CometNativeCastSuite` | 1082 | cast StringType to timestamp types - ANSI
rejects a value that trims to nothing | convert |
cast/cast_string_trim_ansi.sql (new); cast/cast_string_trim.sql |
| `CometNativeCastSuite` | 1127 | cast StringType to ByteType | convert |
cast/cast_string_to_integral.sql (new); cast/cast_string_to_integral_ansi.sql
(new) |
| `CometNativeCastSuite` | 1134 | cast StringType to ShortType | convert |
cast/cast_string_to_integral.sql (new); cast/cast_string_to_integral_ansi.sql
(new) |
| `CometNativeCastSuite` | 1141 | cast StringType to IntegerType | convert |
cast/cast_string_to_integral.sql (new); cast/cast_string_to_integral_ansi.sql
(new) |
| `CometNativeCastSuite` | 1148 | cast StringType to LongType | convert |
cast/cast_string_to_integral.sql (new); cast/cast_string_to_integral_ansi.sql
(new) |
| `CometNativeCastSuite` | 1155 | cast StringType to DoubleType | convert |
cast/cast_string_to_float.sql (new); cast/cast_string_to_float_ansi.sql (new) |
| `CometNativeCastSuite` | 1160 | cast StringType to FloatType | convert |
cast/cast_string_to_float.sql (new); cast/cast_string_to_float_ansi.sql (new) |
| `CometNativeCastSuite` | 1190 | cast StringType to FloatType special
values | convert | cast/cast_string_to_float.sql (new);
cast/cast_string_to_float_ansi.sql (new) |
| `CometNativeCastSuite` | 1196 | cast StringType to DoubleType special
values | convert | cast/cast_string_to_float.sql (new);
cast/cast_string_to_float_ansi.sql (new) |
| `CometNativeCastSuite` | 1203 | cast StringType to DecimalType(10,2) |
convert | cast/cast_string_to_decimal.sql (new) |
| `CometNativeCastSuite` | 1208 | cast StringType to DecimalType(10,2) fuzz
| convert | cast/cast_string_to_decimal.sql (new);
cast/cast_string_to_decimal_ansi.sql (new) |
| `CometNativeCastSuite` | 1214 | cast StringType to DecimalType(2,2) |
convert | cast/cast_string_to_decimal.sql (new);
cast/cast_string_to_decimal_ansi.sql (new) |
| `CometNativeCastSuite` | 1220 | cast StringType to DecimalType check if
right exception message is thrown | convert | cast/cast_string_to_decimal.sql
(new); cast/cast_string_to_decimal_ansi.sql (new) |
| `CometNativeCastSuite` | 1226 | cast StringType to DecimalType(2,2) check
if right exception is being thrown | convert | cast/cast_string_to_decimal.sql
(new); cast/cast_string_to_decimal_ansi.sql (new) |
| `CometNativeCastSuite` | 1232 | cast StringType to DecimalType(38,10) high
precision - check 0 mantissa | convert | cast/cast_string_to_decimal.sql (new);
cast/cast_string_to_decimal_ansi.sql (new) |
| `CometNativeCastSuite` | 1238 | cast StringType to DecimalType(38,10) high
precision | convert | cast/cast_string_to_decimal.sql (new);
cast/cast_string_to_decimal_ansi.sql (new) |
| `CometNativeCastSuite` | 1244 | cast StringType to DecimalType - null
bytes and fullwidth digits | convert | cast/cast_string_to_decimal.sql (new);
cast/cast_string_to_decimal_ansi.sql (new) |
| `CometNativeCastSuite` | 1270 | cast StringType to DecimalType(10,2) basic
values | convert | cast/cast_string_to_decimal.sql (new);
cast/cast_string_to_decimal_ansi.sql (new) |
| `CometNativeCastSuite` | 1300 | cast StringType to Decimal type scientific
notation | convert | cast/cast_string_to_decimal.sql (new);
cast/cast_string_to_decimal_ansi.sql (new) |
| `CometNativeCastSuite` | 1318 | cast StringType to BinaryType | convert |
cast/cast_string_to_binary.sql (new) |
| `CometNativeCastSuite` | 1322 | cast StringType to DateType | convert |
cast/cast_string_to_date.sql (new); cast/cast_string_to_date_ansi.sql |
| `CometNativeCastSuite` | 1399 | (ansi) cast error message should include
SQL query | blocked: #6617 | cast/cast_string_to_date_ansi.sql |
| `CometNativeCastSuite` | 1419 | cast StringType to TimestampType - UTC |
convert | cast/cast_string_to_timestamp.sql (new);
cast/cast_string_to_timestamp_ansi.sql (new) |
| `CometNativeCastSuite` | 1456 | cast StringType to TimestampType | convert
| cast/cast_string_to_timestamp.sql (new);
cast/cast_string_to_timestamp_ansi.sql (new) |
| `CometNativeCastSuite` | 1464 | cast StringType to TimestampType - subset
of supported values | convert | cast/cast_string_to_timestamp.sql (new);
cast/cast_string_to_timestamp_ansi.sql (new) |
| `CometNativeCastSuite` | 1521 | cast StringType to TimestampType -
T-hour-only whitespace handling | convert | cast/cast_string_to_timestamp.sql
(new); cast/cast_string_to_timestamp_ansi_spark35.sql (new, -- MaxSparkVersion:
3.5); cast/cast_string_to_timestamp_ansi_spark40.sql (new, -- MinSparkVersion:
4.0) |
| `CometNativeCastSuite` | 1566 | cast StringType to TimestampType -
time-only offset leading whitespace | convert |
cast/cast_string_to_timestamp.sql (new);
cast/cast_string_to_timestamp_ansi_spark35.sql (new, -- MaxSparkVersion: 3.5);
cast/cast_string_to_timestamp_ansi_spark40.sql (new, -- MinSparkVersion: 4.0) |
| `CometNativeCastSuite` | 1578 | cast StringType to $toType - out-of-range
years (loop: TimestampType, TimestampNTZType) | convert |
cast/cast_string_to_timestamp.sql (new); cast/cast_string_to_timestamp_ansi.sql
(new) |
| `CometNativeCastSuite` | 1587 | cast StringType to TimestampNTZType |
convert | cast/cast_timestamp_ntz.sql; cast/cast_timestamp_ntz_ansi.sql |
| `CometNativeCastSuite` | 1690 | cast StringType to TimestampType - Spark
segment rules | convert | cast/cast_string_to_timestamp.sql (new);
cast/cast_string_to_timestamp_ansi.sql (new) |
| `CometNativeCastSuite` | 1704 | cast StringType to TimestampNTZType -
Spark segment rules | convert | cast/cast_string_to_timestamp.sql (new);
cast/cast_string_to_timestamp_ansi.sql (new) |
| `CometNativeCastSuite` | 1716 | cast StringType to
TimestampType/TimestampNTZType - whitespace before zone suffix | convert |
cast/cast_string_to_timestamp.sql (new); cast/cast_string_to_timestamp_ansi.sql
(new) |
| `CometNativeCastSuite` | 1734 | cast StringType to
TimestampType/TimestampNTZType - numeric offset validation | convert |
cast/cast_string_to_timestamp.sql (new); cast/cast_string_to_timestamp_ansi.sql
(new) |
</details>
--
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]
