Skip to content

[SPARK-51414][SQL] Add the make_time() function#50269

Closed
robreeves wants to merge 78 commits intoapache:masterfrom
robreeves:SPARK-51414
Closed

[SPARK-51414][SQL] Add the make_time() function#50269
robreeves wants to merge 78 commits intoapache:masterfrom
robreeves:SPARK-51414

Conversation

@robreeves
Copy link
Copy Markdown
Contributor

@robreeves robreeves commented Mar 13, 2025

What changes were proposed in this pull request?

This adds a new function make_time for the new time datatype. A new function MakeTime is added and some logic was refactored out of MakeTimestamp to be shared with the new function.

Why are the changes needed?

This is needed to create time data type objects.

Does this PR introduce any user-facing change?

Yes, a new function make_time is added.

How was this patch tested?

New unit tests and manually spark-shell testing.

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions Bot added the SQL label Mar 13, 2025
@robreeves
Copy link
Copy Markdown
Contributor Author

@MaxGekk this is ready for review

@robreeves robreeves marked this pull request as ready for review March 15, 2025 04:28
@robreeves
Copy link
Copy Markdown
Contributor Author

These test failures don't look related to my changes. I'm rerunning them.

@MaxGekk
Copy link
Copy Markdown
Member

MaxGekk commented Mar 15, 2025

This test failure will be fixed by #50282

[info] - check outputs of expression examples *** FAILED *** (9 seconds, 272 milliseconds)
...
[info]   Cause: scala.MatchError: (06:30:45.887,TimeType(6)) (of class scala.Tuple2)
[info]   at org.apache.spark.sql.execution.HiveResult$.toHiveString(HiveResult.scala:111)

@robreeves
Copy link
Copy Markdown
Contributor Author

@MaxGekk this is ready for review again

Copy link
Copy Markdown
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robreeves Could you add a couple of end-to-end tests to

-- time literals, functions and operations
create temporary view time_view as select '11:53:26.038344' time_str, 'HH:mm:ss.SSSSSS' fmt_str;
select time '16:39:45\t';
select to_time(null), to_time('01:02:03'), to_time('23-59-59.999999', 'HH-mm-ss.SSSSSS');
select to_time(time_str, fmt_str) from time_view;
-- missing fields in `to_time`
select to_time("11", "HH");
-- invalid: there is no 13 hours
select to_time("13-60", "HH-mm");
select try_to_time(null), try_to_time('00:12:00'), try_to_time('01:02:03', 'HH:mm:ss');
select try_to_time(1);
select try_to_time('12:48:31 abc');
select try_to_time('10:11:12.', 'HH:mm:ss.SSSSSS');
select try_to_time("02-69", "HH-mm");
select try_to_time('11:12:13', 'HH:mm:ss', 'SSSSSS');

LGTM in general. Please, resolve conflicts.

@robreeves robreeves requested a review from MaxGekk April 16, 2025 20:10
@MaxGekk
Copy link
Copy Markdown
Member

MaxGekk commented Apr 17, 2025

+1, LGTM. Merging to master.
Thank you, @robreeves.

@MaxGekk MaxGekk closed this in 216f05b Apr 17, 2025
@MaxGekk
Copy link
Copy Markdown
Member

MaxGekk commented Apr 17, 2025

@robreeves Congratulations with your first contribution to Apache Spark!

@robreeves
Copy link
Copy Markdown
Contributor Author

@MaxGekk thanks for the reviews! This wasn't my first contribution to Spark, but was my first change in the SQL layer.

@MaxGekk
Copy link
Copy Markdown
Member

MaxGekk commented Apr 19, 2025

@robreeves There is a list of contributors of Apache Spark in JIRA. You wasn't in the list for some reasons (I guess, a previous committer forgot to add you). Now you are in the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants