MaxGekk commented on PR #56690:
URL: https://github.com/apache/spark/pull/56690#issuecomment-4778202948
Nice work — the coverage here is clean and the golden diffs all reflect
correct behavior. Approving as-is.
A few **optional** parity gaps you might consider, either here or as a
follow-up. I checked these against the dedicated `time.sql` suite to avoid
overlap — these are shared suites where DATE/TIMESTAMP already have coverage
that TIME still lacks:
**`literals.sql`** — has DATE/TIMESTAMP typed literals (incl.
case-insensitive keyword and the unary +/- block) but no TIME:
```sql
select tImE '12:34:56'; -- case-insensitive keyword, like dAte /
tImEstAmp
select +time '12:34:56';
select -time '12:34:56'; -- parity with the "can't negate
date/timestamp" cases
```
The negation case is the meaningful one — it pins that `-TIME` is rejected.
**`predicate-functions.sql`** — full `>`/`>=`/`<`/`<=`/`BETWEEN` operator
set for DATE/TIMESTAMP, but no TIME (`postgreSQL/time.sql` only exercises
`<`/`>` inside WHERE):
```sql
select time '12:34:56' >= time '12:34:56';
select time '12:34:56' < '13:00:00'; --
string coercion
select time '12:34:56' between time '10:00:00' and time '14:00:00';
```
**`typeCoercion/native/dateTimeOperations.sql`** — this file's character is
a type × interval matrix that includes the *illegal* combinations, but the TIME
additions are all happy-path. Worth adding the negative cases:
```sql
select cast('12:34:56' as time) + interval 2 month; --
year-month interval -> reject
select interval '02:00:00' hour to second - cast('12:34:56' as time); --
interval - TIME
select cast('12:34:56' as time) + cast('2017-12-11' as date); --
TIME + DATE -> reject
```
Currently the only illegal-TIME-arithmetic assertion in the PR is `TIME +
TIME` in `postgreSQL/time.sql`; the systematic coercion failures belong here.
Lower priority: adding a TIME column to `extract.sql`'s shared view would
give `date_part`-on-TIME parity (the `extract` form is already in `time.sql`).
None of this blocks merge — the deferral of formatting parity to SPARK-54588
is well reasoned, and these could ride alongside it.
--
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]
