-
Notifications
You must be signed in to change notification settings - Fork 28.7k
[SPARK-51423][SQL] Add the current_time() function for TIME datatype #50336
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
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.
... which returns the current time at the start of query evaluation.
I think the current implementation doesn't satisfy the requirement, and every call in a query returns new current time. Could you look at the CurrentTimestampLike
, and the rule:
spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala
Lines 108 to 111 in 2822526
/** | |
* Computes the current date and time to make sure we return the same result in a single query. | |
*/ | |
object ComputeCurrentTime extends Rule[LogicalPlan] { |
Ahh sure! Thanks for the early review, Max. Will refer |
Thanks for the feedback @MaxGekk , let me know how this one looks! |
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala
Outdated
Show resolved
Hide resolved
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala
Outdated
Show resolved
Hide resolved
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.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/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
Outdated
Show resolved
Hide resolved
Thanks for the feedback @MaxGekk , let me know how this revision one looks! Hopefully this is better aligned to the review comments! |
@the-sakthi Thank you for the ping. I am traveling right now. I will look at the PR slightly latter. |
Thanks for the response, Max, even while traveling. Will await your review! |
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 in general. Let me review it again tomorrow.
Sure thanks @MaxGekk ! Will wait. |
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 comment.
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Show resolved
Hide resolved
@the-sakthi Please, resolve conflicts. |
- Handling foldable expressions via multiple constructors approach rather than expressionBuilder - Added more tests - Split lines to avoid using scalastyle line limit config - Other review comments
@MaxGekk The latest push has the updated user description and rebased with latest main branch. Let me know! |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala
Outdated
Show resolved
Hide resolved
+1, LGTM. Merging to master. |
Thank you very much @MaxGekk ! |
What changes were proposed in this pull request?
This PR adds support for a new function current_time() which returns the current time at the start of query evaluation.
Why are the changes needed?
Adds a built-in current_time([n]) function returning just the time portion (in a TIME(n) type). This aligns Spark with other SQL systems offering a native time function, improves convenience for time-only queries, and complements existing functions like current_date and current_timestamp.
Does this PR introduce any user-facing change?
Yes, adds a new function. Users can now get the current time using this function.
How was this patch tested?
Manual testing as shown above and running UTs added:
Was this patch authored or co-authored using generative AI tooling?
No