andygrove opened a new issue, #6625:
URL: https://github.com/apache/datafusion-comet/issues/6625
Part of #6615.
## What / Why
This issue moves datetime expression coverage to Comet SQL tests under
`sql-tests/expressions/datetime/`. The sources are
`CometTemporalExpressionSuite` and the datetime tests in `CometExpressionSuite`
(`date_add`/`date_sub`, `hour`/`minute`/`second`, `trunc`/`date_trunc`, INT96
timestamps, `from_unixtime`, day-of-week, date partition columns).
String-to-timestamp/date casts, including all of `CometDateTimeUtilsSuite`, are
in the string-cast issue.
Static analysis on `main` at 3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometTemporalExpressionSuite` | 5 | 24 | 1 | 0 | 7 | 37 |
| `CometExpressionSuite` | 1 | 18 | 0 | 0 | 1 | 20 |
| **Total** | **6** | **42** | **1** | **0** | **8** | **57** |
The Scala tests pin far more than the fixtures do, which is why few are
covered:
- `date_trunc`: 15 formats, pinned in UTC and in non-UTC zones on the
native, dispatch and fallback routes. Fixtures have at most 6 formats, mostly
under allowIncompatible.
- `date_format`: 23 native formats and the non-UTC routes. Fixtures have 3
native formats.
- `unix_timestamp` is tested across zones and on NTZ and date input; the
fixtures are UTC-only.
- `hour`/`minute`/`second` on NTZ are tested across four zones.
Closed PR #4765 migrated four of these tests and concluded that the
fuzz-based tests, `Days`/`Hours` and the operator-only allowIncompatible test
could not move. Only the `Days`/`Hours` part holds up.
- **Fuzz tests.** They reduce to curated values. `FuzzDataGenerator`'s
default base date resolves to about 3333-01-01, so fuzz timestamps are winter
dates around the 3332/3333 boundary, written as INT96 and never NULL. Use
far-future winter values to mirror them; summer far-future values in DST zones
would test behaviour the Scala tests never covered.
- **The operator-only test (L737).** It converts with rows at 12:00 UTC, so
`expect_native(date_format)` can compare answers too.
## Work
Line numbers refer to `CometTemporalExpressionSuite` unless marked
`CometExpressionSuite`.
**New fixtures**
- `trunc_timestamp_formats.sql` (L145, L170)
- UTC.
- Data: far-future INT96 timestamps with millisecond fractions, a
pre-epoch value and NULL.
- Add a format column with valid, mixed-case, invalid and NULL values.
- `expect_native(date_trunc)` for each of the 15 formats, and
`expect_dispatch(date_trunc)` for `'invalid'` and for the format column.
- `trunc_timestamp_dispatch.sql` (L200): matrix over Los_Angeles, London and
Tokyo, with 15 `expect_dispatch(date_trunc)` queries.
- `trunc_timestamp_non_utc_fallback.sql` (L222)
- New_York and London with the dispatcher off.
- Assert `expect_fallback(date_trunc:
spark.comet.exec.scalaUDF.codegen.enabled=false)`.
- `trunc_timestamp_non_utc_native.sql` (L239)
- Set `TruncTimestamp.allowIncompatible=true` and turn the dispatcher off.
- Matrix over New_York, London and Tokyo, with data from 2024-05-21 to
2024-07-10.
- Assert `expect_native(date_trunc)`.
- `trunc_timestamp_ntz.sql` (L317, `CometExpressionSuite` L1150)
- Matrix over four zones, with NTZ values in summer 2024 and all 15
formats.
- No fixture runs `date_trunc` on NTZ today.
- `unix_timestamp_timezones.sql` (L264, L273, L285)
- Timestamp, date and NTZ columns with far-future, DST-date, pre-epoch and
NULL values.
- Matrix over four zones, with `expect_native(unix_timestamp)`.
- `unix_timestamp_collation.sql` (L457, L481, `MinSparkVersion: 4.0`)
- Use an allowIncompatible matrix.
- Assert `expect_dispatch(unix_timestamp)` for string input with a
collated format or subject.
- Assert `expect_native(unix_timestamp)` for date, timestamp and NTZ input.
- `hour_minute_second_ntz.sql` (L301): matrix over four zones, with
`expect_native(hour, minute, second)` on NTZ.
- `date_format_native_formats.sql` (L603, L620, L650)
- UTC.
- Every key in `CometDateFormat.supportedFormats` (23) as
`expect_native(date_format)`, over far-future, AM/PM-edge, pre-epoch and NULL
values.
- Add five literal `TIMESTAMP '...'` queries.
- `date_format_non_utc_fallback.sql` (L721).
- `date_format_non_utc_native.sql` (L737)
- Set `DateFormatClass.allowIncompatible=true` and turn the dispatcher off.
- Matrix over New_York, London and Tokyo, with rows at 12:00 UTC.
- `date_format_ntz.sql` (L349): matrix over four zones and 22 formats.
- `timestamp_ntz_cross_timezone.sql` (L371)
- Use a read-zone matrix: London, New_York, UTC and Auckland.
- NTZ writes don't depend on the zone, so the Scala test's separate write
zone adds nothing.
- `date_add_overflow.sql` (`CometExpressionSuite` L722): `query
ignore(https://github.com/apache/datafusion-comet/issues/2539)` over `d +
2147483647`.
- `date_sub_overflow.sql` (`MaxSparkVersion: 3.5`) and
`date_sub_overflow_spark4.sql` (`MinSparkVersion: 4.0`) (L769)
- `d - 2147483647`.
- Assert `expect_error(integer overflow)` on Spark 3.x and
`expect_error(EXPRESSION_DECODING_FAILED)` on Spark 4.
- `int96_timestamp_conversion.sql` (L983, L1213)
- Set `spark.sql.parquet.int96TimestampConversion=true`.
- That conf turns off the whole Comet extension, so this leg compares
Spark with Spark. Use `spark_answer_only`, or drop this leg.
- The conversion=false legs are covered once the trunc fixtures have all
15 formats, because INT96 is Spark's default timestamp output type.
- `date_timestamp_literals.sql` (L693)
- `WHERE d > DATE'2020-01-01' AND ts < TIMESTAMP'2020-01-01 00:00:00'`,
over data on both sides of the boundary.
- The Scala data never passes this filter.
- `dayofweek_weekday.sql` (L2308)
- Set `spark.sql.parquet.datetimeRebaseModeInWrite=CORRECTED`.
- A DATE column covering the epoch week, leap days, 1900-01-01, 0001-01-01
and 9999-12-31.
- `date_partition_column.sql` (L2279): a `PARTITIONED BY (cal_dt)` table,
`CAST(cal_dt AS STRING)`, and `SHOW PARTITIONS` as `spark_answer_only`.
**Existing fixtures to extend**
- `trunc_date.sql` (L82, `CometExpressionSuite` L1080)
- Add the `yyyy`, `yy`, `mon`, `mm` and `week` formats, plus the
upper-case aliases `YEAR`, `YYYY`, `YY`, `QUARTER`, `MON`, `MONTH`, `MM` and
`WEEK`, as `expect_native(trunc)`.
- Add dates across the 3332/3333 boundary.
- Add `expect_dispatch(trunc)` for `'invalid'` and for a format column.
- `trunc_date_enabled.sql` (L1094): the same aliases as values in the format
column.
- `trunc_timestamp.sql` (L1113)
- Add the 11 missing formats, over pre-epoch fractional values and one
microsecond-precision value.
- Pin the session time zone. Today the file sets
`TruncTimestamp.allowIncompatible=true` without one, so on a non-UTC machine it
runs the Incompatible native path in that machine's zone.
- `trunc_timestamp_enabled.sql` (L1187): all 15 formats as format-column
values, plus an NTZ column and pre-epoch values.
- `unix_timestamp_strings.sql` (L423): add `ConfigMatrix:
spark.comet.expression.UnixTimestamp.allowIncompatible=false,true`.
- `date_format.sql` (L703)
- Replace the Los_Angeles `Config` with a matrix over New_York,
Los_Angeles, London and Tokyo.
- Switch the queries to `expect_dispatch(date_format)`.
- `unix_date.sql` (L758)
- Change `spark_answer_only` to `query`, since `CometUnixDate` is a native
cast.
- Add 1960-01-01, 1970-01-02 and dates near 3333.
- `date_add.sql` / `date_sub.sql` (L707, L741, L755, L789): add tinyint and
smallint day counts, using `2Y`/`2S` literals and tinyint/smallint columns.
- `hour.sql`, `minute.sql`, `second.sql` (L934)
- Add pre-epoch timestamps with fractional seconds.
- Fix the stale comments saying TimestampNTZ is Incompatible and
dispatched (#3180). `CometHour`, `CometMinute` and `CometSecond` have no NTZ
check, and the native kernel extracts NTZ fields directly.
- `from_unix_time_enabled.sql` (L2334)
- Change the default-format queries from `spark_answer_only` to `query`.
- Add a string-input column and seconds in -128..127.
- Add an Asia/Kathmandu variant.
**Blocked**
- **#6617: L51** (`next_day` / `make_date` ANSI exception parity).
- The test checks exception class, error class, SQLSTATE, message
parameters and exact message, and that the error is not a
`CometNativeException`.
- Interim: `make_date(1000000000, 1, 1)` and `make_date(1000000000, 13,
0)` as `expect_error`.
## Stays in Scala
- **L795, L805, L819, L885, L899.** `Days` / `Hours` are `Unevaluable`
partition transforms built with `getColumnFromExpression`. SQL cannot call them.
- **L111** unit-tests `CometTimeZone.nativeId`. Eight of its IDs are covered
end to end by `session_timezone_ids.sql`; EST, GMT and +00:00 could be added
there.
- **L138** checks the native tzdata version and a warning-text helper.
- **`CometExpressionSuite` L960** asserts that Spark constant-folds
`hour`/`minute`/`second`, which is optimizer plan shape.
## Fixture problems to fix along the way
- **NULL literal arguments are folded by `NullPropagation`** and never reach
Comet: `last_day.sql:29`, `date_diff.sql:37`, `unix_date.sql:29`,
`date_format_time_parser_policy.sql:48-49`, `next_day.sql:73-74` (the whole
query) and `make_date.sql:57-64`. Read NULLs from a column instead.
- **`date_format_time_parser_policy.sql` uses `spark_answer_only`** although
the plan is native (UTC with whitelisted formats).
- **The leap-year `datediff` literals** look like calendar tests, but
`CometCast` parses the `DATE('...')` literals on the JVM. Native code only
subtracts two integers. Use date columns.
## 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 |
|---|---|---|---|---|
| `CometExpressionSuite` | 693 | date and timestamp type literals | convert
| datetime/date_timestamp_literals.sql (new) |
| `CometExpressionSuite` | 707 | date_add with int scalars | convert |
datetime/date_add.sql |
| `CometExpressionSuite` | 722 | date_add with scalar overflow (ignored,
TODO #2539) | convert | datetime/date_add_overflow.sql (new) |
| `CometExpressionSuite` | 741 | date_add with int arrays | convert |
datetime/date_add.sql |
| `CometExpressionSuite` | 755 | date_sub with int scalars | convert |
datetime/date_sub.sql |
| `CometExpressionSuite` | 769 | date_sub with scalar overflow | convert |
datetime/date_sub_overflow.sql (new, MaxSparkVersion: 3.5) +
datetime/date_sub_overflow_spark4.sql (new, MinSparkVersion: 4.0) |
| `CometExpressionSuite` | 789 | date_sub with int arrays | convert |
datetime/date_sub.sql |
| `CometExpressionSuite` | 934 | hour, minute, second | convert |
datetime/hour.sql + datetime/minute.sql + datetime/second.sql |
| `CometExpressionSuite` | 960 | time expressions folded on jvm | keep | |
| `CometExpressionSuite` | 983 | hour on int96 timestamp column | convert |
datetime/int96_timestamp_conversion.sql (new) |
| `CometExpressionSuite` | 1080 | trunc | convert | datetime/trunc_date.sql |
| `CometExpressionSuite` | 1094 | trunc with format array | convert |
datetime/trunc_date_enabled.sql |
| `CometExpressionSuite` | 1113 | date_trunc | convert |
datetime/trunc_timestamp.sql |
| `CometExpressionSuite` | 1150 | date_trunc with timestamp_ntz | convert |
datetime/trunc_timestamp.sql |
| `CometExpressionSuite` | 1187 | date_trunc with format array | convert |
datetime/trunc_timestamp_enabled.sql |
| `CometExpressionSuite` | 1213 | date_trunc on int96 timestamp column |
convert | datetime/int96_timestamp_conversion.sql (new) +
datetime/trunc_timestamp.sql |
| `CometExpressionSuite` | 2279 | date partition column does not forget date
type | convert | datetime/date_partition_column.sql (new) |
| `CometExpressionSuite` | 2293 | DatePart functions:
Year/Month/DayOfMonth/DayOfWeek/DayOfYear/WeekOfYear/Quarter | covered |
datetime/datetime.sql |
| `CometExpressionSuite` | 2308 | dayofweek/weekday over a date column,
including ancient dates | convert | datetime/dayofweek_weekday.sql (new) |
| `CometExpressionSuite` | 2334 | from_unixtime | convert |
datetime/from_unix_time_enabled.sql + a new Asia/Kathmandu variant |
| `CometTemporalExpressionSuite` | 51 | next_day and make_date ANSI errors
match Spark exceptions | blocked: #6617 | datetime/next_day_ansi.sql;
datetime/make_date_ansi.sql |
| `CometTemporalExpressionSuite` | 82 | trunc (TruncDate) | convert |
datetime/trunc_date.sql |
| `CometTemporalExpressionSuite` | 111 | session timezone IDs are normalized
for native code | keep | |
| `CometTemporalExpressionSuite` | 138 | native tzdata version is reported |
keep | |
| `CometTemporalExpressionSuite` | 145 | date_trunc (TruncTimestamp) -
reading from DataFrame | convert | datetime/trunc_timestamp_formats.sql (new) |
| `CometTemporalExpressionSuite` | 170 | date_trunc (TruncTimestamp) -
reading from Parquet | convert | datetime/trunc_timestamp_formats.sql (new) |
| `CometTemporalExpressionSuite` | 200 | date_trunc - non-UTC timezone
routes via codegen dispatcher | convert | datetime/trunc_timestamp_dispatch.sql
(new) |
| `CometTemporalExpressionSuite` | 222 | date_trunc - non-UTC timezone falls
back when codegen dispatcher disabled | convert |
datetime/trunc_timestamp_non_utc_fallback.sql (new) |
| `CometTemporalExpressionSuite` | 239 | date_trunc - non-UTC timezone takes
native path when allowIncompatible is enabled | convert |
datetime/trunc_timestamp_non_utc_native.sql (new) |
| `CometTemporalExpressionSuite` | 264 | unix_timestamp - timestamp input |
convert | datetime/unix_timestamp_timezones.sql (new) |
| `CometTemporalExpressionSuite` | 273 | unix_timestamp - date input |
convert | datetime/unix_timestamp_timezones.sql (new) |
| `CometTemporalExpressionSuite` | 285 | unix_timestamp - timestamp_ntz
input | convert | datetime/unix_timestamp_timezones.sql (new) |
| `CometTemporalExpressionSuite` | 301 | hour/minute/second - timestamp_ntz
input | convert | datetime/hour_minute_second_ntz.sql (new) |
| `CometTemporalExpressionSuite` | 317 | date_trunc - timestamp_ntz input |
convert | datetime/trunc_timestamp_ntz.sql (new) |
| `CometTemporalExpressionSuite` | 349 | date_format - timestamp_ntz input |
convert | datetime/date_format_ntz.sql (new) |
| `CometTemporalExpressionSuite` | 371 | timestamp_ntz - cross-timezone
Parquet round-trip | convert | datetime/timestamp_ntz_cross_timezone.sql (new) |
| `CometTemporalExpressionSuite` | 423 | unix_timestamp - string input uses
codegen dispatch | convert | datetime/unix_timestamp_strings.sql |
| `CometTemporalExpressionSuite` | 457 | unix_timestamp - collated strings
use codegen even when native is opted into | convert |
datetime/unix_timestamp_collation.sql (new) |
| `CometTemporalExpressionSuite` | 481 | unix_timestamp - date and timestamp
inputs ignore collated formats and stay native | convert |
datetime/unix_timestamp_collation.sql (new) |
| `CometTemporalExpressionSuite` | 528 | last_day | covered |
datetime/last_day.sql |
| `CometTemporalExpressionSuite` | 554 | datediff | covered |
datetime/date_diff.sql |
| `CometTemporalExpressionSuite` | 603 | date_format with timestamp column |
convert | datetime/date_format_native_formats.sql (new) |
| `CometTemporalExpressionSuite` | 620 | date_format with specific format
strings | convert | datetime/date_format_native_formats.sql (new) |
| `CometTemporalExpressionSuite` | 650 | date_format with literal timestamp
| convert | datetime/date_format_native_formats.sql (new) |
| `CometTemporalExpressionSuite` | 669 | date_format with null | covered |
datetime/date_format_enabled.sql |
| `CometTemporalExpressionSuite` | 678 | date_format unsupported format
routes via codegen dispatcher | covered | datetime/routing_temporal_enabled.sql
|
| `CometTemporalExpressionSuite` | 691 | date_format unsupported format
falls back when codegen dispatcher disabled | covered |
datetime/routing_temporal_disabled.sql |
| `CometTemporalExpressionSuite` | 703 | date_format with non-UTC timezone
routes via codegen dispatcher | convert | datetime/date_format.sql |
| `CometTemporalExpressionSuite` | 721 | date_format with non-UTC timezone
falls back when codegen dispatcher disabled | convert |
datetime/date_format_non_utc_fallback.sql (new) |
| `CometTemporalExpressionSuite` | 737 | date_format with non-UTC timezone
takes native path when allowIncompatible is enabled | convert |
datetime/date_format_non_utc_native.sql (new) |
| `CometTemporalExpressionSuite` | 758 | unix_date | convert |
datetime/unix_date.sql |
| `CometTemporalExpressionSuite` | 795 | days - date input | keep | |
| `CometTemporalExpressionSuite` | 805 | days - timestamp input | keep | |
| `CometTemporalExpressionSuite` | 819 | days - literal edge cases | keep |
|
| `CometTemporalExpressionSuite` | 885 | hours - timestamp input | keep | |
| `CometTemporalExpressionSuite` | 899 | hours - timestamp_ntz input | keep
| |
| `CometTemporalExpressionSuite` | 924 | cast TimestampNTZ to Timestamp -
DST edge cases | convert | cast/cast_timestamp_ntz.sql |
</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]
