You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "vboo123 (via GitHub)" <gi...@apache.org> on 2026/06/23 04:01:48 UTC
[PR] [SPARK-57563][SQL] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
vboo123 opened a new pull request, #56690:
URL: https://github.com/apache/spark/pull/56690
### What changes were proposed in this pull request?
Add TIME data type coverage to the SQL golden-file test suites (`sql-tests`) where DATE and TIMESTAMP already have coverage but TIME did not. This brings TIME to parity with the other temporal types for these suites. No engine or logic changes are included. SQL inputs were added and the `results/` and `analyzer-results/` files were regenerated via `SQLQueryTestSuite` (generated `.out` files are not hand-edited).
Per-file changes:
- `try_cast.sql` — add `TRY_CAST(... AS TIME)`, valid and invalid cases (invalid returns NULL)
- `try_datetime_functions.sql` — add `try_to_time`, mirroring the existing `try_to_timestamp` cases
- `datetime-parsing.sql` / `datetime-parsing-invalid.sql` / `datetime-parsing-legacy.sql` — add `to_time` parsing including invalid inputs; the legacy file inherits the cases through its existing `--IMPORT` of `datetime-parsing.sql`
- `typeCoercion/native/dateTimeOperations.sql` — add TIME +/- day-time interval and TIME - TIME (which yields INTERVAL HOUR TO SECOND)
- `postgreSQL/time.sql` (new) — port analogous to the existing `postgreSQL/date.sql` and `postgreSQL/timestamp.sql`
TIME formatting parity in `datetime-formatting.sql` / `datetime-formatting-invalid.sql` / `datetime-formatting-legacy.sql` is intentionally left out of this PR. It depends on the `time_format` function (SPARK-54588), which has not landed yet, and `date_format` / `to_char` do not accept the TIME type, so there is currently no pattern-based way to format a TIME. It will be added as a follow-up once SPARK-54588 merges.
### Why are the changes needed?
The TIME type was added by SPIP SPARK-51162 (shipped in Spark 4.1.0) and works in the engine, but it lacked the shared golden-file coverage that DATE and TIMESTAMP have. Adding it locks in correctness for these suites so future changes that regress TIME casting, parsing, or arithmetic are caught automatically. This is a sub-task of the TIME umbrella SPARK-57550.
### Does this PR introduce _any_ user-facing change?
No. Test-only change.
### How was this patch tested?
Regenerated golden files with `SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite"`, then confirmed the suite passes in verify mode (without the regenerate flag) for all touched files: `try_cast.sql`, `try_datetime_functions.sql`, `datetime-parsing*.sql`, `typeCoercion/native/dateTimeOperations.sql`, and `postgreSQL/time.sql`. All passed.
### Was this patch authored or co-authored using generative AI tooling?
Yes. Generated using Kiro (Claude Opus 4.8)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL][TESTS] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #56690: [SPARK-57563][SQL][TESTS] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP
URL: https://github.com/apache/spark/pull/56690
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "vboo123 (via GitHub)" <gi...@apache.org>.
vboo123 commented on PR #56690:
URL: https://github.com/apache/spark/pull/56690#issuecomment-4775496640
If the community believes this should include TIME formatting parity in `datetime-formatting.sql` / `datetime-formatting-invalid.sql` / `datetime-formatting-legacy.sql`
I can also first complete https://issues.apache.org/jira/browse/SPARK-54588 and then update this PR.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL][TESTS] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "vboo123 (via GitHub)" <gi...@apache.org>.
vboo123 commented on code in PR #56690:
URL: https://github.com/apache/spark/pull/56690#discussion_r3470017822
##########
sql/core/src/test/resources/sql-tests/inputs/postgreSQL/time.sql:
##########
@@ -0,0 +1,43 @@
+--
+-- Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+--
Review Comment:
@MaxGekk thanks. Makes sense & I have removed 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL][TESTS] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "vboo123 (via GitHub)" <gi...@apache.org>.
vboo123 commented on PR #56690:
URL: https://github.com/apache/spark/pull/56690#issuecomment-4802392637
@MaxGekk gargvboo is the id. Thanks
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #56690:
URL: https://github.com/apache/spark/pull/56690#discussion_r3461464414
##########
sql/core/src/test/resources/sql-tests/inputs/try_cast.sql:
##########
@@ -51,4 +51,10 @@ SELECT TRY_CAST("2021-101-01" AS date);
-- TRY_CAST string to timestamp
SELECT TRY_CAST("2021-01-01 00:00:00" AS timestamp);
-SELECT TRY_CAST("2021-101-01 00:00:00" AS timestamp);
\ No newline at end of file
+SELECT TRY_CAST("2021-101-01 00:00:00" AS timestamp);
+
+-- TRY_CAST string to time
+SELECT TRY_CAST("12:34:56" AS time);
+SELECT TRY_CAST("12:34:56.789012" AS time(6));
+SELECT TRY_CAST("24:00:00" AS time);
+SELECT TRY_CAST("12:34:56 abc" AS time);
Review Comment:
Please add newline
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL][TESTS] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56690:
URL: https://github.com/apache/spark/pull/56690#issuecomment-4797334573
+1, LGTM. Merging to master/4.x.
Thank you, @vboo123 and @uros-b @shrirangmhalgi for review.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL][TESTS] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "vboo123 (via GitHub)" <gi...@apache.org>.
vboo123 commented on code in PR #56690:
URL: https://github.com/apache/spark/pull/56690#discussion_r3461986739
##########
sql/core/src/test/resources/sql-tests/inputs/postgreSQL/time.sql:
##########
@@ -0,0 +1,43 @@
+--
+-- Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+--
Review Comment:
@uros-b I included it since I thought it mirrors the existing `postgreSQL/date.sql` ([L1-L6](https://github.com/apache/spark/blob/master/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/date.sql#L1-L6)) and `postgreSQL/timestamp.sql` ([L1-L6](https://github.com/apache/spark/blob/master/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/timestamp.sql#L1-L6)). Thesecarry the same PostgreSQL attribution header since, from what I understand, these are ports of PostgreSQL's src/test/regress/sql/* tests.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL][TESTS] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56690:
URL: https://github.com/apache/spark/pull/56690#issuecomment-4797352566
@vboo123 Could you give me your JIRA id to assign the ticket to you.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "vboo123 (via GitHub)" <gi...@apache.org>.
vboo123 commented on PR #56690:
URL: https://github.com/apache/spark/pull/56690#issuecomment-4781006634
Thanks for the review @MaxGekk and the comments. I will address the optional parity gaps as a follow up.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #56690:
URL: https://github.com/apache/spark/pull/56690#discussion_r3461477945
##########
sql/core/src/test/resources/sql-tests/inputs/postgreSQL/time.sql:
##########
@@ -0,0 +1,43 @@
+--
+-- Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+--
Review Comment:
Is this an accidental license include?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL][TESTS] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56690:
URL: https://github.com/apache/spark/pull/56690#discussion_r3465171923
##########
sql/core/src/test/resources/sql-tests/inputs/postgreSQL/time.sql:
##########
@@ -0,0 +1,43 @@
+--
+-- Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+--
Review Comment:
time.sql should reflect Postresql's time.sql precisely except queries not supported by Spark SQL (that should be commented), see https://issues.apache.org/jira/browse/SPARK-27764. I would avoid adding new file to `inputs/postgreSQL`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57563][SQL] Add SQL golden-file test coverage for TIME to match DATE/TIMESTAMP [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org