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

Implement current_time Function #3982

Closed
2 tasks
Tracked by #3148
alamb opened this issue Oct 27, 2022 · 7 comments · Fixed by #4054
Closed
2 tasks
Tracked by #3148

Implement current_time Function #3982

alamb opened this issue Oct 27, 2022 · 7 comments · Fixed by #4054
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Oct 27, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
As we add more full featured date/time support for DataFusion users need to be able to use the current time to calculate things like "all data in the last 5 hours"

Describe the solution you'd like
Support the current_time postgres function https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-CURRENT

Note I think we should avoid the current_time(precision) variant initially -- we can implement it as a follow on PR.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
This is a feature request we got from an early (internal) user of InfluxDB IOx

I believe this is a good first issue as you can follow the existing implementation of now()

It would be a good exercise to add a new build in scalar function, physical planing support, and then tests

Note this the same ticket exists for the current_date function #3981

@alamb alamb added enhancement New feature or request good first issue Good for newcomers labels Oct 27, 2022
@naosense
Copy link
Contributor

This sounds good start to me, could you please assign it to me or some extra advises?

@waitingkuo
Copy link
Contributor

@alamb postgresql's current_time returns TIMETZ

willy=# select pg_typeof(current_time);
      pg_typeof      
---------------------
 time with time zone
(1 row)

what should we return here?

@alamb
Copy link
Contributor Author

alamb commented Oct 29, 2022

@waitingkuo I was imagining it would return a https://docs.rs/arrow/latest/arrow/array/type.Time64NanosecondArray.html

But I have not thought as deeply as you about the timezone implications, so if you have a different suggestion I would love to hear it.

This sounds good start to me, could you please assign it to me or some extra advises?

Hi @naosense -- thanks and done!

@comphead
Copy link
Contributor

This sounds good start to me, could you please assign it to me or some extra advises?

Hi @naosense please follow the #4022 PR

@waitingkuo
Copy link
Contributor

@alamb i have no idea for timetz in arrow-rs for now as Time has not timezone (e.g. DataType::Time64(TimeUnit::Nanosecond) while Timestamp has (e.g. DataType::Timestamp(TimeUnit::Nanosecond, Some("UTC".to_string()))

i think we could either

  1. follow postgresql's function signature that means we could implement LOCAL_TIME instead
  2. keep it simple, return TIME directly for now and comment it

in the long run, we might consider make these function names explicitly like what was discussed in
#686 (comment)

@naosense
Copy link
Contributor

This sounds good start to me, could you please assign it to me or some extra advises?

Hi @naosense please follow the #4022 PR

That's nice, thanks!

@alamb
Copy link
Contributor Author

alamb commented Oct 31, 2022

keep it simple, return TIME directly for now and comment it

This would be my suggestion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants