JAVA-3143: Extend driver vector support to arbitrary subtypes and fix handling of variable length types (OSS C* 5.0)

[SiyaoIsHiding]

No description provided.

[SiyaoIsHiding]

This branch is on top of #1931

[SiyaoIsHiding]

Previously we promised that CqlVector.from should mirror CqlVector.toString. CqlVector.from has access to a subtype codec and expects some values to be single-quoted, e.g. texts. Therefore, without this change to toString, a round-trip test like CqlVector.from(vector1.toString(), codec); will fail. However, fetching the default codec for the subtype looks like too much effort for something simple like toString.

[joao-r-reis]

We could store the subtype codec as a field of the vector but then it will only be available if the vector was created with from() so it's a bit weird.

Using the default codec could lead to unexpected behavior if the user has custom codecs so I don't think it's a good solution either.

The C# CqlVector class has methods to easily convert between native arrays and CqlVector objects so that the user can use existing code that is compatible with arrays like debugging, printing, etc. There's no way to convert a C# CqlVector object to string directly but there's also no way to create a CqlVector object from a string representation. I think this was a fine idea when CqlVector was only for float values but it becomes very complex to support when we extend the subtype to any cql supported type including collections, udts, etc.

Personally I'd try to find a way to keep supporting this for subtypes that were already supported before this change but just throw an exception if the subtype would require access to the TypeCodec to make it work. Alternatively if we don't really mind breaking this string conversion support for existing apps that use the current CqlVector class then that would also work but I don't think that's a good idea.

[SiyaoIsHiding]

In the past we only supported the from and toString of number elements. I updated so that toString only works for number elements and will throw otherwise.

[SiyaoIsHiding]

On no, if toString throws an exception, it will cause too much inconvenience, e.g. the data provider annotation will even break. I changed it to

    return this.list.stream()
        .map(ele -> ele.toString())
        .collect(Collectors.joining(", ", "[", "]"));

The only problem is that strings and date times are not properly quoted. Everything else works fine.

[joao-r-reis]

👍 looks fine to me, @absurdfarce can I get your feedback here before I close this thread?

[absurdfarce]

I mean, I... guess this is fine. But it seems to me like the current implementation pretty much does... exactly what Iterables.toString() does, right? Maybe I'm missing something there... but I don't think so. This passes without issue:

$ git diff
diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/data/CqlVector.java b/core/src/main/java/com/datastax/oss/driver/api/core/data/CqlVector.java
index 4e2fc77cf..69da04a9b 100644
--- a/core/src/main/java/com/datastax/oss/driver/api/core/data/CqlVector.java
+++ b/core/src/main/java/com/datastax/oss/driver/api/core/data/CqlVector.java
@@ -241,6 +241,8 @@ public class CqlVector<T> implements Iterable<T>, Serializable {
     	.collect(Collectors.joining(", ", "[", "]"));
   }
 
+  public String oldToString() { return Iterables.toString(this.list); }
+
   /**
	* Serialization proxy for CqlVector. Allows serialization regardless of implementation of list
	* field.
diff --git a/core/src/test/java/com/datastax/oss/driver/api/core/data/CqlVectorTest.java b/core/src/test/java/com/datastax/oss/driver/api/core/data/CqlVectorTest.java
index 3e0872cb9..208440d30 100644
--- a/core/src/test/java/com/datastax/oss/driver/api/core/data/CqlVectorTest.java
+++ b/core/src/test/java/com/datastax/oss/driver/api/core/data/CqlVectorTest.java
@@ -29,6 +29,7 @@ import com.tngtech.java.junit.dataprovider.UseDataProvider;
 import java.io.ByteArrayInputStream;
 import java.io.ObjectInputStream;
 import java.io.ObjectStreamException;
+import java.time.LocalDate;
 import java.time.LocalTime;
 import java.util.AbstractList;
 import java.util.ArrayList;
@@ -256,4 +257,15 @@ public class CqlVectorTest {
   	assertThat(e).isInstanceOf(ObjectStreamException.class);
 	}
   }
+
+  @Test
+  public void compare_iterables_to_custom() {
+
+	CqlVector<String> stringVector = CqlVector.newInstance("abc", "def", "ghi");
+	assertThat(stringVector.toString()).isEqualTo(stringVector.oldToString());
+	CqlVector<LocalTime> timeVector = CqlVector.newInstance(LocalTime.now(), LocalTime.now(), LocalTime.now());
+	assertThat(timeVector.toString()).isEqualTo(timeVector.oldToString());
+	CqlVector<LocalDate> dateVector = CqlVector.newInstance(LocalDate.now(), LocalDate.now(), LocalDate.now());
+	assertThat(dateVector.toString()).isEqualTo(dateVector.oldToString());
+  }
 }

I guess I'd argue that unless our custom implementation gives us something we don't get from the well-proven third-party lib I'd just as soon continue using Iterables.toString()

[lukasz-antoniak]

Shall we expose a getter like for other fields?

[joao-r-reis]

Looks good! I left some comments but they're almost all NITs

[joao-r-reis]

We could store the subtype codec as a field of the vector but then it will only be available if the vector was created with from() so it's a bit weird.

Using the default codec could lead to unexpected behavior if the user has custom codecs so I don't think it's a good solution either.

The C# CqlVector class has methods to easily convert between native arrays and CqlVector objects so that the user can use existing code that is compatible with arrays like debugging, printing, etc. There's no way to convert a C# CqlVector object to string directly but there's also no way to create a CqlVector object from a string representation. I think this was a fine idea when CqlVector was only for float values but it becomes very complex to support when we extend the subtype to any cql supported type including collections, udts, etc.

Personally I'd try to find a way to keep supporting this for subtypes that were already supported before this change but just throw an exception if the subtype would require access to the TypeCodec to make it work. Alternatively if we don't really mind breaking this string conversion support for existing apps that use the current CqlVector class then that would also work but I don't think that's a good idea.

[joao-r-reis]

Why did you decide to stop using paramSplitter?

[SiyaoIsHiding]

paramSplitter does not work for cases like vector<vector<float,2>,2>

[joao-r-reis]

Does it make sense to improve paramSplitter to work for these cases instead? Maybe Bret's feedback here is needed as well @absurdfarce

[absurdfarce]

I agree with @SiyaoIsHiding that the original impl doesn't handle nested vector types. For example, with the current code the following fails on "test_type_string_nested" but passes everywhere else:

package com.datastax.oss.driver.api.core.type;

import com.datastax.oss.driver.internal.core.type.DefaultVectorType;
import org.junit.Test;

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

public class DataTypesTest {

    private DataType float3 = DataTypes.vectorOf(DataTypes.FLOAT,3);

    @Test
    public void test_type_string_basic() {
        String typeString = "org.apache.cassandra.db.marshal.VectorType(" +
                "org.apache.cassandra.db.marshal.FloatType,3)";
        DataType dataType = DataTypes.custom(typeString);
        assertThat(dataType).isInstanceOf(DefaultVectorType.class);
        DefaultVectorType result = (DefaultVectorType)dataType;
        assertThat(result.getElementType()).isEqualTo(DataTypes.FLOAT);
        assertThat(result.getDimensions()).isEqualTo(3);
    }

    @Test
    public void test_type_string_nested() {
        String typeString = "org.apache.cassandra.db.marshal.VectorType(" +
                "org.apache.cassandra.db.marshal.VectorType(" +
                "org.apache.cassandra.db.marshal.FloatType,3), 3)";
        DataType dataType = DataTypes.custom(typeString);
        assertThat(dataType).isInstanceOf(DefaultVectorType.class);
        DefaultVectorType result = (DefaultVectorType)dataType;
        assertThat(result.getElementType()).isEqualTo(float3);
        assertThat(result.getDimensions()).isEqualTo(3);
    }

    @Test
    public void test_vector_data_type_basic() {
        assertThat(float3).isInstanceOf(DefaultVectorType.class);
        DefaultVectorType result = (DefaultVectorType)float3;
        assertThat(result.getElementType()).isEqualTo(DataTypes.FLOAT);
        assertThat(result.getDimensions()).isEqualTo(3);
    }

    @Test
    public void test_vector_data_type_nested() {
        DataType float3 = DataTypes.vectorOf(DataTypes.FLOAT,3);
        DataType float3Nested = DataTypes.vectorOf(float3, 3);
        assertThat(float3Nested).isInstanceOf(DefaultVectorType.class);
        DefaultVectorType result = (DefaultVectorType)float3Nested;
        assertThat(result.getElementType()).isEqualTo(float3);
        assertThat(result.getDimensions()).isEqualTo(3);
    }
}

The changes in this PR make that test pass in it's entirety.

I will argue that the implementation in this PR does split the logic for handling these argument lists into two places. Note that DataTypeClassNameParser already has logic for handling type parameters for type strings. This PR then re-implements something similar in DataTypes.custom(). This has the effect of actually bouncing back and forth between the two classes when dealing with nested types: DataTypes.custom() calls DTCNP.parse() which calls DataTypes.custom() if it doesn't find a customized handler for a type and so on.

This back-and-forth behaviour seems to be the key to the whole thing. To me it's indicating that DataTypeClassNameParser hasn't been properly updated to handle vector types yet. A quick review of the source code confirms that that's the case. I'll note that I can also make the DataTypesTest class above pass via the following change:

$ git diff                                                                                                                [29/29]
diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/type/DataTypes.java b/core/src/main/java/com/datastax/oss/driver/api/core/type/DataTypes.java
index 4447e2830..492fc121c 100644
--- a/core/src/main/java/com/datastax/oss/driver/api/core/type/DataTypes.java
+++ b/core/src/main/java/com/datastax/oss/driver/api/core/type/DataTypes.java
@@ -65,29 +65,8 @@ public class DataTypes {  
     if (className.equals("org.apache.cassandra.db.marshal.DurationType")) return DURATION;
  
     /* Vector support is currently implemented as a custom type but is also parameterized */                                                                                                                   -    if (className.startsWith(DefaultVectorType.VECTOR_CLASS_NAME)) {
-      String paramsString =     
-          className.substring(                                                                                                                                                                                 
-              DefaultVectorType.VECTOR_CLASS_NAME.length() + 1, className.length() - 1);                                                                                                                       
-      int lastCommaIndex = paramsString.lastIndexOf(',');                       
-      if (lastCommaIndex == -1) {                                                                      
-        throw new IllegalArgumentException(                                                            
-            String.format(                                                                             
-                "Invalid vector type %s, expected format is %s<subtype, dimensions>",
-                className, DefaultVectorType.VECTOR_CLASS_NAME));        
-      }                                                                                                
-      String subTypeString = paramsString.substring(0, lastCommaIndex).trim();   
-      String dimensionsString = paramsString.substring(lastCommaIndex + 1).trim();
-                                                                                                       
-      DataType subType = classNameParser.parse(subTypeString, AttachmentPoint.NONE);
-      int dimensions = Integer.parseInt(dimensionsString);
-      if (dimensions <= 0) {
-        throw new IllegalArgumentException(
-            String.format(
-                "Request to create vector of size %d, size must be positive", dimensions));
-      }               
-      return new DefaultVectorType(subType, dimensions);
-    }
+    if (className.startsWith(DefaultVectorType.VECTOR_CLASS_NAME))
+      return classNameParser.parse(className, AttachmentPoint.NONE);
     return new DefaultCustomType(className);
   }
diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/parsing/DataTypeClassNameParser.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/parsing/Da
taTypeClassNameParser.java
index fd6f1a4bd..4cf77d467 100644                                                                       
--- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/parsing/DataTypeClassNameParser.java
+++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/parsing/DataTypeClassNameParser.java
@@ -26,17 +26,21 @@ import com.datastax.oss.driver.api.core.type.UserDefinedType;
 import com.datastax.oss.driver.api.core.type.codec.TypeCodecs;
 import com.datastax.oss.driver.internal.core.context.InternalDriverContext;
 import com.datastax.oss.driver.internal.core.type.DefaultTupleType;
+import com.datastax.oss.driver.internal.core.type.DefaultVectorType;
 import com.datastax.oss.driver.internal.core.type.UserDefinedTypeBuilder;
 import com.datastax.oss.driver.internal.core.type.codec.ParseUtils;
 import com.datastax.oss.driver.shaded.guava.common.annotations.VisibleForTesting;
 import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableList;
 import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap;
 import com.datastax.oss.protocol.internal.util.Bytes;
+
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+
 import net.jcip.annotations.ThreadSafe;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -164,6 +168,13 @@ public class DataTypeClassNameParser implements DataTypeParser {
       return new DefaultTupleType(componentTypesBuilder.build(), attachmentPoint);
     }
  
+    if (next.startsWith("org.apache.cassandra.db.marshal.VectorType")) {
+      Iterator<String> rawTypes = parser.getTypeParameters().iterator();
+      DataType subtype = parse(rawTypes.next(), userTypes, attachmentPoint, logPrefix);
+      int dimensions = Integer.parseInt(rawTypes.next());
+      return DataTypes.vectorOf(subtype, dimensions);
+    }
+
     DataType type = NATIVE_TYPES_BY_CLASS_NAME.get(next);
     return type == null ? DataTypes.custom(toParse) : type;
   }

I'd argue this fix (or something very much like it) is much preferred because (a) it keeps the arg parsing logic consolidated in DataTypeClassNameParser (which seems like the ideal place to put it) and (b) it improves DTCNP so that it can be used for additional vector-related functionality in the future.

[absurdfarce]

Very much agree with @joao-r-reis; this is really good work @SiyaoIsHiding! There are a few changes I think need to happen here but I don't think any of them are huge.

[joao-r-reis]

Can't we put the if (isVarSized) here instead? Otherwise there's still a bit of duplication in my mind. Same for encode

Pseudocode:

for (int i = 0; i < cqlType.getDimensions(); ++i) {
    if (isVarSized) {
        // decode elementSize and move input position forward
    }
    if (elementSize > 0) {  // this will always be true if is not varSized so it works for both cases
        // decode element and add it to collection
    } else {
        // add null to collection (maybe call codec with null? Idk what's correct in java driver, looks like in the current version of this PR the codec is being called with "null"
    }
}

[absurdfarce]

Agree with @joao-r-reis, we should be able to simplify this a bit. I was able to get VectorCodecTest passing with the following:

  @Nullable
  @Override
  public CqlVector<SubtypeT> decode(
      @Nullable ByteBuffer bytes, @NonNull ProtocolVersion protocolVersion) {
    if (bytes == null || bytes.remaining() == 0) {
      return null;
    }

    // Upfront check for fixed-size types only
    subtypeCodec.serializedSize().ifPresent((fixed_size) -> {

      if (bytes.remaining() != cqlType.getDimensions() * fixed_size) {
        throw new IllegalArgumentException(
                String.format(
                        "Expected elements of uniform size, observed %d elements with total bytes %d",
                        cqlType.getDimensions(), bytes.remaining()));
      }
    });
            ;
    ByteBuffer slice = bytes.slice();
    List<SubtypeT> rv = new ArrayList<SubtypeT>(cqlType.getDimensions());
    for (int i = 0; i < cqlType.getDimensions(); ++i) {

      int size = subtypeCodec.serializedSize().orElseGet(() -> {
        int vint_size = VIntCoding.getUnsignedVInt32(slice, slice.position());
        // Side effects!  Skip over the bytes that vint encode the size of this element
        slice.position(slice.position() + VIntCoding.computeUnsignedVIntSize(vint_size));
        return vint_size;
      });
      int originalPosition = slice.position();
      slice.limit(originalPosition + size);
      rv.add(this.subtypeCodec.decode(slice, protocolVersion));
      // Move to the start of the next element
      slice.position(originalPosition + size);
      // Reset the limit to the end of the buffer
      slice.limit(slice.capacity());
    }

    // if too many elements, throw
    if (slice.hasRemaining()) {
      throw new IllegalArgumentException(
              String.format(
                      "Too many elements; must provide elements for %d dimensions",
                      cqlType.getDimensions()));
    }

    return CqlVector.newInstance(rv);
  }

I might be missing an error case or two there but I think that covers everything.

I don't love the side effects in the orElseGet() Supplier implementation... but at least they're constrained.

[absurdfarce]

Okay, I can make it at least a bit clearer with this cleaned up (and already formatted!) version:

  @Nullable
  @Override
  public CqlVector<SubtypeT> decode(
      @Nullable ByteBuffer bytes, @NonNull ProtocolVersion protocolVersion) {
    if (bytes == null || bytes.remaining() == 0) {
      return null;
    }

    // Upfront check for fixed-size types only
    subtypeCodec
        .serializedSize()
        .ifPresent(
            (fixed_size) -> {
              if (bytes.remaining() != cqlType.getDimensions() * fixed_size) {
                throw new IllegalArgumentException(
                    String.format(
                        "Expected elements of uniform size, observed %d elements with total bytes %d",
                        cqlType.getDimensions(), bytes.remaining()));
              }
            });
    ;
    ByteBuffer slice = bytes.slice();
    List<SubtypeT> rv = new ArrayList<SubtypeT>(cqlType.getDimensions());
    for (int i = 0; i < cqlType.getDimensions(); ++i) {

      int size =
          subtypeCodec
              .serializedSize()
              .orElseGet(() -> VIntCoding.getUnsignedVInt32(slice, slice.position()));
      // If we aren't dealing with a fixed-size type we need to move the current slice position
      // beyond the vint-encoded size of the current element.  Ideally this would be
      // serializedSize().ifNotPresent(Consumer) but the Optional API isn't doing us any favors
      // there.
      if (!subtypeCodec.serializedSize().isPresent())
        slice.position(slice.position() + VIntCoding.computeUnsignedVIntSize(size));
      int originalPosition = slice.position();
      slice.limit(originalPosition + size);
      rv.add(this.subtypeCodec.decode(slice, protocolVersion));
      // Move to the start of the next element
      slice.position(originalPosition + size);
      // Reset the limit to the end of the buffer
      slice.limit(slice.capacity());
    }

    // if too many elements, throw
    if (slice.hasRemaining()) {
      throw new IllegalArgumentException(
          String.format(
              "Too many elements; must provide elements for %d dimensions",
              cqlType.getDimensions()));
    }

    return CqlVector.newInstance(rv);
  }

[SiyaoIsHiding]

Updated
@absurdfarce

[joao-r-reis]

+1 but CI failure needs to be looked at

[absurdfarce]

That CI failure appears to be from a run that went down on January 14 '25. There's been a lot of changes since then so I kicked off another run just now to make sure we're good (or at least approaching good).

[absurdfarce]

Yup, confirmed; re-run of the Jenkins build for this PR is now green.

[absurdfarce]

One (relatively) minor change on a comment that I don't think applies anymore... and then I think we're there.

[absurdfarce]

Many, many thanks to @SiyaoIsHiding for her excellent work and her persistence in getting this done!

[absurdfarce]

All righty @SiyaoIsHiding, this ticket is finally wrapped up and we're ready to merge it in! Can you squash all the commits down to a single commit and add the "patch by" line?

Really great work on this one!

[SiyaoIsHiding]

Squashed! @absurdfarce

[absurdfarce]

Thanks @SiyaoIsHiding!