-
Notifications
You must be signed in to change notification settings - Fork 874
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
JAVA-3061 Re-introduce an improved CqlVector, add support for accessing vectors directly as float arrays #1666
Conversation
core/src/test/java/com/datastax/oss/driver/internal/core/type/codec/VectorCodecTest.java
Show resolved
Hide resolved
Okay, this should largely be ready for validation from everybody involved. I still need an explicit test for CqlVector itself; that's next, but my main goal was to get the API in front of everyone so that we could get buy-off on the general idea. As of the most recent round of commits this should build a usable artifact. @eolivelli I'm most interested in feedback from you confirming that this will address the use case you were addressing with the change in #1643. There should be equivalent changes to what you had there in this PR but I'd like explicit confirmation from you before we proceed with this thing. |
...stax/oss/driver/internal/core/type/codec/registry/CachingCodecRegistryTestDataProviders.java
Outdated
Show resolved
Hide resolved
...ain/java/com/datastax/oss/driver/internal/core/type/codec/registry/CachingCodecRegistry.java
Outdated
Show resolved
Hide resolved
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.
Added a few nit-pick comments, lgtm
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 looks easy to use and efficient!
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. From perspective of flexibility for usage, I like the operations provided by CqlVector
.
core/src/main/java/com/datastax/oss/driver/api/core/type/reflect/GenericType.java
Outdated
Show resolved
Hide resolved
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
Excellent, thanks @hhughes! Given all of the feedback above and the various conversations in various other venues I'm going to call this good and merge this change now. There are several downstream efforts which depend on a released driver that includes these changes and finalizing this and getting it merged is necessary to make that happen. A huge thanks to everybody who was involved in this process; thanks for making the Java driver's support for vectors even better! |
Fixes in the same vein as #1643 adapted to play nicely with recent changes to the Java types representing CQL vectors.