-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-51419][SQL] Get hours of TIME datatype #50355
base: master
Are you sure you want to change the base?
Conversation
Hi @MaxGekk Could you please review this PR? |
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.
Could you fix the output of the example:
scala> spark.sql("SELECT hour(TIME'07:01:09.12312321231232');").show()
+------------------------------+
|minute(TIME '07:01:09.123123')|
+------------------------------+
| 7|
+------------------------------+
should be hour
not minute
.
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
...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala
Outdated
Show resolved
Hide resolved
Yes corrected |
Modified as per review feedback @MaxGekk , Please check it looks good |
ExpressionsSchemaSuite failled with below error
|
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/resources/sql-functions/sql-expression-schema.md
Outdated
Show resolved
Hide resolved
|
||
test("Hour with TIME type") { | ||
// A few test times in microseconds since midnight: | ||
// time in microseconds -> expected minute |
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.
// time in microseconds -> expected minute | |
// time in microseconds -> expected hours |
...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Show resolved
Hide resolved
...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Outdated
Show resolved
Hide resolved
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.
Overall, LGTM except @MaxGekk 's comments.
…Builder in TimeExpressionSuite
…rExpressionBuilder" This reverts commit e24c07f.
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.
lgtm except of a couple comments.
What changes were proposed in this pull request?
This PR adds support for extracting the hour component from TIME (TimeType) values in Spark SQL.
Why are the changes needed?
Spark previously supported hour() for only TIMESTAMP type values. TIME support was missing, leading to implicit casting attempt to TIMESTAMP, which was incorrect. This PR ensures that
hour(TIME'HH:MM:SS.######')
behaves correctly without unnecessary type coercion.Does this PR introduce any user-facing change?
Yes
How was this patch tested?
By running new tests:
$ build/sbt "test:testOnly *TimeExpressionsSuite"
Was this patch authored or co-authored using generative AI tooling?
No