-
Notifications
You must be signed in to change notification settings - Fork 96
GH-825: Add UUID canonical extension type #903
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 comment has been minimized.
This comment has been minimized.
lidavidm
left 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.
See previous PR. It is not sufficient to move the existing files, the "uuid" type used in tests was never meant to implement the canonical type. #824
|
@lidavidm could you take another look? Let me know if anything else is missing |
bodduv
left 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.
Thank you for adding the canonical type.
vector/src/main/java/org/apache/arrow/vector/complex/impl/UuidWriterImpl.java
Show resolved
Hide resolved
vector/src/main/java/org/apache/arrow/vector/FixedSizeExtensionType.java
Outdated
Show resolved
Hide resolved
|
there are some integration tests failling: It seems the metadata elements when it's a map the order is not maintained. I this a known issue? @lidavidm |
|
It's apache/arrow-go#571 |
|
@lidavidm it was merged, should this be re-run? |
|
@lidavidm anything else missing? |
bodduv
left 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.
Thank you for addressing the comments.
lidavidm
left 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.
This has conflicts
| /** Registers the UuidType in the extension type registry. */ | ||
| private static final AtomicBoolean registered = new AtomicBoolean(false); | ||
|
|
||
| static { | ||
| ExtensionTypeRegistry.register(INSTANCE); | ||
| } | ||
|
|
||
| /** Register the extension type so it can be used globally. */ | ||
| public static void ensureRegistered() { | ||
| if (!registered.getAndSet(true)) { | ||
| // The values don't matter, we just need an instance | ||
| ExtensionTypeRegistry.register(UuidType.INSTANCE); | ||
| } | ||
| } |
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.
Isn't all of this redundant? If we have the static initializer, then ensureRegistered can just be a no-op. Or if we're going to use the atomic boolean, then we don't need the static initializer.
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.
this is only used in tests similarly to Opaque extension type
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.
There is no static initializer there. There is one here.
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.
Thank you! I added the static implementation following your suggestion
What's Changed
Add Uuid Extension type canonical implementation in Java. The proposed implementation was validated in Dremio engine
Closes #825