feat(firestore): refactor pipeline API for uniform stage options#14322
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Firestore Pipeline API by replacing specific settings structs with a unified options pattern using map[string]any and the StageOption interface. The changes involve updating method signatures for various pipeline stages and adding ergonomic helper functions like Fields and Accumulators. Review feedback highlights concerns regarding the integration tests, specifically the skipping of failing functional tests and the presence of commented-out test cases for features like MapKeys and ArrayFilter. These issues should be addressed to ensure the refactoring does not introduce regressions or leave the codebase with undocumented gaps in feature support.
| eo.RawOptions[k] = v | ||
| } | ||
| }) | ||
| type RawOptions map[string]any |
There was a problem hiding this comment.
I don't have a good sense of how well defined the keys are for the general options mechanism. My read here is that the map is used to invalidate a previous option with the same key, as opposed to other option patterns where its just a list that allows potential multiple modifications. Is that the driving factor here?
Do we have any problems with key conflicts across these disparate option interfaces? If that's a property we want do we need a test that ensures all defined options are uniquely keyed?
There was a problem hiding this comment.
-
Is that the driving factor here ?
Yes, exactly. The driving factor for using a map[string]any is that it directly mirrors the underlying Firestore Protocol Buffer definition.In the protobuf, a pipeline stage is defined as:
message Stage { string name = 1; repeated Value args = 2; map<string, Value> options = 3; // <-- The backend expects a map }Because the backend enforces unique string keys for options per stage, the Go SDK uses a map to naturally accumulate them. When a user passes multiple options (e.g.,
pipeline.Collection("users", opt1, opt2)), the SDK iterates through them and applies them to a single map. This gives us standard Go "last-writer-wins" semantics: if opt2 writes to the same
key as opt1, it intentionally overrides it. -
Do we have any problems with key conflicts across disparate option interfaces?
No, because the map is scoped per stage instance. There is no global options map. When a user creates a stage, a brand-new, empty map is instantiated exclusively for that specific stage:func (ps *PipelineSource) Collection(path string, opts ...CollectionOption) *Pipeline { options := make(map[string]any) // <-- Brand new map scoped only to this stage for _, opt := range opts { if opt != nil { opt.applyStage(options) } } // ...Therefore, if FindNearest uses the key "limit" and a theoretical future Sort option also uses the key "limit", they will never conflict because their options are written to completely isolated maps.
There was a problem hiding this comment.
added a test for unique keys per stage
There was a problem hiding this comment.
Thanks for the explanations!
|
update and delete stages were added while the current PR was in review. Refactored those to match the rest of the stages and their options. Also, added a test for unique keys. |
Co-authored-by: Alex Hong <9397363+hongalex@users.noreply.github.com>
Co-authored-by: Alex Hong <9397363+hongalex@users.noreply.github.com>
Co-authored-by: Alex Hong <9397363+hongalex@users.noreply.github.com>
Design decision here
Overview
This PR standardizes the API surface for the Firestore Pipeline builder in the Go SDK, ensuring idiomatic conformance and aligning closely with cross-platform design paradigms (Java/Node.js). It replaces complex stage-specific configuration structs with a universal functional option pattern and introduces typed slice constants for varied expressions.
Key Features and API Changes:
Universal
RawOptionsEscape Hatch:Introduced
RawOptionsto serve as a universal pass-through for backend options that might not be natively modeled by the Go SDK (e.g.,RawOptions{"limit": 5, "distance_field": "dist"}). This deprecates intermediate parameter structs likePipelineFindNearestOptionsacross pipeline stages.Standardized Variadic Arguments with Slices:
Methods that formerly relied on raw variadic argument unpacking (e.g.
...Ordering,...any) have been updated to accept typed slices. To preserve clean query ergonomics, we introduced domain-specific helper constructors (Orders(),Fields(),Accumulators(),Selectables()).Pipeline.Sort(Orders(Ascending(FieldOf("f")), Descending(FieldOf("__name__"))))Pipeline.Aggregate(Accumulators(Count(DocumentID).As("total")))Pipeline.Select(Fields("a", "b"))Functional Stage Options Migration:
Implemented standard functional options for specialized stage modifiers:
WithFindNearestLimit(),WithFindNearestDistanceField()WithAggregateGroups()WithUnnestIndexField()WithExplainMode()Internal Code Simplification & Testing:
baseStageremoval):Removed the
baseStagestruct embedding across our internal pipeline stages. We replaced it with explicitoptions map[string]anyfields within each stage and introduced a sharedstageOptionsToProto(options)helper. This simplifies the internal struct hierarchy, removes opaque state inherited from struct embedding, and makes configuration serialization significantly more explicit and easier to trace inside each stage'stoProto()implementation.Refactored internal
pipelineStagestructures (newAggregateStage,newFindNearestStage,newSortStage, etc.) to reliably map the newRawOptionsdown to standard*pb.Valuepayloads.AlwaysUseImplicitOrderBy:Added dedicated test cases (
TestQuery_AlwaysUseImplicitOrderBy) to explicitly verify the behavior of the existingWithAlwaysUseImplicitOrderByclient toggle, ensuring it appropriately serializes implicit inequality fields and__name__into theOrderByproto requests.Fully migrated the integration suite inside
pipeline_integration_test.goandpipeline_test.goto assert against the newly stabilizedRawOptionsand slice-builder interfaces.