-
Notifications
You must be signed in to change notification settings - Fork 268
V3: Introduce timestamp_ns
and timestamptz_ns
#1632
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
This is looking great @sungwy 🥳
Ideally, we want to use |
I think it would make sense to break this PR up into two separate items:
This PR will focus on item (1) |
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.
Thanks for adding this! Looks great, just have a few comments
Looks like there is some issue with the
in I think this PR is ready to go. There are some parts we need to double-check, for example, the downcasting of micros to nanos, and that we don't have to fail for V3 when that isn't set. But since we don't yet support producing V3 metadata, I think that's okay. |
… into ns-timestamp
I removed this integration test because we can't write V3 metadata yet |
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!
Thanks for working on this @sungwy, and thanks for the reviews @smaheshwar-pltr and @kevinjqliu 🙌 |
Fixes: #1552
python native
datetime
module does not have support for nanoseconds. We'll need to update our internal date time representations to use a different library. numpy? arrow?