[Draft] Implement vector field type by wu-hui · Pull Request #1899 · googleapis/nodejs-firestore

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

+  }
+
+  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

+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

+          '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


+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

+    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


 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

    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

 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

+
+    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

 import api = proto.google.firestore.v1;

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


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

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


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

+  }
+
+  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

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


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

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


Yeah. Deleted.

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


+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


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

+setLogFunction(console.log);


Another debug line. Deleted.

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

    internalSettings.databaseId = process.env.FIRESTORE_NAMED_DATABASE;
  }

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


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

+
+    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 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

MarkDuckworth closed this Oct 9, 2024


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

		setLogFunction(console.log);

Conversation

wu-hui commented Sep 15, 2023

Uh oh!

conventional-commit-lint-gcf Bot commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarkDuckworth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

edited

Loading