[Draft] Implement vector field type #1899

wu-hui · 2023-09-15T14:22:46Z

No description provided.

conventional-commit-lint-gcf · 2023-09-15T14:22:51Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

Revert "chore: remove compile protos from post processor (#1899)" This reverts commit 73248044166b6ba2dd102862f8cd2c4829e868db. Source-Link: googleapis/synthtool@2f0fea6 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:37f2f91dea317c75ebf4e19880aa8f10b2228b8d07859c0f384dbcf660735ba2

Revert "chore: remove compile protos from post processor (#1899)" This reverts commit 73248044166b6ba2dd102862f8cd2c4829e868db. Source-Link: https://togithub.com/googleapis/synthtool/commit/2f0fea69b6ae56dba965ea8817750793862b3b2d Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:37f2f91dea317c75ebf4e19880aa8f10b2228b8d07859c0f384dbcf660735ba2

MarkDuckworth

LGTM with suggestions

MarkDuckworth · 2024-01-17T18:32:36Z

dev/src/field-value.ts

+  }
+
+  public toArray(): number[] {
+    return this.values;


Consider returning a copy of the internal data so the values array cannot be modified.

Done.

This is usually a good suggestion, I am wondering if it is as good with vectors, given the size and the likelihood of people modifying vectors.

Nevertheless, I am OK with making copies for now, and later optimize.

MarkDuckworth · 2024-01-17T18:33:11Z

dev/src/field-value.ts

+export class VectorValue implements firestore.VectorValue {
+  private readonly values: number[];
+  constructor(values: number[] | undefined) {
+    this.values = values || [];


Consider making a copy of the argument so the internal values array cannot be modified.

MarkDuckworth · 2024-01-17T19:14:16Z

dev/src/index.ts

+          'client',
+          'init',
+          `${JSON.stringify((client as any)._opts.servicePath)}`
+        );


Is it intentional to leave this logger statement here?

It looks like something you may have used for debugging.

Yeah. Deleted.

MarkDuckworth · 2024-01-17T19:46:35Z

dev/src/serializer.ts

@@ -38,6 +38,10 @@ import api = proto.google.firestore.v1;
 */
 const MAX_DEPTH = 20;

+const RESERVED_MAP_KEY = '__type__';
+const RESERVED_MAP_KEY_VECTOR_VALUE = '__vector__';
+const RESERVED_VECTOR_MAP_VECTORS_KEY = 'value';


nit: "RESERVED_VECTOR_MAP_VECTORS_KEY" may not need the "RESERVED_" prefix since the value is not surrounded with "__"

MarkDuckworth · 2024-01-17T20:05:55Z

dev/src/field-value.ts

+    return serializer.encodeVector({
+      arrayValue: {
+        values: this.values.map(value => {
+          return {
+            doubleValue: value,
+          };
+        }),
+      },


nit: it's a little challenging to read this proto serialization since it is spread across this fie and the Serializer. Would it be cleaner to move the array serialization into encodeVector so it's all in one place?

Or move the whole implementation here?

MarkDuckworth · 2024-01-17T20:09:46Z

dev/system-test/firestore.ts

@@ -62,6 +62,8 @@ use(chaiAsPromised);

 const version = require('../../package.json').version;

+setLogFunction(console.log);


not sure if this statement is in the PR intentionally

Another debug line. Deleted.

MarkDuckworth · 2024-01-17T20:14:19Z

dev/system-test/firestore.ts

@@ -104,6 +106,8 @@ function getTestRoot(settings: Settings = {}): CollectionReference {
    internalSettings.databaseId = process.env.FIRESTORE_NAMED_DATABASE;
  }

+  settings.host = 'test-firestore.sandbox.googleapis.com';


FWIW. You can also set environment variable FIRESTORE_TARGET_BACKEND=nightly to run against nightly. I set up multiple IntelliJ run configs, so easily switch back and forth.

I think this statement needs to be removed before committing.

MarkDuckworth · 2024-01-17T20:50:13Z

dev/src/field-value.ts

@@ -30,6 +30,49 @@ import {

 import api = proto.google.firestore.v1;

+export class VectorValue implements firestore.VectorValue {
+  private readonly values: number[];


Consider prefixing the variable with underscore to discourage use by JS developers

MarkDuckworth · 2024-01-17T21:15:38Z

dev/system-test/firestore.ts

+
+    const snap1 = await ref.get();
+    expect(snap1.get('vectorEmpty')).to.deep.equal(FieldValue.vector());
+    expect(snap1.get('vector1')).to.deep.equal(FieldValue.vector([1, 2, 3.99]));


Consider using VectorValue.isEqual instead of to.deep.equal because to.deep.equal relies on equality testing using private members.

I want to change this in the other PR to save myself from conflict resolving, hopefully that is OK with you.

wu-hui

Thank for the review!

wu-hui · 2024-01-23T15:47:58Z

dev/src/field-value.ts

@@ -30,6 +30,49 @@ import {

 import api = proto.google.firestore.v1;

+export class VectorValue implements firestore.VectorValue {
+  private readonly values: number[];


wu-hui · 2024-01-23T15:49:30Z

dev/src/field-value.ts

+export class VectorValue implements firestore.VectorValue {
+  private readonly values: number[];
+  constructor(values: number[] | undefined) {
+    this.values = values || [];


wu-hui · 2024-01-23T15:52:45Z

dev/src/field-value.ts

+  }
+
+  public toArray(): number[] {
+    return this.values;


Done.

This is usually a good suggestion, I am wondering if it is as good with vectors, given the size and the likelihood of people modifying vectors.

Nevertheless, I am OK with making copies for now, and later optimize.

wu-hui · 2024-01-23T15:58:06Z

dev/src/field-value.ts

+    return serializer.encodeVector({
+      arrayValue: {
+        values: this.values.map(value => {
+          return {
+            doubleValue: value,
+          };
+        }),
+      },


wu-hui · 2024-01-23T15:58:47Z

dev/src/index.ts

+          'client',
+          'init',
+          `${JSON.stringify((client as any)._opts.servicePath)}`
+        );


Yeah. Deleted.

wu-hui · 2024-01-23T15:59:42Z

dev/src/serializer.ts

@@ -38,6 +38,10 @@ import api = proto.google.firestore.v1;
 */
 const MAX_DEPTH = 20;

+const RESERVED_MAP_KEY = '__type__';
+const RESERVED_MAP_KEY_VECTOR_VALUE = '__vector__';
+const RESERVED_VECTOR_MAP_VECTORS_KEY = 'value';


wu-hui · 2024-01-23T16:00:13Z

dev/system-test/firestore.ts

@@ -62,6 +62,8 @@ use(chaiAsPromised);

 const version = require('../../package.json').version;

+setLogFunction(console.log);


Another debug line. Deleted.

wu-hui · 2024-01-23T16:00:40Z

dev/system-test/firestore.ts

@@ -104,6 +106,8 @@ function getTestRoot(settings: Settings = {}): CollectionReference {
    internalSettings.databaseId = process.env.FIRESTORE_NAMED_DATABASE;
  }

+  settings.host = 'test-firestore.sandbox.googleapis.com';


wu-hui · 2024-01-23T16:02:57Z

dev/system-test/firestore.ts

+
+    const snap1 = await ref.get();
+    expect(snap1.get('vectorEmpty')).to.deep.equal(FieldValue.vector());
+    expect(snap1.get('vector1')).to.deep.equal(FieldValue.vector([1, 2, 3.99]));


I want to change this in the other PR to save myself from conflict resolving, hopefully that is OK with you.

wu-hui requested review from a team as code owners September 15, 2023 14:22

product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Sep 15, 2023

Implement vector field type

36a50cb

wu-hui force-pushed the wuandy/VectorType branch from ad58cc1 to 36a50cb Compare September 15, 2023 14:46

wu-hui and others added 3 commits September 15, 2023 10:48

Merge branch 'main' into wuandy/VectorType

415c94e

test

3551acb

Merge branch 'main' into wuandy/VectorType

248af30

gcf-owl-bot bot mentioned this pull request Nov 28, 2023

Revert "chore: remove compile protos from post processor" #1951

Merged

wu-hui added 2 commits November 29, 2023 11:42

Added extra logs and vector type test

6042c4d

Add vector watch test

764800b

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Dec 5, 2023

Add vectorvalue to dts.

93ae1d7

wu-hui requested a review from MarkDuckworth January 17, 2024 16:41

wu-hui assigned MarkDuckworth Jan 17, 2024

MarkDuckworth approved these changes Jan 17, 2024

View reviewed changes

Address Mark's comments.

2f1b964

wu-hui commented Jan 23, 2024

View reviewed changes

Merge branch 'main' into wuandy/VectorType

9ebd42a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Implement vector field type #1899

[Draft] Implement vector field type #1899

wu-hui commented Sep 15, 2023

conventional-commit-lint-gcf bot commented Sep 15, 2023 •

edited

MarkDuckworth left a comment

MarkDuckworth Jan 17, 2024

wu-hui Jan 23, 2024

MarkDuckworth Jan 17, 2024

wu-hui Jan 23, 2024

MarkDuckworth Jan 17, 2024

MarkDuckworth Jan 17, 2024

wu-hui Jan 23, 2024

MarkDuckworth Jan 17, 2024

wu-hui Jan 23, 2024

MarkDuckworth Jan 17, 2024

MarkDuckworth Jan 17, 2024

wu-hui Jan 23, 2024

MarkDuckworth Jan 17, 2024

wu-hui Jan 23, 2024

MarkDuckworth Jan 17, 2024

wu-hui Jan 23, 2024

MarkDuckworth Jan 17, 2024

wu-hui Jan 23, 2024

MarkDuckworth Jan 17, 2024

wu-hui Jan 23, 2024

wu-hui left a comment

wu-hui Jan 23, 2024

wu-hui Jan 23, 2024

wu-hui Jan 23, 2024

wu-hui Jan 23, 2024

wu-hui Jan 23, 2024

wu-hui Jan 23, 2024

wu-hui Jan 23, 2024

wu-hui Jan 23, 2024

wu-hui Jan 23, 2024

		@@ -62,6 +62,8 @@ use(chaiAsPromised);

		const version = require('../../package.json').version;

		setLogFunction(console.log);

[Draft] Implement vector field type #1899

Are you sure you want to change the base?

[Draft] Implement vector field type #1899

Conversation

wu-hui commented Sep 15, 2023

conventional-commit-lint-gcf bot commented Sep 15, 2023 • edited

MarkDuckworth 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

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

wu-hui 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

conventional-commit-lint-gcf bot commented Sep 15, 2023 •

edited