-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-47848: [C++] Add TimeUnit::PICO #48018
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
base: main
Are you sure you want to change the base?
Conversation
This enum value is unused for now, but I plan to introduce a timestamp128 type in the future which supports this.
|
|
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.
I think this change isn't necessary for this particular PR, but it will be for timestamp128 support. Let me know if you want me to break this out.
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.
Actually, maybe it is, because otherwise cpp/src/arrow/array/diff.cc won't compile. I needed to be able to handle all unit enum values, and I don't think it makes sense for picoseconds to be stored in anything smaller than int128.
This enum value is unused for now, but I plan to introduce a timestamp128 type in the future which supports this.
|
Looks like I probably need to adjust some linker stuff on Windows? Lots of errors like this: |
|
This seems to be a spec change. Perhaps it is better to discuss this on the [email protected] per the process https://arrow.apache.org/docs/format/Changing.html#discussion-and-voting-process |
Rationale for this change
I'm working on a system that supports picosecond precision timestamps, and I'm interested in interoperability with the Arrow ecosystem. See #47848
What changes are included in this PR?
This PR only adds the new enum value TimeUnit::PICO. This enum value is unused for now, but I plan to introduce a timestamp128 type in the future which supports this unit type.
Are these changes tested?
I have updated and run the C++ test suite.
Are there any user-facing changes?
This PR includes breaking changes to public APIs.
Adding a new enum variable did break some tests that assumed that all TimeUnit::values() are valid for use in the timestamp (64-bit) data type.