vectorization on demand #1258

Yuqi-Du · 2024-07-10T21:04:03Z

For these update commands: findOneAndUpdate, updateOne, updateMany, findOneAndReplace
We need to vectorize the update clause or replacement document as needed, that is when:

we have document returned by find operation.
OR
upsert, will vectorize no matter what

Since the pre-requisite for vectorization on demand is findOperation's execution., So we can not continue to do vectorization for these four update commands at commandResolver level.Postpone the vectorization in ReadAndUpdateOperation, utilize the documentUpdater, only vectorize as needed.

As you can see, in the DataVectorizer, there will be no more vectorizeUpdateClause, since we postpone the vectorization for updateCommand in operation level. The new refactored documentUpdater will return a updateResponse that contains a possible UpdateEmbeddingOperation. And we can update the $vector after apply vectorization to $vectorize.

Checklist

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

# Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/FindOneAndReplaceCommandResolver.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/FindOneAndUpdateCommandResolver.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/UpdateManyCommandResolver.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/UpdateOneCommandResolver.java

…pdate-vectorize

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

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

...a/io/stargate/sgv2/jsonapi/service/resolver/model/impl/FindOneAndReplaceCommandResolver.java

# Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/embedding/DataVectorizerService.java # src/test/java/io/stargate/sgv2/jsonapi/service/embedding/operation/DataVectorizerTest.java # src/test/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/CommandResolverWithVectorizerTest.java # src/test/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/UpdateManyCommandResolverTest.java

vkarpov15

LGTM 👍

# Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/embedding/DataVectorizer.java # src/main/java/io/stargate/sgv2/jsonapi/service/embedding/DataVectorizerService.java # src/test/java/io/stargate/sgv2/jsonapi/service/embedding/operation/DataVectorizerTest.java # src/test/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/CommandResolverWithVectorizerTest.java # src/test/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/UpdateManyCommandResolverTest.java

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/SetOperation.java

amorton

went through, couple of improvements but also 2 bugs

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/UpdateOperation.java

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/SetOperation.java

src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/SetOperation.java

src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java

src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java

# Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/embedding/DataVectorizer.java # src/main/java/io/stargate/sgv2/jsonapi/service/embedding/DataVectorizerService.java # src/main/java/io/stargate/sgv2/jsonapi/service/operation/collections/ReadAndUpdateCollectionOperation.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/FindOneAndReplaceCommandResolver.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/FindOneAndUpdateCommandResolver.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/UpdateManyCommandResolver.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/UpdateOneCommandResolver.java # src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/ReadAndUpdateCollectionOperationRetryTest.java # src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/ReadAndUpdateCollectionOperationTest.java # src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/SerialConsistencyOverrideOperationTest.java # src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CommandResolverWithVectorizerTest.java # src/test/java/io/stargate/sgv2/jsonapi/service/resolver/FindOneAndReplaceCommandResolverTest.java # src/test/java/io/stargate/sgv2/jsonapi/service/resolver/UpdateManyCommandResolverTest.java

maheshrajamani · 2024-07-25T02:48:37Z

src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java

+          dataVectorizerService.constructDataVectorizer(dataApiRequestInfo, commandContext);
+      // TODO: only SetOperation and Replacement may create one embeddingUpdateOperation, Refactor
+      // when there are multiple
+      final EmbeddingUpdateOperation embeddingUpdateOperation = embeddingUpdateOperations.get(0);


Why pick it as get(0) when we have a list? I understand we support only one vector currently. Would be better to use Multi and merge here, so we don't have to worry about this code in case of multiple vectorize

maheshrajamani · 2024-07-25T02:59:19Z

src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java

+      // when there are multiple
+      final EmbeddingUpdateOperation embeddingUpdateOperation = embeddingUpdateOperations.get(0);
+      return dataVectorizer
+          .vectorize(embeddingUpdateOperation.vectorizeContent())


Looks like this method is called multiple times if the update needs to update on multiple documents in case of UpdateMany. Let's lazy cache the vector for inside the EmbeddingUpdateOperation.

This part Aaron and I discussed, and decide not do the cache here, possibly another improvemtn
understand that what you did in DataVectorizer previously can avoid this problem

Will create a ticket.

...io/stargate/sgv2/jsonapi/service/operation/collections/ReadAndUpdateCollectionOperation.java

maheshrajamani · 2024-07-25T03:03:36Z

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/UpdateOperation.java

@@ -110,4 +110,7 @@ public int compare(ActionWithLocator o1, ActionWithLocator o2) {
      return o1.path().compareTo(o2.path());
    }
  }
+
+  public record UpdateOperationResult(


maheshrajamani · 2024-07-25T03:04:07Z

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/UpdateOperation.java

@@ -30,7 +30,7 @@ public List<A> actions() {
   * @param doc Document to apply operation to
   * @return True if document was modified by operation; false if not.


Update the java doc here.

maheshrajamani · 2024-07-25T03:06:50Z

src/test/java/io/stargate/sgv2/jsonapi/service/resolver/UpdateManyCommandResolverTest.java

@@ -227,77 +223,6 @@ public void dynamicFilterCondition() throws Exception {
              });
    }

-    @Test
-    public void dynamicFilterConditionSetVectorize() throws Exception {


Instead of removing the test can we modify it to show how the data would look like? Will be helpful when changes are done around it.

I have tests in the DocumentUpdaterTest class. Should be clear to see.
Reason of deleting this one, is because we no longer do updateClause vectorize.
Instead, we vectorize the document if found. Here in UpdateManyCommandResolverTest.java, we don't actually mimic document returned by DB.

maheshrajamani · 2024-07-25T03:07:57Z

src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CommandResolverWithVectorizerTest.java

@@ -420,183 +413,6 @@ public void findOneAndDelete() throws Exception {
              });
    }

-    @Test
-    public void findOneAndReplace() throws Exception {


Instead of removing can we change the test to show how the object look like when $vectorize is used?

Same as above.

# Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/embedding/DataVectorizer.java # src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java

Yuqi-Du added 17 commits June 14, 2024 09:51

init

4581baa

fix

77415ad

Merge remote-tracking branch 'origin/main' into fix-update-vectorize

5fecd64

fix

8a99d28

Merge remote-tracking branch 'origin/main' into fix-update-vectorize

94ded0e

added logic to check if there is $vectorize text diff

7d27b2a

delete two vectorize update unit tests

e961200

Merge branch 'main' into fix-update-vectorize

3e681ba

Merge branch 'main' into fix-update-vectorize

9a5aafc

Merge branch 'main' into fix-update-vectorize

3f332bc

Merge remote-tracking branch 'origin/main' into fix-update-vectorize

a35e328

refactor

5073fc2

Merge remote-tracking branch 'origin/fix-update-vectorize' into fix-u…

f50d64a

…pdate-vectorize

Merge remote-tracking branch 'origin/main' into fix-update-vectorize

1cb614d

fix

b1b5ad7

Merge remote-tracking branch 'origin/main' into fix-update-vectorize

95ed7d9

Yuqi-Du requested a review from a team as a code owner July 10, 2024 21:04

fix

62ce663

Yuqi-Du changed the title ~~Fix update vectorize~~ vectorization on demand Jul 10, 2024

Yuqi-Du commented Jul 10, 2024

View reviewed changes

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

Yuqi-Du commented Jul 10, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/embedding/DataVectorizerService.java Outdated Show resolved Hide resolved

Yuqi-Du commented Jul 10, 2024

View reviewed changes

...a/io/stargate/sgv2/jsonapi/service/resolver/model/impl/FindOneAndReplaceCommandResolver.java Outdated Show resolved Hide resolved

Yuqi-Du added 5 commits July 10, 2024 14:33

add unset test

4f1d47c

merge from main

930dc2e

fix IT

a65880c

vkarpov15 reviewed Jul 11, 2024

View reviewed changes

Yuqi-Du marked this pull request as draft July 12, 2024 17:51

Yuqi-Du added 4 commits July 15, 2024 17:12

Merge remote-tracking branch 'origin/main' into fix-update-vectorize

d51ff20

refactor

2e3e7f6

Merge branch 'main' into fix-update-vectorize

f59ed44

Yuqi-Du commented Jul 16, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/SetOperation.java Show resolved Hide resolved

fix

fc5b0fc

amorton reviewed Jul 19, 2024

View reviewed changes

Yuqi-Du added 7 commits July 22, 2024 10:33

Merge remote-tracking branch 'origin/main' into fix-update-vectorize

55f1c25

fix comments

7d3e0df

fix comments

2eec55b

fix IT

f2f6a21

merged from tables, fix conflicts

577703f

Merge branch 'main' into fix-update-vectorize

3e4fe40

Yuqi-Du marked this pull request as ready for review July 24, 2024 00:34

maheshrajamani reviewed Jul 25, 2024

View reviewed changes

Yuqi-Du added 2 commits July 26, 2024 12:56

Merge remote-tracking branch 'origin/main' into fix-update-vectorize

247742c

# Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/embedding/DataVectorizer.java # src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java

fix comments

ca9d04f

maheshrajamani approved these changes Jul 29, 2024

View reviewed changes

Yuqi-Du merged commit 6b62bc6 into main Jul 29, 2024
3 checks passed

Yuqi-Du deleted the fix-update-vectorize branch July 29, 2024 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vectorization on demand #1258

vectorization on demand #1258

Yuqi-Du commented Jul 10, 2024 •

edited

Loading

vkarpov15 left a comment

amorton left a comment

maheshrajamani Jul 25, 2024

Yuqi-Du Jul 26, 2024

maheshrajamani Jul 25, 2024

Yuqi-Du Jul 26, 2024

Yuqi-Du Jul 29, 2024

Yuqi-Du Jul 29, 2024

maheshrajamani Jul 25, 2024

Yuqi-Du Jul 26, 2024

maheshrajamani Jul 25, 2024

Yuqi-Du Jul 26, 2024

maheshrajamani Jul 25, 2024

Yuqi-Du Jul 26, 2024

maheshrajamani Jul 25, 2024

Yuqi-Du Jul 26, 2024

		@@ -30,7 +30,7 @@ public List<A> actions() {
		* @param doc Document to apply operation to
		* @return True if document was modified by operation; false if not.

vectorization on demand #1258

vectorization on demand #1258

Conversation

Yuqi-Du commented Jul 10, 2024 • edited Loading

vkarpov15 left a comment

Choose a reason for hiding this comment

amorton 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

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

Yuqi-Du commented Jul 10, 2024 •

edited

Loading