JAVA-3061 Remove CqlVector, represent CQL vector types as Lists #1656

absurdfarce · 2023-06-22T14:52:55Z

While also playing nice with CqlVectorCodec. Also set out to add some Javadoc in order to make usage a bit clearer.

Largely derived from previous work done by @jshook in PR 1641

absurdfarce · 2023-06-22T15:43:14Z

My original comment reviewing what became this change I mentioned that I didn't want to use both the newInstance() methods and the builder in the same impl. After reading comments from the other PR (and in other spots) I realized this wasn't really a sensible position... so I decided to leave both in.

jshook

I only have one issue which I think is critical to address, which is the direct values accessor. Please see my comment for details.

jshook · 2023-06-22T17:47:07Z

core/src/main/java/com/datastax/oss/driver/api/core/data/CqlVector.java

+ * <p>Instances may be created iteratively using {@link CqlVector.Builder} or in a single method
+ * call via {@link #newInstance(Object[])} or {@link #newInstance(Collection)}.
+ */
+public class CqlVector<T> implements Iterable<T> {


There is still a critical need to have simple access to the underlying values. The iterative accessor is simply not sufficient for our application's needs.

Looking at the pros/cons, my understanding is:

Iterable access provides a generic approach and could be used where lazy evaluation and large datasets are known.

List commonly used interface, easy way to work with collections, random access, specific list operations. This also allows for getting an idea of the size.

Since our full set of vector search use cases are somewhat vague at this point, +1 for having both options.

I attempted to address this by adding an export(Collection) method which dumps the vectors content into a collection type of your choosing. This is significantly more flexible than exporting a single collection type; whatever type we export may have properties (list vs. set vs. queue, immutable vs. immutable, linked lists vs. array lists and so on) which almost certainly will not apply in even a majority of situations. This approach allows API users to create the list in whatever way they see fit (and even perform some initial operations on said list if they choose to do so) and then hand it over to our vector for supplying the data.

It's my hope that this addresses your immediate concern @jshook while also maximizing our flexibility for the many Java driver users who will have use cases with different parameters. If I outright missed the boat here and didn't understand something please let me know.

This is an ergonomic improvement, and quite flexible. However, it doesn't improve the situation around the heap allocation acrobatics for large (assume 2K nominal) vectors. I'm working up some microbenches to quantify this better. It seems worth it at least for something that, once settled, will be how CqlVector works for a long time.

…in order to work with it in other contexts.

…SequenceType interface and make both ListType and VectorType extend from that. Also converted VectorType to a pure interface and moved impl to DefaultVectorType to better match driver conventions.

absurdfarce · 2023-06-23T23:30:05Z

I spent some time looking at this last night, trying out different ideas like supporting custom codecs for mutable vector types. There just wasn't a clean way to do this which generalized nicely, but fortunately while digging into these ideas I hit upon some comments which inverted my original way of thinking. CqlDuration is the exception here, it isn't the model to be followed... which in turn means we have no particular need to have a custom type for vectors. With that in mind it was much easier to simply represent vectors in Java as a List with elements of some subtype, a change implemented by the most recent set of commits.

The change was quite intrusive, changing the direction of this PR and making it quite a bit bigger than it was. But after this change you now do things like the following:

            ResultSet rs = session.execute("select j from test.foo where j ann of [3.4, 7.8, 9.1] limit 1");
            for (Row row : rs) {
                List<Float> v = row.getVector(0, Float.class);
                System.out.println(v)
            }

And this:

            PreparedStatement preparedInsert = session.prepare("insert into test.foo (i, j) values (?,?)");
            List<Float> vector = Lists.newArrayList(1.4f, 2.5f, 3.6f);
            session.execute(preparedInsert.bind(3, vector));
            session.execute(preparedInsert.bind(4).setVector(1, vector, Float.class));
            session.execute(preparedInsert.bind(5).setVector("j", vector, Float.class));
            session.execute(preparedInsert.bind(6).setVector(CqlIdentifier.fromInternal("j"), vector, Float.class));

This avoids all the Sturm und Drang brought on by the prior implementation while also behaving in a way that fits more naturally with the Java drivers re-use of existing Java types wherever possible. Finally, it also more closely mirrors what we do with the Python driver, which isn't necessary but is a sign that we're on a good path.

A lot of this PR now covers driver internals, so really my interest for the reviewers who don't work on the Java driver on a regular basis is to make sure they're okay with the idea of doing away with CqlVector and using a List instead. We can handle the review of these changes from the perspective of the Java driver code base separately.

jeffbanks · 2023-06-24T00:02:41Z

@absurdfarce as you stated, the internal changes are less of a concern for the majority of folks on this PR interested in the general use of what was CqlVector. I'm 👍🏼 for this implementation, it seems to address the concerns raised by @jshook. I do like the fact that this aligns with the Python driver implementation.

jeffbanks

lgtm for the cqlvector usage; the driver specific bits, I'm leaving that detail to those more familiar with the internals.

…es what's already done with ListCodec.

eolivelli

+1 very nice

jshook

This looks good to me.We'll try it out as soon as we can in a release. It looks like you also added some APIs to help manage collection/container types more cleanly and efficently too, from what little I know of the driver internals. Thanks for taking this to the next level!

absurdfarce · 2023-06-26T17:56:08Z

A quick note on this: @hhughes and I will be walking through these changes (including the changes to the driver innards) in detail later this afternoon. If that goes well this will get merged and the set of other fixes dependent on the outcome of this PR will get addressed.

hhughes

One comment about edge case checking when encoding vectors and a few inconsistencies in javadocs. LGTM

hhughes · 2023-06-26T22:49:16Z

core/src/main/java/com/datastax/oss/driver/api/core/data/SettableByIndex.java

@@ -423,8 +423,9 @@ default SelfT setCqlDuration(int i, @Nullable CqlDuration v) {
   */
  @NonNull
  @CheckReturnValue
-  default SelfT setCqlVector(int i, @Nullable CqlVector<?> v) {
-    return set(i, v, CqlVector.class);
+  default <ElementT> SelfT setVector(


* Sets the {@code i}th value to the provided duration.

Should this be vector?

It should indeed... good catch @hhughes!

hhughes · 2023-06-26T22:50:05Z

core/src/main/java/com/datastax/oss/driver/api/core/data/SettableByName.java

@@ -570,10 +570,11 @@ default SelfT setCqlDuration(@NonNull String name, @Nullable CqlDuration v) {
   */
  @NonNull
  @CheckReturnValue
-  default SelfT setCqlVector(@NonNull String name, @Nullable CqlVector<?> v) {
+  default <ElementT> SelfT setVector(


Replace duration with vector in the javadoc

Yup, this one too!

hhughes · 2023-06-26T23:01:22Z

core/src/main/java/com/datastax/oss/driver/internal/core/type/codec/VectorCodec.java

    if (value == null || cqlType.getDimensions() <= 0) {
      return null;
    }
    ByteBuffer[] valueBuffs = new ByteBuffer[cqlType.getDimensions()];
-    Iterator<SubtypeT> values = value.getValues().iterator();
+    Iterator<SubtypeT> values = value.iterator();
    int allValueBuffsSize = 0;
    for (int i = 0; i < cqlType.getDimensions(); ++i) {
      ByteBuffer valueBuff = this.subtypeCodec.encode(values.next(), protocolVersion);


ListCodec and MapCodec have some handling around null elements and classcastexceptions, do we want to have the same checks here?

My original thought was that this wasn't necessary because the codecs were pretty good about not meeting these error cases. Then I went back and looked at the code and realized that recollection was entirely wrong. Added that code in there (as well as check for a few other error conditions) and beefed up the codec test a bit to detect them.

absurdfarce · 2023-06-27T15:44:23Z

Okay, we have buy-in from consumers on the API changes and some level of confidence for the driver internals (hat tip to @hhughes). I'm gonna call this good and merge so that I can start working on the downstream dependencies of this one.

absurdfarce added 3 commits June 21, 2023 22:04

Initial take

dfd3087

Some minor build fixes

0185e7a

A few other minor fixes

e1b1daf

absurdfarce mentioned this pull request Jun 22, 2023

JAVA-3061 CqlVector refinements #1641

Closed

Trying to make revapi happy

fcc3ce4

absurdfarce requested review from jshook, jeffbanks, ShaunakDas88, msmygit and MikeYaacoubStax June 22, 2023 15:45

jshook suggested changes Jun 22, 2023

View reviewed changes

absurdfarce added 6 commits June 22, 2023 17:12

Add export() in an attempt to simplify getting data out of CqlVector …

67ec906

…in order to work with it in other contexts.

Initial steps in removing CqlVector

e9f30ae

Removal completed

96a521d

Updates to revapi config

50cade5

Fix for CachingCodecRegistry. Easiest option was to introduce common …

046b146

…SequenceType interface and make both ListType and VectorType extend from that. Also converted VectorType to a pure interface and moved impl to DefaultVectorType to better match driver conventions.

Merge branch '4.x' into java3061

671c060

absurdfarce changed the title ~~JAVA-3061 Improve CqlVector usability~~ JAVA-3061 Remove CqlVector, represent CQL vector types as Lists Jun 23, 2023

jeffbanks approved these changes Jun 24, 2023

View reviewed changes

absurdfarce added 2 commits June 23, 2023 19:40

Forgot to switch VectorCodec over to use ArrayList. This nicely match…

563532b

…es what's already done with ListCodec.

Some additional minor refactorings

8f7cb9c

eolivelli reviewed Jun 26, 2023

View reviewed changes

absurdfarce requested a review from jshook June 26, 2023 15:04

jshook approved these changes Jun 26, 2023

View reviewed changes

Feedback from code review

88a01b7

hhughes approved these changes Jun 26, 2023

View reviewed changes

Additional feedback from code review

b0815c2

absurdfarce merged commit 6cf075f into 4.x Jun 27, 2023

absurdfarce deleted the java3061 branch June 27, 2023 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JAVA-3061 Remove CqlVector, represent CQL vector types as Lists #1656

JAVA-3061 Remove CqlVector, represent CQL vector types as Lists #1656

absurdfarce commented Jun 22, 2023 •

edited

Loading

absurdfarce commented Jun 22, 2023

jshook left a comment

jshook Jun 22, 2023

jeffbanks Jun 22, 2023

absurdfarce Jun 22, 2023

jshook Jun 22, 2023

absurdfarce commented Jun 23, 2023 •

edited

Loading

jeffbanks commented Jun 24, 2023

jeffbanks left a comment

eolivelli left a comment

jshook left a comment

absurdfarce commented Jun 26, 2023

hhughes left a comment

hhughes Jun 26, 2023

absurdfarce Jun 27, 2023

absurdfarce Jun 27, 2023

hhughes Jun 26, 2023

absurdfarce Jun 27, 2023

absurdfarce Jun 27, 2023

hhughes Jun 26, 2023

absurdfarce Jun 27, 2023

absurdfarce commented Jun 27, 2023

JAVA-3061 Remove CqlVector, represent CQL vector types as Lists #1656

JAVA-3061 Remove CqlVector, represent CQL vector types as Lists #1656

Conversation

absurdfarce commented Jun 22, 2023 • edited Loading

absurdfarce commented Jun 22, 2023

jshook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absurdfarce commented Jun 23, 2023 • edited Loading

jeffbanks commented Jun 24, 2023

jeffbanks left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

jshook left a comment

Choose a reason for hiding this comment

absurdfarce commented Jun 26, 2023

hhughes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absurdfarce commented Jun 27, 2023

absurdfarce commented Jun 22, 2023 •

edited

Loading

absurdfarce commented Jun 23, 2023 •

edited

Loading