feat(firestore): [PQ] Support pipeline queries#12347
Conversation
5806663 to
a68c104
Compare
* feat(firestore): add pipeline queries * add comments * remove exists
* feat(firestore): Add rest of the input stages * correct the method comment to resolve vet failure * address review comments
a68c104 to
f069709
Compare
* feat(firestore): Add pipeline expressions basic structure * refactor
) b/364927702 1. Fixes test ```go --- FAIL: TestPipelineResult_NoResults (0.00s) panic: runtime error: invalid memory address or nil pointer dereference [recovered, repanicked] [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x10d40b6] goroutine 4633 [running]: testing.tRunner.func1.2({0x13c5240, 0x1ec2150}) /usr/local/go/src/testing/testing.go:1872 +0x419 testing.tRunner.func1() /usr/local/go/src/testing/testing.go:1875 +0x683 panic({0x13c5240?, 0x1ec2150?}) /usr/local/go/src/runtime/panic.go:783 +0x132 cloud.google.com/go/firestore.(*PipelineResult).Data(0xc00046f380) /tmpfs/src/google-cloud-go/firestore/pipeline_result.go:95 +0x96 cloud.google.com/go/firestore.TestPipelineResult_NoResults(0xc000455500) /tmpfs/src/google-cloud-go/firestore/pipeline_result_test.go:359 +0x312 testing.tRunner(0xc000455500, 0x157dca0) /usr/local/go/src/testing/testing.go:1934 +0x21d created by testing.(*T).Run in goroutine 1 /usr/local/go/src/testing/testing.go:1997 +0x9d3 FAIL cloud.google.com/go/firestore 2.337s ``` ```go === RUN TestPipelineResultIterator_GetAll pipeline_result_test.go:249: second result id: got 1, want: 2 --- FAIL: TestPipelineResultIterator_GetAll (0.00s) ``` 2. Add enterprise database env variable Previous pull requests: - #12217 - #12425 - #12538 - #13147
|
/gemini help |
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: pipeline queries. The changes include adding the ExecutePipeline RPC to the client, defining the necessary protobuf messages, and implementing a comprehensive fluent API for building and executing these pipelines. The overall design of the pipeline builder API is solid, using interfaces and a fluent style to provide a good developer experience. However, I've identified a critical area for improvement in the low-level client implementation where routing header construction logic is inefficient and duplicated. Addressing this will improve both performance and maintainability.
| routingHeaders := "" | ||
| routingHeadersMap := make(map[string]string) | ||
| if reg := regexp.MustCompile("projects/(?P<project_id>[^/]+)(?:/.*)?"); reg.MatchString(req.GetDatabase()) && len(url.QueryEscape(reg.FindStringSubmatch(req.GetDatabase())[1])) > 0 { | ||
| routingHeadersMap["project_id"] = url.QueryEscape(reg.FindStringSubmatch(req.GetDatabase())[1]) | ||
| } | ||
| if reg := regexp.MustCompile("projects/[^/]+/databases/(?P<database_id>[^/]+)(?:/.*)?"); reg.MatchString(req.GetDatabase()) && len(url.QueryEscape(reg.FindStringSubmatch(req.GetDatabase())[1])) > 0 { | ||
| routingHeadersMap["database_id"] = url.QueryEscape(reg.FindStringSubmatch(req.GetDatabase())[1]) | ||
| } | ||
| for headerName, headerValue := range routingHeadersMap { | ||
| routingHeaders = fmt.Sprintf("%s%s=%s&", routingHeaders, headerName, headerValue) | ||
| } | ||
| routingHeaders = strings.TrimSuffix(routingHeaders, "&") |
There was a problem hiding this comment.
This block of code for building routing headers has several inefficiencies:
- Regex Compilation:
regexp.MustCompileis called on every request, which is computationally expensive. These regular expressions should be compiled once at the package level and reused. - Redundant Calls:
reg.FindStringSubmatchandurl.QueryEscapeare called multiple times for the same input within theifcondition and the block. - Inefficient String Building: Building the query string using
fmt.Sprintfin a loop is inefficient. The idiomatic and more performant way to build URL query strings in Go is to use thenet/url.Valuestype and itsEncode()method.
This same logic is also duplicated in the restClient implementation. I recommend refactoring this into a single, efficient helper function that is shared between the gRPC and REST clients.
routingHeaders := func() string {
params := url.Values{}
if matches := regexp.MustCompile(`projects/([^/]+)`).FindStringSubmatch(req.GetDatabase()); len(matches) > 1 {
params.Set("project_id", matches[1])
}
if matches := regexp.MustCompile(`projects/[^/]+/databases/([^/]+)`).FindStringSubmatch(req.GetDatabase()); len(matches) > 1 {
params.Set("database_id", matches[1])
}
return params.Encode()
}()There was a problem hiding this comment.
This is generated code and cannot be modified manually.
| routingHeaders := "" | ||
| routingHeadersMap := make(map[string]string) | ||
| if reg := regexp.MustCompile("projects/(?P<project_id>[^/]+)(?:/.*)?"); reg.MatchString(req.GetDatabase()) && len(url.QueryEscape(reg.FindStringSubmatch(req.GetDatabase())[1])) > 0 { | ||
| routingHeadersMap["project_id"] = url.QueryEscape(reg.FindStringSubmatch(req.GetDatabase())[1]) | ||
| } | ||
| if reg := regexp.MustCompile("projects/[^/]+/databases/(?P<database_id>[^/]+)(?:/.*)?"); reg.MatchString(req.GetDatabase()) && len(url.QueryEscape(reg.FindStringSubmatch(req.GetDatabase())[1])) > 0 { | ||
| routingHeadersMap["database_id"] = url.QueryEscape(reg.FindStringSubmatch(req.GetDatabase())[1]) | ||
| } | ||
| for headerName, headerValue := range routingHeadersMap { | ||
| routingHeaders = fmt.Sprintf("%s%s=%s&", routingHeaders, headerName, headerValue) | ||
| } | ||
| routingHeaders = strings.TrimSuffix(routingHeaders, "&") |
There was a problem hiding this comment.
This logic for constructing routing headers is a duplicate of the implementation in the gRPCClient.ExecutePipeline method. Duplicating code like this makes the software harder to maintain, as any future changes or bug fixes will need to be applied in multiple places.
As suggested in the other comment, this logic should be extracted into a single, shared helper function to improve maintainability and reduce redundancy.
There was a problem hiding this comment.
This is generated code and cannot be modified manually.
…3194) b/364927702 1. add all the remaining private preview aggregate functions. Merging this PR completes the implementation of all the **type: "Function" subType : "Accumulators (Aggregation)"** private preview features. See "Firestore Features (Pipeline)" sheet in [go/firestore-query-tracker](http://go/firestore-query-tracker) for the list of features. Java reference: - https://github.com/googleapis/java-firestore/blob/ccaf9d4fac5bd87a4da3d37493ca66fdc7681bc3/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/AggregateFunction.java#L43-L71 - https://github.com/googleapis/java-firestore/blob/ccaf9d4fac5bd87a4da3d37493ca66fdc7681bc3/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/AggregateFunction.java#L93-L111 2. add all the remaining private preview timestamp functions. Merging this PR completes the implementation of all the **type: "Function" subType: "Date / Timestamp"** private preview features. (except timestamp_trunc function which is not yet inmplemented in any of the SDKs. Requires additonal approvals from Firestore team and will be added to separate PR). See "Firestore Features (Pipeline)" sheet in [go/firestore-query-tracker](http://go/firestore-query-tracker) for the list of functions. Java reference: - https://github.com/googleapis/java-firestore/blob/ccaf9d4fac5bd87a4da3d37493ca66fdc7681bc3/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/Expression.java#L2262-L2517 3. Add integration tests for all functions. 4. Remove Rand function since it is not targeted for private preview. 5. Renamed numericExprOrField to numericExprOrFieldPath since field is a separate type/expression. https://github.com/googleapis/google-cloud-go/blob/a3ee1f19068c6d3fb77ad797e29884a90d6402a2/firestore/pipeline_field.go#L21-L41 Previous pull requests - #12217 - #12425 - #12538 - #13147 - #13199 - #13218
…13218) b/364927702 1. add all the remaining private preview stages. Merging this PR completes the implementation of all the type "Stage" subType "General" private preview features. (except literals stage which is not yet inmplemented in Java and Node. Requires additonal approvals from Firestore team and will be added to separate PR). See "Firestore Features (Pipeline)" sheet in [go/firestore-query-tracker](http://go/firestore-query-tracker) 2. Add integration and unit tests. 3. Refactor existing stages code to remove duplicated code and rearrange in alphabetical order. 4. Modify behaviour of Data and DataTo to match existing implementation in document.go https://github.com/googleapis/google-cloud-go/blob/9a4cb31f4d34948404d91123fb560a43aeebe83e/firestore/document.go#L64-L127 Previous pull requests - #12217 - #12425 - #12538 - #13147 - #13199
) 1. add all the private preview 'array' functions. Merging this PR completes the implementation of all the **type: "Function" subType : "Array"** private preview features (except 'maximum' and 'minimum' which are not yet implemented in any of the SDKs. Requires additonal approvals from Firestore team and will be added to separate PR). See "Firestore Features (Pipeline)" sheet in [go/firestore-query-tracker](http://go/firestore-query-tracker) for the list of features. Java reference: - https://github.com/googleapis/java-firestore/blob/wuandy/JavaPplPP/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/Expression.java 2. add all the private preview 'string' functions. (except 'string_split' which is not yet implemented in any of the SDKs. Requires additonal approvals from Firestore team and will be added to separate PR) 3. add all the private preview 'vector' functions. 4. add remaining types to ConstantOf to match Java's implementation. Java reference: - https://github.com/googleapis/java-firestore/blob/ccaf9d4fac5bd87a4da3d37493ca66fdc7681bc3/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/Expression.java#L70-L211 Previous pull requests - #12217 - #12425 - #12538 - #13147 - #13199 - #13218 - #13194
b/364927702 - toExprOrField was renamed to asFieldExpr in https://github.com/googleapis/google-cloud-go/pull/13194/files#diff-4a55211f7d38a1f0599e2f4cc92795073f138b2c56b846c933bda19e26bc3a7a . There were a few call locations were rename was missed while resolving merge conflicts which caused build failures. Fixing those failures in this PR. - the function signature of Data was changed in #13218. It no longer returns err as second argument. Fixing this in this PR. - remove duplicate asInt64Expr and asStringExpr - Move pipeline tests to their own file Previous pull requests - #12217 - #12425 - #12538 - #13147 - #13199 - #13218 - #13194 - #13245
b/364927702 - Combine FieldOf and FieldOfPath to avoid verbose name FieldOfPath Previous pull requests - #12217 - #12425 - #12538 - #13147 - #13199 - #13218 - #13194 - #13245 - #13270 --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
#13283) Changes in this PR: 1. firestore_client.go : Updated generated client as per googleapis/gapic-generator-go#1661 . Removed retries from tests since the headers have now been fixed. 2. Remove Equivalent since it was removed from backend. 3. Add/update comments 4. Add timestamp truncate (pending from #13194) and string split (pending from #13245) functions. 5. add all the private preview general, key, logical (except iferror), type and object functions. See "Firestore Features (Pipeline)" sheet in [go/firestore-query-tracker](http://go/firestore-query-tracker) for the list of functions. Java reference: - https://github.com/googleapis/java-firestore/blob/ccaf9d4fac5bd87a4da3d37493ca66fdc7681bc3/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/Expression.java Previous pull requests - #12217 - #12425 - #12538 - #13147 - #13199 - #13218 - #13194 - #13245 - #13270 - #13271 - #13279 - #13280 - #13281 - #13282 - googleapis/gapic-generator-go#1661
add collection and collectiongroup stage options similar to Java https://github.com/googleapis/java-firestore/blob/742fab6583c9a6f9c47cf0496124c3c9b05fe0ee/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/stages/CollectionGroupOptions.java#L34-L36 https://github.com/googleapis/java-firestore/blob/742fab6583c9a6f9c47cf0496124c3c9b05fe0ee/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/stages/CollectionOptions.java#L34-L36 https://github.com/googleapis/java-firestore/blob/742fab6583c9a6f9c47cf0496124c3c9b05fe0ee/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/stages/CollectionHints.java#L34-L40 Previous pull requests - #12217 - #12425 - #12538 - #13147 - #13199 - #13218 - #13194 - #13245 - #13270 - #13271 - #13279 - #13280 - #13282 - googleapis/gapic-generator-go#1661
Add ExecuteOptions similar to query run options which was introduced in #10164. GetRawData and GetText implementation similar to Java https://github.com/googleapis/java-firestore/blob/742fab6583c9a6f9c47cf0496124c3c9b05fe0ee/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/stages/PipelineExecuteOptions.java https://github.com/googleapis/java-firestore/blob/742fab6583c9a6f9c47cf0496124c3c9b05fe0ee/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/stages/ExplainOptions.java https://github.com/googleapis/java-firestore/blob/742fab6583c9a6f9c47cf0496124c3c9b05fe0ee/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ExplainStats.java#L42-L71 https://github.com/googleapis/java-firestore/blob/742fab6583c9a6f9c47cf0496124c3c9b05fe0ee/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITPipelineTest.java#L2474-L2498 Previous pull requests - #12217 - #12425 - #12538 - #13147 - #13199 - #13218 - #13194 - #13245 - #13270 - #13271 - #13279 - #13280 - #13281 - #13282 - googleapis/gapic-generator-go#1661 - #13283
Add CreateFrom() and Pipeline() similar to Java. https://github.com/googleapis/java-firestore/blob/742fab6583c9a6f9c47cf0496124c3c9b05fe0ee/google-cloud-firestore/src/main/java/com/google/cloud/firestore/PipelineSource.java#L140-L164 Previous pull requests - #12217 - #12425 - #12538 - #13147 - #13199 - #13218 - #13194 - #13245 - #13270 - #13271 - #13279 - #13280 - #13281 - #13282 - googleapis/gapic-generator-go#1661 - #13283 - #13274 - #13338
header-check fails on #12347 with error firestore/pipeline.go should have a copyright year of 2026 firestore/pipeline_aggregate.go should have a copyright year of 2026 firestore/pipeline_constant.go should have a copyright year of 2026 firestore/pipeline_expression.go should have a copyright year of 2026 firestore/pipeline_field.go should have a copyright year of 2026 firestore/pipeline_filter_condition.go should have a copyright year of 2026 firestore/pipeline_function.go should have a copyright year of 2026 firestore/pipeline_integration_test.go should have a copyright year of 2026 firestore/pipeline_result.go should have a copyright year of 2026 firestore/pipeline_result_test.go should have a copyright year of 2026 firestore/pipeline_source.go should have a copyright year of 2026 firestore/pipeline_source_test.go should have a copyright year of 2026 firestore/pipeline_stage.go should have a copyright year of 2026 firestore/pipeline_stage_test.go should have a copyright year of 2026 firestore/pipeline_test.go should have a copyright year of 2026 firestore/pipeline_utils.go should have a copyright year of 2026 Fixing it in this PR
In Java, Node and Python, Execute returns a Pipeline snapshot. This PR introduces similar structure in Go https://github.com/googleapis/java-firestore/blob/6e30a6c11efe5d428607bfd78f82ba7b49497bd9/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Pipeline.java#L150-L159 It does not have execution time similar to Java yet. (Coming soon in GA) --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…e Function (#13594) 1) refactor CountIf and rename Function to match Java and Node https://github.com/googleapis/java-firestore/blob/6e30a6c11efe5d428607bfd78f82ba7b49497bd9/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/BooleanExpression.java#L26-L35 https://github.com/googleapis/java-firestore/blob/6e30a6c11efe5d428607bfd78f82ba7b49497bd9/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/AggregateFunction.java#L111-L121 https://github.com/googleapis/java-firestore/blob/6e30a6c11efe5d428607bfd78f82ba7b49497bd9/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/FunctionExpression.java#L28 2) Revert proto files permission changes 3) Revert empty proto files 4) Refactor Replace and Unnest to match Java https://github.com/googleapis/java-firestore/blob/6e30a6c11efe5d428607bfd78f82ba7b49497bd9/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Pipeline.java#L707-L777 https://github.com/googleapis/java-firestore/blob/6e30a6c11efe5d428607bfd78f82ba7b49497bd9/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Pipeline.java#L851-L996
Index mode has been postponed to GA. removing it.
All the proto changes have been reverted. Removing breaking change label |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: Firestore Pipeline Queries. The implementation is extensive, adding a fluent API for building complex data processing pipelines. The code is well-structured, following good practices for creating a chained, immutable API. I've found a couple of minor issues in the documentation examples that could confuse users. Overall, this is a solid addition to the library.
| // client.Pipeline().Collection("books"). | ||
| // .Offset(20) // Skip the first 20 results | ||
| // .Limit(20) // Take the next 20 results |
There was a problem hiding this comment.
The code example for Offset contains a syntax error. The leading dot . on line 262 makes the example invalid Go code. This could confuse users who copy and paste the example. Please correct it to be a valid, runnable snippet.
| // client.Pipeline().Collection("books"). | |
| // .Offset(20) // Skip the first 20 results | |
| // .Limit(20) // Take the next 20 results | |
| // client.Pipeline().Collection("books"). | |
| // Offset(20). // Skip the first 20 results | |
| // Limit(20) // Take the next 20 results |
| // client.Pipeline().Collection("users").Select("info.email") | ||
| // client.Pipeline().Collection("users").Select(FieldOf("info.email")) | ||
| // client.Pipeline().Collection("users").Select(FieldOf([]string{"info", "email"})) | ||
| // client.Pipeline().Collection("users").Select(FieldOf([]string{"info", "email"})) |
daniel-sanche
left a comment
There was a problem hiding this comment.
These PRs were individually reviewed and approved, so merging the branch LGTM
b/364927702
This is a combination of all below PRs: