-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-51420][SQL] Get minutes of TIME datatype #50296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@MaxGekk looking forward to know your thoughts on this one! |
MaxGekk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, it would be nice to re-use the existing code for getting minutes, but I believe it would be better to implement minute from a TIME value as a separate expression.
...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
Outdated
Show resolved
Hide resolved
...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
Outdated
Show resolved
Hide resolved
|
Thanks for the review @MaxGekk |
@the-sakthi Let's leave it as is.
Could you introduce new expression |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Outdated
Show resolved
Hide resolved
|
Let me know if this updated PR aligns better with your suggestions, @MaxGekk |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralGenerator.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralGenerator.scala
Outdated
Show resolved
Hide resolved
...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala
Outdated
Show resolved
Hide resolved
...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala
Outdated
Show resolved
Hide resolved
|
@the-sakthi Could you update PR's description: This PR #50299 fixed the column name. |
|
Thanks for the review @MaxGekk , I'll shortly in few mins update the PR. Major changes:
|
|
Updated the revision with the suggested changes @MaxGekk ! Let me know how this one looks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the function usage/desc/example to the builder to fix a failing test which was complaining about this.
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Outdated
Show resolved
Hide resolved
|
Rebased with current main (master) branch! |
|
+1, LGTM. Merging to master. |
|
Thank you very much for helping on this one and merging @MaxGekk |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Show resolved
Hide resolved
… minute function ### What changes were proposed in this pull request? - Followup to the original PR: #50296 - Extend the minute(...) function (MinutesOfTime) to handle TIME types of any precision from 0 to 6. - Add tests verifying that minute(...) works for all valid TIME precisions. ### Why are the changes needed? - Previously, minute(...) did not consistently support TIME type inputs with arbitrary precision. - Users need the minute function to handle TIME(0) through TIME(6). ### Does this PR introduce _any_ user-facing change? - Yes. Users can now call minute(...) on TIME(p) columns or literals with any valid precision. ### How was this patch tested? By running new tests: ``` $ build/sbt "test:testOnly *TimeExpressionsSuite.scala" ``` By manual tests: ``` scala> spark.sql("select minute(cast('12:30' as time(0)));").show() +------------------------------+ |minute(CAST(12:30 AS TIME(0)))| +------------------------------+ | 30| +------------------------------+ scala> spark.sql("select minute(cast('12:30' as time(2)));").show() +------------------------------+ |minute(CAST(12:30 AS TIME(2)))| +------------------------------+ | 30| +------------------------------+ scala> spark.sql("select minute(cast('12:30' as time(5)));").show() +------------------------------+ |minute(CAST(12:30 AS TIME(5)))| +------------------------------+ | 30| +------------------------------+ ``` ### Was this patch authored or co-authored using generative AI tooling? No Closes #50551 from the-sakthi/SPARK-51420-FOLLOWUP. Authored-by: Sakthi <sakthi@apache.org> Signed-off-by: Max Gekk <max.gekk@gmail.com>
What changes were proposed in this pull request?
This PR adds support for extracting the minute component from TIME (TimeType) values in Spark SQL.
Why are the changes needed?
minute(TIME'HH:MM:SS.######')behaves correctly without unnecessary type coercion.Does this PR introduce any user-facing change?
Yes
minute(TIME'HH:MM:SS.######')resulted in a type mismatch error or an implicit cast attempt to TIMESTAMP, which was incorrect.minute(TIME'HH:MM:SS.######')now works correctly for TIME values without implicit casting.How was this patch tested?
By running new tests:
Was this patch authored or co-authored using generative AI tooling?
No