Pipelines Implementation #6710

tom-andersen · 2025-02-20T16:37:00Z

No description provided.

google-oss-bot · 2025-02-20T16:46:14Z

Coverage Report ¹

Affected Products

No changes between base commit (7999ab4) and merge commit (42b9aaf).

Test Logs

Merge (42b9aaf): https://github.com/firebase/firebase-android-sdk/actions/runs/16577006040

https://storage.googleapis.com/firebase-sdk-metric-reports/vxC6MxXIOn.html

github-actions · 2025-02-20T16:47:46Z

Test Results

8 files 8 suites 1m 0s ⏱️
12 tests 12 ✅ 0 💤 0 ❌
24 runs 24 ✅ 0 💤 0 ❌

Results for commit f579bf4.

♻️ This comment has been updated with latest results.

MarkDuckworth

Review is WIP. There is lots of great cleanup in this. Nice work. Only a few comments so far.

MarkDuckworth · 2025-07-08T20:45:53Z

firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java

@@ -120,6 +120,15 @@ public String getPath() {
    return key.getPath().canonicalString();
  }

+  @RestrictTo(RestrictTo.Scope.LIBRARY)
+  @NonNull
+  public String getFullPath() {


Should javadocs for RestrictTo Library indicate that status? Or will this be visible even without use of the linter?

It should restrict visibility from my understanding.

MarkDuckworth · 2025-07-08T21:04:04Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Pipeline.kt

+  fun replace(field: String): Pipeline = replace(Expr.field(field))
+
+  /**
+   * Fully overwrites all fields in a document with those coming from a nested map.


i don't think this has to be a nested map, it can be any expression that evaluates to a map

MarkDuckworth · 2025-07-08T21:05:59Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Pipeline.kt

+    unnest(Expr.field(arrayField).alias(alias))
+
+  /**
+   * Takes a specified array from the input documents and outputs a document for each element with


Same, i think this can be any Selectable with an expression that evaluates to an array.

"Takes a specified array" -> "Transforms an array expression" ?

MarkDuckworth

really impressive code. I learned a lot about kotlin. I'm offering a handful of questions and comments.

MarkDuckworth · 2025-07-09T15:46:16Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/aggregates.kt

+
+  companion object {
+
+    @JvmStatic fun generic(name: String, vararg expr: Expr) = AggregateFunction(name, expr)


should this be raw?

Yes, I think so.

MarkDuckworth · 2025-07-09T15:47:48Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/aggregates.kt

+     *
+     * @return A new [AggregateFunction] representing the countAll aggregation.
+     */
+    @JvmStatic fun countAll() = AggregateFunction("count")


should the SDK name and server API name match?

I think we discussed this and were sticking to countAll ... but feel free to change in my absence.

MarkDuckworth · 2025-07-09T19:13:18Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/expressions.kt

+    @JvmStatic
+    fun constant(value: Boolean): BooleanExpr {
+      val encodedValue = encodeValue(value)
+      return object : BooleanExpr("N/A", emptyArray()) {


It might be useful to include some documentation covering the reason why you are creating an anonymous BooleanExpr class here

Added comment

MarkDuckworth · 2025-07-09T19:15:35Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/expressions.kt

+     * @return A new [Expr] constant instance representing the Blob.
+     */
+    @JvmStatic
+    fun blob(bytes: ByteArray): Expr {


Possible discussion topic, but it might be more clear to indicate in the function name that this will create a Constant Expr. blobConstant(...)

Worthy discussion topic. I tend to agree with you.

I'll leave this in your hands.

MarkDuckworth · 2025-07-09T19:16:07Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/expressions.kt

+     * @return A [Expr] constant instance.
+     */
+    @JvmStatic
+    fun vector(vector: DoubleArray): Expr {


Same, might be worth naming vectorConstant(...) or constantVector(...)

MarkDuckworth · 2025-07-10T00:15:15Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/options.kt

+
+      @JvmField val JSON = OutputFormat("json")
+
+      @JvmField val STRUCT = OutputFormat("struct")


I haven't seen 'struct' in the spec. Is this different from JSON?

Whereas JSON is a large string, Struct is the equivalent proto format that takes advantage of protocol buffers to be much smaller. So Struct is the same as JSON, the same way YAML is the same as JSON. They both offer the same expressiveness, in a slightly different format. There are of course nuances, JSON can contain very large numeric since they are stored as strings. And YAML can contain comments.

This is a remnant from an early discussion. So yes, it should be removed for now.

At some point I hope Firestore will concern itself more with wire format, instead of stuffing JSON into protocol buffers. Which I imagine is like stuffing JSON inside a JSON string (which I believe is the case of REST variant of explain)?

MarkDuckworth · 2025-07-10T00:15:49Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/options.kt

+    }
+  }
+
+  class Verbosity private constructor(internal val value: String) {


same, i haven't seen this in the design. is this new?

MarkDuckworth · 2025-07-10T00:15:53Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/options.kt

+    }
+  }
+
+  class Profiles private constructor(internal val value: String) {


same, i haven't seen this in the design. is this new?

MarkDuckworth · 2025-07-10T00:20:14Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/stage.kt

+  override fun args(userDataReader: UserDataReader): Sequence<Value> = emptySequence()
+}
+
+class CollectionSource


in node, i found that i liked calling all of these *StageOptions. It gave consistency and meaning across the Pipeline api. Maybe you would consider calling these *Stage.

E.g. CollectionStage or CollectionSourceStage, DatabaseStage, LimitStage, WhereStage, ...

MarkDuckworth · 2025-07-10T00:23:05Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/stage.kt

+    fields.asSequence().map(Field::toProto)
+}
+
+internal class ReplaceStage


Do you have intent to make all of these internal stages public to support options overrides?

Yes.

Does it need to be done before release? Maybe (your call)

I only exposed the stage classes when the API required it, not just future proofing raw options. Exposing the stages will require also adding constructors / create methods that offer same semantics and documentation as Pipeline API. I wasn't going to do this work until the dust settled on options, and it is not critical for release in my opinion. There is always the rawStage.

github-actions · 2025-07-28T18:17:16Z

Firebase AI Mock Responses Check ⚠️

A newer major version of the mock responses for Firebase AI unit tests is available. update_responses.sh should be updated to clone the latest version of the responses: v14.2

tom-andersen

@MarkDuckworth I have addressed your feedback. Let me know if there is anything else.

tom-andersen · 2025-07-28T16:20:45Z

firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java

@@ -120,6 +120,15 @@ public String getPath() {
    return key.getPath().canonicalString();
  }

+  @RestrictTo(RestrictTo.Scope.LIBRARY)
+  @NonNull
+  public String getFullPath() {


It should restrict visibility from my understanding.

tom-andersen · 2025-07-28T16:43:50Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Pipeline.kt

+  fun replace(field: String): Pipeline = replace(Expr.field(field))
+
+  /**
+   * Fully overwrites all fields in a document with those coming from a nested map.


tom-andersen · 2025-07-28T16:48:09Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Pipeline.kt

+    unnest(Expr.field(arrayField).alias(alias))
+
+  /**
+   * Takes a specified array from the input documents and outputs a document for each element with


"Takes a specified array" -> "Transforms an array expression" ?

tom-andersen · 2025-07-28T17:23:25Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/aggregates.kt

+     *
+     * @return A new [AggregateFunction] representing the countAll aggregation.
+     */
+    @JvmStatic fun countAll() = AggregateFunction("count")


I think we discussed this and were sticking to countAll ... but feel free to change in my absence.

tom-andersen · 2025-07-28T17:24:06Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/aggregates.kt

+
+  companion object {
+
+    @JvmStatic fun generic(name: String, vararg expr: Expr) = AggregateFunction(name, expr)


Yes, I think so.

tom-andersen · 2025-07-28T17:56:32Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/options.kt

+    companion object {
+      @JvmField val EXECUTE = ExplainMode("execute")
+
+      @JvmField val EXPLAIN = ExplainMode("explain")


Removed the entire explain options.

tom-andersen · 2025-07-28T18:06:24Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/options.kt

+
+      @JvmField val JSON = OutputFormat("json")
+
+      @JvmField val STRUCT = OutputFormat("struct")


Whereas JSON is a large string, Struct is the equivalent proto format that takes advantage of protocol buffers to be much smaller. So Struct is the same as JSON, the same way YAML is the same as JSON. They both offer the same expressiveness, in a slightly different format. There are of course nuances, JSON can contain very large numeric since they are stored as strings. And YAML can contain comments.

This is a remnant from an early discussion. So yes, it should be removed for now.

At some point I hope Firestore will concern itself more with wire format, instead of stuffing JSON into protocol buffers. Which I imagine is like stuffing JSON inside a JSON string (which I believe is the case of REST variant of explain)?

tom-andersen · 2025-07-28T18:06:36Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/options.kt

+    }
+  }
+
+  class Verbosity private constructor(internal val value: String) {


tom-andersen · 2025-07-28T18:06:45Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/options.kt

+    }
+  }
+
+  class Profiles private constructor(internal val value: String) {


tom-andersen · 2025-07-28T18:11:25Z

firebase-firestore/src/main/java/com/google/firebase/firestore/pipeline/stage.kt

+    fields.asSequence().map(Field::toProto)
+}
+
+internal class ReplaceStage


Yes.

Does it need to be done before release? Maybe (your call)

I only exposed the stage classes when the API required it, not just future proofing raw options. Exposing the stages will require also adding constructors / create methods that offer same semantics and documentation as Pipeline API. I wasn't going to do this work until the dust settled on options, and it is not critical for release in my opinion. There is always the rawStage.

tom-andersen added 23 commits October 10, 2024 16:32

Proto changes

fe05087

Copyright

159cb37

Expressions

282409a

Test

7f95bb5

Stage

2ec848d

Pipeline

16bb38c

Merge remote-tracking branch 'origin/main' into tomandersen/pipelines

c2d2ae5

Fix

a8361ca

Spotless

a0da150

Spotless

0ce7d4f

Rename .java to .kt

ab5f217

Work

dec61c0

Copyright

e5734cd

Merge remote-tracking branch 'origin/main' into tomandersen/pipelines

8976978

spotless

70bdf62

docStubs fix

fc8b2d1

fix

1e7e72e

fix

35bcff7

More tests

3cb1886

Merge remote-tracking branch 'origin/main' into tomandersen/pipelines

cea0c48

Refactor and more tests.

5abf13f

Add tests

ef7cbda

Cleanup

ca31612

google-oss-bot added the size/XXL label Feb 20, 2025

tom-andersen requested review from wu-hui, MarkDuckworth and cherylEnkidu February 20, 2025 16:38

Merge branch 'feat/pipelines' into tomandersen/pipelines

63cd54e

tom-andersen added 7 commits May 23, 2025 15:30

Fix query to pipeline logic.

93a44a7

Fix query to pipeline logic.

96aac51

Fix query to pipeline logic.

385d3cf

small refactor

9f9f95a

Fixes, addition, whitespace.

45280be

Fixes / Refactor of Values

5eb6a01

Merge branch 'feat/pipelines' into tomandersen/pipelines

ac26746

MarkDuckworth reviewed Jul 8, 2025

View reviewed changes

MarkDuckworth reviewed Jul 10, 2025

View reviewed changes

firebase deleted a comment from google-oss-bot Jul 28, 2025

Response to PR review (and back porting from evaluation)

78ab98c

tom-andersen commented Jul 28, 2025

View reviewed changes

tom-andersen requested a review from MarkDuckworth July 28, 2025 18:19

Copyright

f579bf4


		companion object {

		@JvmStatic fun generic(name: String, vararg expr: Expr) = AggregateFunction(name, expr)


		@JvmField val JSON = OutputFormat("json")

		@JvmField val STRUCT = OutputFormat("struct")

Pipelines Implementation #6710

Are you sure you want to change the base?

Pipelines Implementation #6710

Uh oh!

Conversation

tom-andersen commented Feb 20, 2025

Uh oh!

google-oss-bot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

Test Logs

Uh oh!

github-actions bot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

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!

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!

github-actions bot commented Jul 28, 2025

Firebase AI Mock Responses Check ⚠️

Uh oh!

tom-andersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

google-oss-bot commented Feb 20, 2025 •

edited

Loading

Coverage Report ¹

github-actions bot commented Feb 20, 2025 •

edited

Loading