Skip to content
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

feat: add current_date and current_timestamp functions #524

Closed

Conversation

MasseGuillaume
Copy link
Contributor

No description provided.

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

We need some more details around how this should behave. There was an old discussion on these functions a while back (I don't recall exactly where) and one of the concerns was that many query engines will go out of their way to ensure that this function gives a consistent result if invoked multiple times within a query.

For example, a query might be a streaming query (and thus the function is called on each batch as the data passes through) or a plan might have multiple project nodes (and each one might call this function) or maybe the plan includes distribution and this function is called both on the workers and on the master node.

Given that current_date and current_timestamp can be replaced by literals on the producer is it essential that this be part of the plan? If so, can you explain how it should behave in the above scenarios?

@MasseGuillaume
Copy link
Contributor Author

Ah got it, it makes sense. In my use case, I use substrait to serialize the SQL so a rich UI can display a query plan to the user. In this case, the query is not yet executed, so it would not make sense to constant fold the current_time/current_date. What's missing is substrait is an optional function result for non-deterministic functions.

@EpsilonPrime
Copy link
Member

EpsilonPrime commented Aug 30, 2023

Similar functionality will be added in #528 so I believe we can close this PR.

@westonpace
Copy link
Member

@EpsilonPrime did you perhaps mean #548 ?

@EpsilonPrime
Copy link
Member

Ah, yes, thanks for the correction!

@jacques-n
Copy link
Contributor

Closing in preference to #548

@jacques-n jacques-n closed this Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants