-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Draft] Implement vector field type #1899
base: main
Are you sure you want to change the base?
Conversation
🤖 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 -- conventional-commit-lint bot |
ad58cc1
to
36a50cb
Compare
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://github.com/googleapis/synthtool/commit/2f0fea69b6ae56dba965ea8817750793862b3b2d Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:37f2f91dea317c75ebf4e19880aa8f10b2228b8d07859c0f384dbcf660735ba2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with suggestions
dev/src/field-value.ts
Outdated
} | ||
|
||
public toArray(): number[] { | ||
return this.values; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider returning a copy of the internal data so the values array cannot be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dev/src/field-value.ts
Outdated
export class VectorValue implements firestore.VectorValue { | ||
private readonly values: number[]; | ||
constructor(values: number[] | undefined) { | ||
this.values = values || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making a copy of the argument so the internal values
array cannot be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dev/src/index.ts
Outdated
'client', | ||
'init', | ||
`${JSON.stringify((client as any)._opts.servicePath)}` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to leave this logger statement here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like something you may have used for debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Deleted.
dev/src/serializer.ts
Outdated
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "RESERVED_VECTOR_MAP_VECTORS_KEY" may not need the "RESERVED_" prefix since the value is not surrounded with "__"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dev/src/field-value.ts
Outdated
return serializer.encodeVector({ | ||
arrayValue: { | ||
values: this.values.map(value => { | ||
return { | ||
doubleValue: value, | ||
}; | ||
}), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or move the whole implementation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dev/system-test/firestore.ts
Outdated
@@ -62,6 +62,8 @@ use(chaiAsPromised); | |||
|
|||
const version = require('../../package.json').version; | |||
|
|||
setLogFunction(console.log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this statement is in the PR intentionally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another debug line. Deleted.
dev/system-test/firestore.ts
Outdated
@@ -104,6 +106,8 @@ function getTestRoot(settings: Settings = {}): CollectionReference { | |||
internalSettings.databaseId = process.env.FIRESTORE_NAMED_DATABASE; | |||
} | |||
|
|||
settings.host = 'test-firestore.sandbox.googleapis.com'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted.
dev/src/field-value.ts
Outdated
@@ -30,6 +30,49 @@ import { | |||
|
|||
import api = proto.google.firestore.v1; | |||
|
|||
export class VectorValue implements firestore.VectorValue { | |||
private readonly values: number[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider prefixing the variable with underscore to discourage use by JS developers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
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])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using VectorValue.isEqual
instead of to.deep.equal
because to.deep.equal
relies on equality testing using private members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to change this in the other PR to save myself from conflict resolving, hopefully that is OK with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for the review!
dev/src/field-value.ts
Outdated
@@ -30,6 +30,49 @@ import { | |||
|
|||
import api = proto.google.firestore.v1; | |||
|
|||
export class VectorValue implements firestore.VectorValue { | |||
private readonly values: number[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dev/src/field-value.ts
Outdated
export class VectorValue implements firestore.VectorValue { | ||
private readonly values: number[]; | ||
constructor(values: number[] | undefined) { | ||
this.values = values || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dev/src/field-value.ts
Outdated
} | ||
|
||
public toArray(): number[] { | ||
return this.values; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dev/src/field-value.ts
Outdated
return serializer.encodeVector({ | ||
arrayValue: { | ||
values: this.values.map(value => { | ||
return { | ||
doubleValue: value, | ||
}; | ||
}), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dev/src/index.ts
Outdated
'client', | ||
'init', | ||
`${JSON.stringify((client as any)._opts.servicePath)}` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Deleted.
dev/src/serializer.ts
Outdated
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dev/system-test/firestore.ts
Outdated
@@ -62,6 +62,8 @@ use(chaiAsPromised); | |||
|
|||
const version = require('../../package.json').version; | |||
|
|||
setLogFunction(console.log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another debug line. Deleted.
dev/system-test/firestore.ts
Outdated
@@ -104,6 +106,8 @@ function getTestRoot(settings: Settings = {}): CollectionReference { | |||
internalSettings.databaseId = process.env.FIRESTORE_NAMED_DATABASE; | |||
} | |||
|
|||
settings.host = 'test-firestore.sandbox.googleapis.com'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted.
|
||
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])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to change this in the other PR to save myself from conflict resolving, hopefully that is OK with you.
No description provided.