[feat] ppl #2094

wu-hui · 2024-07-30T16:26:24Z

No description provided.

MarkDuckworth · 2024-08-16T22:57:06Z

dev/system-test/pipeline.ts

+  it('arrayContainsAll works', async () => {
+    const results = await randomCol
+      .pipeline()
+      .where(Field.of('tags').arrayContainsAll('adventure', 'magic'))


I wish this read .where(field('tags').arrayContains... or even .where(Field.named('tags').arrayContains.... As I read through the code, I continue to read it as "where field of something" but the meaning is "where field something".

I think for Java Andy added an function overloads that covers the "common" usecase -- which is the case that the first parameter is a string specifying the name of the field. Is that possible to do for TS?

Also, it adds confusion when reading one line
.where(arrayContains('tags', 'comedy')). The affected field is the argument.

And then reading another line
Field.of('tags').arrayContainsAll('adventure', 'magic'). The affected field is this.

I'm assuming you're only switching around for testing completeness, but also I'm just making sure we need to support both patterns?

Field.of is a Java convention, i am not opposed to make it Field.named() or something else. I think we need more inputs.

MarkDuckworth

Just starting review. Will resume next week, but providing some thoughts now.

MarkDuckworth · 2024-08-16T23:05:05Z

dev/system-test/pipeline.ts

+    const results = await randomCol
+      .pipeline()
+      .select(
+        Field.of('tags').arrayConcat('newTag1', 'newTag2').as('modifiedTags')


Wondering if arrayConcat(['newTag1', 'newTag2']) would be better since it is the concatenation of multiple arrays.

MarkDuckworth · 2024-08-16T23:09:22Z

dev/system-test/pipeline.ts

+      .select(
+        arrayFilter(Field.of('tags'), arrayElement().eq('comedy')).as(
+          'filteredTags'
+        )


Wondering if we can have the array filter syntax mirror the query filter syntax.

Field.of('tags').arrayFilter().where(eq(arrayElement(), 'comedy')))

This can be written as:
Field.of('tags')
.arrayFilter(arrayElement().eq('comedy'))
.as('filteredTags')

which is not bad?

MarkDuckworth · 2024-08-16T23:11:59Z

dev/system-test/pipeline.ts

+        arrayTransform(
+          Field.of('tags'),
+          arrayElement().strConcat('transformed')
+        ).as('transformedTags')


Same, except mirror query select syntax.

Field.of('tags').arrayTransform().select(arrayElement().strConcat('transformed'))

Same, it looks decent after i rewrite it with fluent builder.

MarkDuckworth · 2024-08-16T23:36:03Z

dev/system-test/pipeline.ts

+  it('arrayContainsAll works', async () => {
+    const results = await randomCol
+      .pipeline()
+      .where(Field.of('tags').arrayContainsAll('adventure', 'magic'))


Also, it adds confusion when reading one line
.where(arrayContains('tags', 'comedy')). The affected field is the argument.

And then reading another line
Field.of('tags').arrayContainsAll('adventure', 'magic'). The affected field is this.

I'm assuming you're only switching around for testing completeness, but also I'm just making sure we need to support both patterns?

MarkDuckworth · 2024-08-16T23:53:39Z

dev/system-test/pipeline.ts

+  it('testRegexMatches', async () => {
+    const results = await randomCol
+      .pipeline()
+      .where(regexMatch('title', '.*(?i)(the|of).*'))


I think I understand the difference between regexContains and regexMatch. Perhaps we should drop one of these. Most authors using regex should be able to write an expressions that match beginning and end of line.
regexMatch('title', '(?i)(the|of)') vs regexMatch('title', '^.*(?i)(the|of).*$').

Or perhaps drop regex as a prefix.
where(contains('title', 'the'))
where(contains('title', Regex.of('the|of'))

MarkDuckworth · 2024-08-17T00:01:08Z

dev/system-test/pipeline.ts

+    const results = await randomCol
+      .pipeline()
+      .select(
+        Field.of('awards').mapGet('hugo').as('hugoAward'),


I'd also consider Field.of('awards.hugo').as('hugoAward'), or is there some special behavior to mapGet?

Does mapGet() support dot notation?
Field.of('awards').mapGet('hugo.year').as('hugoAwardYear')
vs
Field.of('awards').mapGet('hugo').mapGet('year').as('hugoAwardYear')

Good question. It looks like mapGet does not support nested fields (always returns null).

One benefit is maybe mapGet supports field with special chars better (., /, etc).

MarkDuckworth · 2024-08-17T00:03:25Z

dev/system-test/pipeline.ts

+    const results = await randomCol
+      .pipeline()
+      .select(
+        cosineDistance(Constant.ofVector(sourceVector), targetVector).as(


Consider just Constant.vector(sourceVector)

MarkDuckworth · 2024-08-17T00:06:49Z

dev/system-test/pipeline.ts

+        ),
+        euclideanDistance(Constant.ofVector(sourceVector), targetVector).as(
+          'euclideanDistance'
+        )


This pattern will support KNN. If eventually there is ANN support we may need a different pattern. Do we need to consider that now?

Not sure I understand.

MarkDuckworth · 2024-08-17T00:09:14Z

dev/system-test/pipeline.ts

+  it('testNestedFields', async () => {
+    const results = await randomCol
+      .pipeline()
+      .where(eq('awards.hugo', true))


Cool. Does this make mapGet redundant?

MarkDuckworth · 2024-08-19T20:31:49Z

dev/src/pipeline.ts

+      } else if (selectable instanceof ExprWithAlias) {
+        const expr = selectable as ExprWithAlias<Expr>;
+        result.set(expr.alias, expr.expr);
+      }


If Selectable is an interface, should we make the interface contain the common methods you need here:

interface Selectable { // @internal toExprMap(): Map<string, Expr>; }

MarkDuckworth · 2024-08-19T20:37:43Z

dev/src/pipeline.ts

+    return new Pipeline(this.db, copy);
+  }
+
+  distinct(...groups: (string | Selectable)[]): Pipeline {


Consider making distinct and select (and possibly other functions) consistent in their API surface.

Either make the API

select(...fields: (Selectable | string)[]): Pipeline distinct(...groups: (Selectable | string)[]): Pipeline

Or:

select(...fields: string[]): Pipeline select(...fields: Selectable[]): Pipeline distinct(...groups: string[]): Pipeline distinct(...groups: Selectable[]): Pipeline

MarkDuckworth · 2024-08-19T20:38:55Z

dev/src/pipeline.ts

+  aggregate(...accumulators: AccumulatorTarget[]): Pipeline;
+  aggregate(options: {
+    accumulators: AccumulatorTarget[];
+    groups?: (string | Selectable)[];


Same comment. Maybe this is pushing us to change select to select(...fields: (Selectable | string)[]): Pipeline

MarkDuckworth · 2024-08-19T20:40:00Z

dev/src/pipeline.ts

+    field: string | Field,
+    vector: FirebaseFirestore.VectorValue | number[],
+    options: FindNearestOptions
+  ): Pipeline;


Note to update this to the single options param design as used in the Query API.

Yes, this is on my todo.

MarkDuckworth · 2024-08-19T20:41:22Z

dev/src/pipeline.ts

+  sort(options: {
+    orderings: Ordering[];
+    density?: 'unspecified' | 'required';
+    truncation?: 'unspecified' | 'disabled';


What's the difference between 'unspecified' and undefined (or not passing a value)?

They are the same. It does not matter anymore though, the backend decided to remove these options.

MarkDuckworth · 2024-08-19T22:21:13Z

dev/src/stage.ts

+  }
+}
+
+export class GenerateStage implements Stage {


Should this be GenericStage to match the API method Pipeline.genericStage()

MarkDuckworth · 2024-08-19T22:30:45Z

dev/src/expression.ts

+
+  arrayConcat(...values: Expr[]): ArrayConcat;
+  arrayConcat(...values: any[]): ArrayConcat;
+  arrayConcat(...values: any[]): ArrayConcat {


Consider making values a standard array parameter, not rest parameters.

MarkDuckworth · 2024-08-19T22:45:31Z

dev/src/expression.ts

+  dotProductDistance(other: Expr): DotProductDistance;
+  dotProductDistance(other: VectorValue): DotProductDistance;
+  dotProductDistance(other: number[]): DotProductDistance;
+  dotProductDistance(other: Expr | VectorValue | number[]): DotProductDistance {


Might be more accurate to just call this this and the class dotProduct. Drop "Distance"

MarkDuckworth · 2024-08-19T22:51:25Z

dev/src/expression.ts

+  static of(value: Array<any>): Constant;
+  static of(value: Map<string, any>): Constant;
+  static of(value: VectorValue): Constant;
+  static of(value: api.IValue): Constant;


If you want to make this more extensible, you could also add an interface

interface FirestoreProtoSerializable { toProto(): api.IValue; }

and have a static of(value: FirestoreProtoSerializable): Constant

MarkDuckworth · 2024-08-19T22:53:25Z

dev/src/expression.ts

+  }
+}
+
+class DotProductDistance extends Function {


Same. Just "DotProduct" is probably more accurate.

MarkDuckworth

Done with reviews for now

MarkDuckworth · 2024-08-20T17:08:24Z

dev/src/expression.ts

+    return new Fields([Field.of(name), ...others.map(Field.of)]);
+  }
+
+  static ofAll(): Fields {


Is this used?

Will remove.

MarkDuckworth · 2024-08-20T17:14:38Z

dev/src/expression.ts

+  filterable = true as const;
+}
+
+class ArrayContainsAny extends Function implements FilterCondition {


Is FilterCondition always a function that returns a boolean? If so, would it be useful to make a more generic BooleanFunction interface in case there is a future use case for boolean functions outside of filtering?

MarkDuckworth · 2024-08-20T17:20:54Z

dev/src/expression.ts

+export function add(left: string, right: Expr): Add;
+export function add(left: string, right: any): Add;
+export function add(left: Expr | string, right: Expr | any): Add {
+  const normalizedLeft = typeof left === 'string' ? Field.of(left) : left;


Great candidate for a helper Field.fromArgument(left). Similar to FieldPath.fromArgument(stringOrFieldPath)

MarkDuckworth · 2024-08-20T17:22:47Z

dev/src/expression.ts

+export function add(left: string, right: any): Add;
+export function add(left: Expr | string, right: Expr | any): Add {
+  const normalizedLeft = typeof left === 'string' ? Field.of(left) : left;
+  const normalizedRight = right instanceof Expr ? right : Constant.of(right);


Expr.fromExpressionOrConstant(right)

MarkDuckworth · 2024-08-20T17:27:04Z

dev/src/expression.ts

+export function inAny(element: Expr, others: Expr[]): In;
+export function inAny(element: Expr, others: any[]): In;
+export function inAny(element: string, others: Expr[]): In; // Added overload
+export function inAny(element: string, others: any[]): In; // Added overload


If this behavior is different from the classic in() supporting array, should we also change Expr.in(...) to name Expr.inAny(...). Or if this is only supporting in arrays, would it make more sense to name it inArray(...)?

Will rename Expr.in to inAny.

MarkDuckworth · 2024-08-20T17:27:12Z

dev/src/expression.ts

+export function notInAny(element: Expr, others: Expr[]): Not;
+export function notInAny(element: Expr, others: any[]): Not;
+export function notInAny(element: string, others: Expr[]): Not; // Added overload
+export function notInAny(element: string, others: any[]): Not; // Added overload


wu-hui

Thanks for the feedback! I addressed some of those, and maybe we should discuss for the rest.

wu-hui · 2024-08-26T14:30:33Z

dev/src/pipeline.ts

+    return new Pipeline(this.db, copy);
+  }
+
+  distinct(...groups: (string | Selectable)[]): Pipeline {


wu-hui · 2024-08-26T14:31:53Z

dev/src/pipeline.ts

+  aggregate(...accumulators: AccumulatorTarget[]): Pipeline;
+  aggregate(options: {
+    accumulators: AccumulatorTarget[];
+    groups?: (string | Selectable)[];


wu-hui · 2024-08-26T14:33:49Z

dev/src/pipeline.ts

+    field: string | Field,
+    vector: FirebaseFirestore.VectorValue | number[],
+    options: FindNearestOptions
+  ): Pipeline;


Yes, this is on my todo.

wu-hui · 2024-08-26T14:37:26Z

dev/src/pipeline.ts

+  sort(options: {
+    orderings: Ordering[];
+    density?: 'unspecified' | 'required';
+    truncation?: 'unspecified' | 'disabled';


They are the same. It does not matter anymore though, the backend decided to remove these options.

wu-hui · 2024-08-26T14:39:44Z

dev/src/pipeline.ts

+          orderings: Ordering[];
+          density?: 'unspecified' | 'required';
+          truncation?: 'unspecified' | 'disabled';
+        },


With density and truncation removed, it's probably an overkill now.

wu-hui · 2024-08-26T14:52:57Z

dev/system-test/pipeline.ts

+        arrayTransform(
+          Field.of('tags'),
+          arrayElement().strConcat('transformed')
+        ).as('transformedTags')


Same, it looks decent after i rewrite it with fluent builder.

wu-hui · 2024-08-26T14:56:19Z

dev/system-test/pipeline.ts

+    const results = await randomCol
+      .pipeline()
+      .select(
+        cosineDistance(Constant.ofVector(sourceVector), targetVector).as(


wu-hui · 2024-08-26T14:56:46Z

dev/system-test/pipeline.ts

+        ),
+        euclideanDistance(Constant.ofVector(sourceVector), targetVector).as(
+          'euclideanDistance'
+        )


Not sure I understand.

wu-hui · 2024-08-26T15:16:07Z

dev/system-test/pipeline.ts

+    const results = await randomCol
+      .pipeline()
+      .select(
+        Field.of('awards').mapGet('hugo').as('hugoAward'),


Good question. It looks like mapGet does not support nested fields (always returns null).

One benefit is maybe mapGet supports field with special chars better (., /, etc).

wu-hui · 2024-08-26T15:16:29Z

dev/system-test/pipeline.ts

+  it('testNestedFields', async () => {
+    const results = await randomCol
+      .pipeline()
+      .where(eq('awards.hugo', true))


… in expression.ts

MarkDuckworth · 2024-10-08T16:33:48Z

dev/src/expression.ts

+   */
+  static of(name: string): Field;
+  static of(path: firestore.FieldPath): Field;
+  static of(nameOrPath: string | firestore.FieldPath): Field;


This overload is redundant. static of(nameOrPath: string | firestore.FieldPath): Field; Either remove this or keep it and remove the two above.

MarkDuckworth · 2024-10-08T16:48:59Z

dev/src/expression.ts

+   * @param value The Firestore proto value.
+   * @return A new `Constant` instance.
+   */
+  static of(value: api.IValue): Constant;


This appears to be redeclared below on line 2083

MarkDuckworth · 2024-10-08T18:16:56Z

dev/src/expression.ts

+      return this.value;
+    }
+
+    return serializer.encodeValue(this.value)!;


serializer.endcodeValue(undefined) will return null, which is a permitted but unhandled case. Do we want to treat undefined as null?

…ement NotEqAny.

wu-hui added 4 commits June 6, 2024 10:28

introduce pipeline proto changes

4080bfb

Add expressions, stages, and pipeline

608a75e

can run pipelines

25ee91a

pipeline impl

9ad2614

wu-hui requested review from a team as code owners July 30, 2024 16:26

product-auto-label bot added size: xl Pull request size is extra large. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Jul 30, 2024

wu-hui assigned MarkDuckworth Jul 31, 2024

wu-hui requested a review from MarkDuckworth July 31, 2024 17:56

MarkDuckworth reviewed Aug 16, 2024

View reviewed changes

MarkDuckworth reviewed Aug 17, 2024

View reviewed changes

MarkDuckworth reviewed Aug 19, 2024

View reviewed changes

MarkDuckworth reviewed Aug 20, 2024

View reviewed changes

wu-hui added 4 commits August 22, 2024 10:37

Fix routing headers

a514c65

Basic initial converter support

e957017

Converter cleanup

50f061b

Address some feedbacks and add more nested field test.

5947337

wu-hui commented Aug 26, 2024

View reviewed changes

wu-hui added 9 commits August 26, 2024 15:38

License header and tests fix

84e5eab

License header and tests fix

eff4589

Merge remote-tracking branch 'origin/wuandy/PplPP' into wuandy/PplPP

0dd8b09

Addressing comments.

a3e62b5

DTS + some other fixes.

997c6c5

FindNearest update

f0fdad5

pipeline and stage dts fix done. expr to go.

f0ba8da

fixing expression.ts type decl

38b3981

length to strLength

3aa8edd

wu-hui force-pushed the wuandy/PplPP branch from 0f4066c to 3aa8edd Compare September 12, 2024 17:43

remove __path__ hack

c97ed97

wu-hui added 10 commits September 25, 2024 15:57

resync with expressions catelog

9116488

comment out bit operations

670a124

more syncs

773bcd7

Fix rest connection by correct url endpoint.

c22a2db

documentation fix

a0fb269

Fix system tests build error by not using instanceof firestore.Expr…

b43de57

… in expression.ts

remove __path__

1d9c585

add reference test

d9194b7

added transaction support.

091ddf0

Add comments

14b67dd

wu-hui force-pushed the wuandy/PplPP branch from 894c1ae to 14b67dd Compare October 4, 2024 15:28

MarkDuckworth reviewed Oct 8, 2024

View reviewed changes

wu-hui and others added 4 commits October 24, 2024 14:46

add remove_fields support

0c04b48

add d.ts change for removeFields

91bd76e

fix build

cb9d692

Rename cond, *minimum, *maximum, eqAny, and notEqAny. New tests. Impl…

5db7307

…ement NotEqAny.

[feat] ppl #2094

Are you sure you want to change the base?

[feat] ppl #2094

Conversation

wu-hui commented Jul 30, 2024

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkDuckworth Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

MarkDuckworth Oct 8, 2024 •

edited

Loading