Skip to content

fix(firestore): Change stages to match Java and Node#14253

Merged
bhshkh merged 3 commits intogoogleapis:mainfrom
bhshkh:fspq-fix-stages
Mar 25, 2026
Merged

fix(firestore): Change stages to match Java and Node#14253
bhshkh merged 3 commits intogoogleapis:mainfrom
bhshkh:fspq-fix-stages

Conversation

@bhshkh
Copy link
Copy Markdown
Contributor

@bhshkh bhshkh commented Mar 25, 2026

Java stages for reference: https://github.com/googleapis/java-firestore/blob/6e30a6c11efe5d428607bfd78f82ba7b49497bd9/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Pipeline.java

Node stages for reference: https://github.com/googleapis/google-cloud-node/blob/c26967ca277fdab196ea94f5561e4f451ac1ec8a/handwritten/firestore/dev/src/pipelines/pipelines.ts

  1. Java allows the user to pass in Fields into remove fields. E.g. user can do in Java .removeFields(field("published"), field("genre"), field("nestedField")) but in Go user cannot do RemoveFields(FieldOf("published"), FieldOf("genre"), FieldOf("nestedField")). This PR fixes this by allowing FieldOf in RemoveFields args
  2. Change unnest signature to a more Go idiomatic one
    1. Idiomatic Go Design: Replaced the struct pointer (*UnnestOptions) with a variadic slice of interfaces (...UnnestOption). This standardizes developer ergonomics and aligns with other areas of the Firestore Go SDK.
    2. Cleaner Zero-value Defaults: Callers are no longer forced to pass nil if they don't wish to supply specific configuration arguments.
    3. Better Hygeine & Robustness: Internal logic initializes default underlying representations implicitly without requiring users to instantiate or check nil pointer values.
  3. Refactored the Sample stage
    The changes to the sample stage from SampleSpec to Sampler, and SampleByDocuments to ByDocuments were driven by idiomatic Go design principles regarding readability and removing structural redundancy:
    1. Reducing Stuttering in Code
      In the previous API, calling a sample operation resulted in a redundant "stutter" in the code:

      // Old Way: Read "Sample" "Sample By Documents"
      pipeline.Sample(SampleByDocuments(10))

      By removing the redundant "Sample" prefix in the builder function name, the code now reads much more cleanly like a sentence

      // New Way: Read "Sample" "By Documents"
      pipeline.Sample(ByDocuments(10))
    2. Conciseness and Clarity
      Renaming SampleSpec to Sampler represents idiomatic Go naming conventions. Interfaces or types that perform a single conceptual action or "sample" elements are standardly suffixed with -er (like Reader, Writer, Sampler) rather than -Spec.

    3. Exposing ByPercentage consistently
      In addition to rewriting SampleByDocuments to ByDocuments, the new API now cleanly supports ByPercentage directly too:

      // Old Way:
      pipeline.Sample(&SampleSpec{Size: 0.6, Mode: SampleModePercent})
      
      // New Way: 
      pipeline.Sample(ByPercentage(0.6))

      This is much more expressive than forced instantiation for percentage configurations.

  4. Refactored the rawstage
    The RawStage refactoring represents a significant shift from a builder pattern (which is Java-idiomatic) to a direct method invocation with variadic arguments (which is Go-idiomatic). Here is a detailed breakdown of the rationale:
    1. Removing Public API Bloat
      In the previous implementation, RawStage was a publicly exported struct (type RawStage struct). This forced the SDK to export a constructor NewRawStage(), and builder methods like WithArguments() and WithOptions().
      The new API makes rawStage internal (lowercase r). The user now simply calls a direct method on the pipeline (pipeline.RawStage(...)). There are no superfluous constructors or builder methods exposed on the documentation surface.

    2. Idiomatic Go (Direct Method Calls)
      Go prefers passing configuring arguments directly into a method rather than instantiating builder objects. Compare the old chained instantiation against the new design:
      Old Way (Java Builder style):

      client.Pipeline().Collection("books").
            RawStage(
                  NewRawStage("limit").WithArguments(3),
       )

      New Way (Idiomatic Go):

      client.Pipeline().Collection("books").RawStage("limit", []any{3})

      The new syntax is much shorter, easier to read, and accurately describes the intent without boilerplate.

    3. Streamlining Options with Variadics
      The new method RawStage(name string, args []any, opts ...RawStageOptions) safely takes variadic pointers for options. If no options are needed, the caller simply omits them. If they are needed, they can supply any amount of RawStageOptions without calling unique chain builders.

    4. Architectural Alignment across the SDK
      This aligns RawStage with the rest of the module's recent refactors (shifting SampleSpec to Sampler, and *UnnestOptions to ...UnnestOption). It unifies the builder's syntax so developers maintain a standard mental model when chained expressions are invoked.

  5. Refactored the AddFields, RemoveFields and Select stages to expect at least one arg similar to Java and Node

@bhshkh bhshkh requested review from a team as code owners March 25, 2026 07:44
@bhshkh bhshkh enabled auto-merge (squash) March 25, 2026 07:44
@bhshkh bhshkh changed the title Fspq fix stages fix(firestore): Change stages to match Java Mar 25, 2026
@bhshkh bhshkh changed the title fix(firestore): Change stages to match Java fix(firestore): Change stages to match Java and Node Mar 25, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors several Firestore pipeline methods, including Select, AddFields, RemoveFields, Unnest, Sample, and RawStage, to improve their API design. Specifically, methods that accept variadic arguments now require at least one argument explicitly. The Unnest and Sample stages were updated to use an options pattern with interfaces and helper functions, replacing direct struct usage. The RawStage builder pattern was simplified to direct arguments. The errInvalidArg helper function was enhanced to include the stage name for clearer error messages. A critical issue was identified where the Select call in firestore/query.go could panic if the fields slice is empty, requiring a guard. Additionally, there is an opportunity to refactor duplicated code for processing UnnestOptions in the Unnest and UnnestWithAlias methods.

Comment thread firestore/query.go Outdated
Comment thread firestore/pipeline.go Outdated
@product-auto-label product-auto-label Bot added the api: firestore Issues related to the Firestore API. label Mar 25, 2026
Copy link
Copy Markdown
Contributor

@daniel-sanche daniel-sanche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gemini comment might be worth looking at. Otherwise LGTM

Copy link
Copy Markdown
Member

@hongalex hongalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit: otherwise LGTM

Comment thread firestore/pipeline.go
}

// RemoveFields removes fields from outputs from previous stages.
// fieldpaths can be a string or a [FieldPath] or an expression obtained by calling [FieldOf].
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make the argument name match the comment

Suggested change
// fieldpaths can be a string or a [FieldPath] or an expression obtained by calling [FieldOf].
// field can be a string or a [FieldPath] or an expression obtained by calling [FieldOf].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #14260

@bhshkh bhshkh merged commit 18946f1 into googleapis:main Mar 25, 2026
10 checks passed
@bhshkh bhshkh deleted the fspq-fix-stages branch March 25, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the Firestore API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants