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

change date_part return types to f64 #3997

Closed
Tracked by #3148
waitingkuo opened this issue Oct 28, 2022 · 10 comments · Fixed by #4385
Closed
Tracked by #3148

change date_part return types to f64 #3997

waitingkuo opened this issue Oct 28, 2022 · 10 comments · Fixed by #4385
Labels
enhancement New feature or request

Comments

@waitingkuo
Copy link
Contributor

date_part currently returns i32 which might lose some information, e.g.

select date_part('second', timestamp '2000-01-01T00:00:00.1');
+--------------------------------------------------------+
| datepart(Utf8("second"),Utf8("2000-01-01T00:00:00.1")) |
+--------------------------------------------------------+
| 0                                                      |
+--------------------------------------------------------+
1 row in set. Query took 0.000 seconds.

while postgresql returns double precision

willy=# select date_part('second', timestamp '2000-01-01T00:00:00.1');
 date_part 
-----------
       0.1
(1 row)

is it recommended to follow postgresql's return type?
if it is preferred, perhaps we could do a separate pr to deal with this

change it to double precision

Describe the solution you'd like
A clear and concise description of what you want to happen.

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

perhaps we could consider return decimals instead to align with extract to return decimal. postgres doc mentions this:

For historical reasons, the date_part function returns values of type double precision. 
This can result in a loss of precision in certain uses. Using extract is recommended instead.

https://www.postgresql.org/docs/current/functions-datetime.html

Additional context
Add any other context or screenshots about the feature request here.

@waitingkuo waitingkuo added the enhancement New feature or request label Oct 28, 2022
@waitingkuo waitingkuo changed the title change date_part return types to f64 change date_part return types to f64 Oct 28, 2022
@comphead
Copy link
Contributor

@waitingkuo may I try this?

@waitingkuo
Copy link
Contributor Author

@waitingkuo may I try this?

sure, please try it~

@comphead
Copy link
Contributor

comphead commented Nov 1, 2022

@waitingkuo the problem is date_part calls for arrow::compute::kernels::temporal::second which is i32, https://docs.rs/arrow/latest/arrow/compute/kernels/temporal/fn.second.html and they dont have millisecond support. Do you think its good idea to implement milliseconds by ourselves?

@waitingkuo
Copy link
Contributor Author

@comphead

arrow-rs uses chrono-rs to extract these date parts. Chono-rs does have nanosecond while arrow-rs doesn't have.

We could extend arrow-rs to support nanosecond and then get back to this issue.

I think adding nanosecond support in arrow-rs is a good start issue. If you're interested in, I'm happy to help you to figure out the codes to modify. Basically, you could follow the second's logic to do so
https://github.com/apache/arrow-rs/blob/master/arrow/src/compute/kernels/temporal.rs#L723-L764

I could help to send a pr to arrow-rs as well

@waitingkuo
Copy link
Contributor Author

cc @alamb

@comphead
Copy link
Contributor

comphead commented Nov 1, 2022

@waitingkuo thanks for the details, let me try to add the ticket and PR to arrow-rs.

@comphead
Copy link
Contributor

comphead commented Nov 1, 2022

Related arrow-rs ticket apache/arrow-rs#2995

@comphead
Copy link
Contributor

comphead commented Nov 3, 2022

@waitingkuo the apache/arrow-rs#2995 merged, should we wait for next arrow-rs version? How it usually happens?

@waitingkuo
Copy link
Contributor Author

@comphead thank you. i think we need to wait for 27.0.0 . 26.0.0 is just fixed. It normally released 1 or 2 times a month

@comphead
Copy link
Contributor

waiting for #4199

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

Successfully merging a pull request may close this issue.

2 participants