-
Notifications
You must be signed in to change notification settings - Fork 599
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
refactor(types): spilt timestamp to timestamp and timestampnano #20192
base: main
Are you sure you want to change the base?
Conversation
This pull request has been modified. If you want me to regenerate unit test for any of the files related, please find the file in "Files Changed" tab and add a comment |
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.
After this PR, what's the precision of the timestamp
type?
timestamp -> 6 us |
I remembered timestamp should already be nano precision. Does it mean this is a breaking change? |
It was also a breaking change regarding the |
Yes. There should be no compatibility issues with this pr, but it will make some user behavior changes |
I am ok with this PR to keep the timestamp with us precision and introduce a new timestampnano type to introduce nano precision timestamp. It can make the code clearer and it doesn't make too much sense to let |
I agree. I meant this breaking change is acceptable and can actually "fix"/revert a previous breaking change. |
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.
Considering other system nano timestamp type naming conventions, I think it's better to use timestamp_ns
instead of timestampnano
https://arc.net/l/quote/xaiqjmve
https://duckdb.org/docs/sql/data_types/timestamp.html
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
in #19827, we support timestamp with nano, but we can't support interval with nano in a similar way. So we have to introduce the new type, and correspondingly, we should implement the new type timestamp_nano, so we distinguish between timestamp_nano and timestamp in this pr
Checklist
Documentation
Release note