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

Add javadoc on internal UUID format #23961

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

ZacBlanco
Copy link
Contributor

@ZacBlanco ZacBlanco commented Nov 6, 2024

Description

Adds a javadoc for presto-native developers to reference the internal format of UUIDs

Motivation and Context

Helps better specify the format to maintain compatibility

Impact

N/A

Test Plan

N/A

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

If release note is NOT required, use:

== NO RELEASE NOTE ==

@ZacBlanco
Copy link
Contributor Author

ZacBlanco commented Nov 6, 2024

Discussed with @BryanCutler about this who has been working on the Presto native support. This will be the officially-documented format for now. However, in the future we would like to update our slice library to be able to perform comparisons without byte swapping the longs internally. This should improve performance for UUIDs in both java and native. The improvement to the behavior of byte-wise comparisons in slices will likely be done in tandem with the JDK upgrade as it allows us to use some new JDK libraries to do this comparison.

BryanCutler
BryanCutler previously approved these changes Nov 6, 2024
Copy link
Contributor

@BryanCutler BryanCutler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

zation99
zation99 previously approved these changes Nov 16, 2024
@tdcmeehan tdcmeehan added the from:IBM PR from IBM label Nov 22, 2024
@prestodb-ci prestodb-ci requested review from a team, ScrapCodes and imsayari404 and removed request for a team November 22, 2024 18:24
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nits, nothing significant. Thanks for the doc.

aditi-pandit
aditi-pandit previously approved these changes Nov 25, 2024
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ZacBlanco

Copy link
Contributor

@imsayari404 imsayari404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ZacBlanco

@ZacBlanco ZacBlanco dismissed stale reviews from aditi-pandit, zation99, and BryanCutler via 49550fc December 3, 2024 18:13
@ZacBlanco ZacBlanco force-pushed the upstream-uuid-format-doc branch from 114a130 to 49550fc Compare December 3, 2024 18:13
* The value of a UUID is stored as two longs in little-endian
* format in the order of [MSB, LSB] when executing within Presto:
* <br>
*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping in a <pre> ... </pre> block might be better

* +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
* | MSB (LE) | LSB (LE) |
* +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*
* </pre>

* The value of a UUID is stored as two longs in little-endian
* format in the order of [MSB, LSB] when executing within Presto:
* <br>
*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*
* <pre>

Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the Clarification.

@tdcmeehan tdcmeehan merged commit f59aaa8 into prestodb:master Dec 4, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants