SchemaObject changes to support multiple vector configs #1499

maheshrajamani · 2024-10-05T03:14:40Z

What this PR does:
Made the SchemaObject store and return List of VectorConfig. This is needed because tables need to support multiple vector config.
Changed the vectorize serialization to be based on VectorizeConfig object used in SchemaObject.

Checklist

Changes manually tested
Automated Tests added/updated
Documentation added/updated
CLA Signed: DataStax CLA

…needed because tables need multiple vector config.

…ormation

…TOR_EMBEDDING_TEXT_FIELD`

maheshrajamani · 2024-10-05T03:21:09Z

src/main/java/io/stargate/sgv2/jsonapi/api/v1/CollectionResource.java

@@ -198,15 +198,15 @@ public Uni<RestResponse<CommandResult>> postCommand(
                // TODO: refactor this code to be cleaner so it assigns on one line
                EmbeddingProvider embeddingProvider = null;
                final VectorConfig.VectorizeConfig vectorizeConfig =
-                    schemaObject.vectorConfig().vectorizeConfig();
+                    schemaObject.vectorConfigs().get(0).vectorizeConfig();


This is changed to get(0) because collections have one config. Will need to change this when we work on vectorize support for tables.

maheshrajamani · 2024-10-07T12:56:47Z

src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateTableCommandResolver.java

@@ -39,7 +40,7 @@ public Operation resolveKeyspaceCommand(
            .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getApiDataType()));
    List<String> partitionKeys = Arrays.stream(command.definition().primaryKey().keys()).toList();

-    Map<String, VectorizeConfig> vectorizeConfigMap =
+    Map<String, VectorConfig.VectorizeConfig> vectorizeConfigMap =


Changed the serialization to use the object used in schema cache

tatu-at-datastax

Ok, so, I understand that we will need to allow multiple vector definitions for API Tables (and possibly eventually for Collections). But I am bit worried about couple of aspects:

Use of "vector-disabled" value for List<VectorConfig> instead of main-level enabled/disabled -- along with special VectorConfig value
Ordering of VectorConfig within List as it's not clear how ordering would be defined/preserved for multi-vector use case (which we don't yet have). Should we use JSON Object instead of JSON Array, keyed by column name?

I think there are good reasons to do above but would be good to outline reasoning.

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java

tatu-at-datastax · 2024-10-07T15:27:25Z

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java

  }

-  // convert a vector jsonNode from table comment to vectorConfig
+  // convert a vector jsonNode from table comment to vectorConfig, used for collection


... not used for tables? So only collection? (if so, comment should state that)

I guess it means "used when reading config for Collections (not Tables)"

The table there denotes cql table. fwiw I will just say comment to avoid confusion

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java

tatu-at-datastax · 2024-10-07T15:30:55Z

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java

@@ -51,17 +81,8 @@ public static VectorConfig fromJson(JsonNode jsonNode, ObjectMapper objectMapper
          vectorizeServiceParameterNode == null
              ? null


Could use Map.of() unless we want to use null as marker of some kind.

Didn't understand this recommendation.

tatu-at-datastax · 2024-10-07T15:36:39Z

src/main/java/io/stargate/sgv2/jsonapi/service/embedding/DataVectorizer.java

@@ -49,6 +51,9 @@ public DataVectorizer(
    this.nodeFactory = nodeFactory;
    this.embeddingCredentials = embeddingCredentials;
    this.schemaObject = schemaObject;
+    // This is getting element at 0 since only one vector is stored in the schema.
+    // This logic needs to be changed to handle multiple vectors columns for tables,
+    vectorConfig = schemaObject.vectorConfigs().get(0);


Should class Javadoc probably mention if it is only being used for Collections?
I assume it is because only single vector is defined as per comment here.

This class has all field names hard coded for collections, this needs to be adapted for api tables.

tatu-at-datastax · 2024-10-07T15:47:20Z

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java

    int vectorSize,
    SimilarityFunction similarityFunction,
    VectorizeConfig vectorizeConfig) {

  // TODO: this is an immutable record, this can be singleton
  // TODO: Remove the use of NULL for the objects like vectorizeConfig
  public static VectorConfig notEnabledVectorConfig() {
-    return new VectorConfig(false, -1, null, null);
+    return new VectorConfig(false, null, -1, null, null);


What are the semantics of null vectorField? I assume we need a placeholder of some kind... ?
Put another way, how are we using this placeholder (how is it checked and so on).

Gave null if the there is no vector field set for the table or collection

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java

maheshrajamani · 2024-10-07T18:26:09Z

Use of "vector-disabled" value for List<VectorConfig> instead of main-level enabled/disabled -- along with special VectorConfig value

Refactored this to keep the flag outside vector definitions.

2. Ordering of VectorConfig within List as it's not clear how ordering would be defined/preserved for multi-vector use case (which we don't yet have). Should we use JSON Object instead of JSON Array, keyed by column name?

We don't need to maintain ordering for the vectorize config, since they have the column name stored with it, in case of collection field name will be $vectorize

…ct-multiple-vectorize

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java

tatu-at-datastax · 2024-10-07T19:04:10Z

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java

+                  entry.getValue(), VectorConfig.ColumnVectorDefinition.VectorizeConfig.class);
+          vectorizeConfigMap.put(entry.getKey(), vectorizeConfig);
+        }
+      } catch (JsonProcessingException | IllegalArgumentException e) {


Ok, one last request: was hoping for separate catch blocks -- so for readTree() generic "Bad JSON" (because that'd be causing it) -- and then for treeToValue() can indicate specific vector field that was bad.

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java

tatu-at-datastax · 2024-10-07T19:05:57Z

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java

-    SimilarityFunction similarityFunction =
-        SimilarityFunction.fromString(jsonNode.get("metric").asText());
+  /**
+   * Configuration for a column, In case of collection this will be of size one


nit: "size one"? Does it mean there will just be one such definition or ... ?

Currently collection has only one vector field and its hard coded to $vectorize in the application. These code need to be refactored when we do table vectorize support. Added the comment since I did get(0)

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/NamespaceCache.java

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java

tatu-at-datastax

Ok looks better -- added some optional suggestions, approved.

…te/data-api into schema-outject-multiple-vectorize

Made the SchemaObject store and return List of VectorConfig. This is …

42d0fb5

…needed because tables need multiple vector config.

maheshrajamani requested a review from a team as a code owner October 5, 2024 03:14

maheshrajamani added 7 commits October 6, 2024 22:36

Changes for TableSchemaObject to deserialize vector and vectorize inf…

c016640

…ormation

Changed the vector config to unmodifiable list

d590679

Fixed the comments

aa656ef

Fixed the comments

02bc38e

Use $vectorize field name from constant `DocumentConstants.Fields.VEC…

24da975

…TOR_EMBEDDING_TEXT_FIELD`

Use $vectorize field name from constant `DocumentConstants.Fields.VEC…

ecabca3

…TOR_EMBEDDING_TEXT_FIELD`

Use $vectorize field name from constant `DocumentConstants.Fields.VEC…

bc30acb

…TOR_EMBEDDING_TEXT_FIELD`

maheshrajamani commented Oct 7, 2024

View reviewed changes

tatu-at-datastax reviewed Oct 7, 2024

View reviewed changes

maheshrajamani added 2 commits October 7, 2024 14:22

Updated the code based on review.

550ce05

Merge branch 'main' into schema-outject-multiple-vectorize

0a24bd0

maheshrajamani requested a review from tatu-at-datastax October 7, 2024 18:27

Merge branch 'main' of github.com:stargate/data-api into schema-outje…

5220f57

…ct-multiple-vectorize

tatu-at-datastax reviewed Oct 7, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java Show resolved Hide resolved

tatu-at-datastax reviewed Oct 7, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java Show resolved Hide resolved

tatu-at-datastax reviewed Oct 7, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/VectorConfig.java Outdated Show resolved Hide resolved

Fixed the vectorize config deserializer

2683c66

tatu-at-datastax reviewed Oct 7, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/NamespaceCache.java Outdated Show resolved Hide resolved

tatu-at-datastax reviewed Oct 7, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java Outdated Show resolved Hide resolved

tatu-at-datastax approved these changes Oct 7, 2024

View reviewed changes

maheshrajamani added 5 commits October 7, 2024 16:16

Changes based on review

e7e92cb

Merge branch 'schema-outject-multiple-vectorize' of github.com:starga…

78ba059

…te/data-api into schema-outject-multiple-vectorize

Merge branch 'main' into schema-outject-multiple-vectorize

366a503

Resolve the merge compile error

27f3bef

Resolve the merge compile error

11b589e

maheshrajamani added 3 commits October 7, 2024 16:33

Resolve the merge compile error

6ad43dc

Formatted the file

dd2f2cb

Fix for IT

5e45661

maheshrajamani merged commit 647012c into main Oct 7, 2024
3 checks passed

maheshrajamani deleted the schema-outject-multiple-vectorize branch October 7, 2024 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SchemaObject changes to support multiple vector configs #1499

SchemaObject changes to support multiple vector configs #1499

maheshrajamani commented Oct 5, 2024 •

edited

Loading

maheshrajamani Oct 5, 2024

maheshrajamani Oct 7, 2024

tatu-at-datastax left a comment

tatu-at-datastax Oct 7, 2024

tatu-at-datastax Oct 7, 2024

maheshrajamani Oct 7, 2024

tatu-at-datastax Oct 7, 2024

maheshrajamani Oct 7, 2024

tatu-at-datastax Oct 7, 2024

maheshrajamani Oct 7, 2024

tatu-at-datastax Oct 7, 2024

maheshrajamani Oct 7, 2024

maheshrajamani commented Oct 7, 2024

tatu-at-datastax Oct 7, 2024

tatu-at-datastax Oct 7, 2024

maheshrajamani Oct 7, 2024

tatu-at-datastax left a comment

		@@ -51,17 +81,8 @@ public static VectorConfig fromJson(JsonNode jsonNode, ObjectMapper objectMapper
		vectorizeServiceParameterNode == null
		? null

SchemaObject changes to support multiple vector configs #1499

SchemaObject changes to support multiple vector configs #1499

Conversation

maheshrajamani commented Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatu-at-datastax 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

Choose a reason for hiding this comment

maheshrajamani commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatu-at-datastax left a comment

Choose a reason for hiding this comment

maheshrajamani commented Oct 5, 2024 •

edited

Loading