Skip to content

Commit 18946f1

Browse files
bhshkhbhshkh
authored
Fspq fix stages (#14253)
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: ```go // 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 ```go // 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: ```go // 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. 5. 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): ```go client.Pipeline().Collection("books"). RawStage( NewRawStage("limit").WithArguments(3), ) ``` New Way (Idiomatic Go): ```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. 6. Refactored the AddFields, RemoveFields and Select stages to expect at least one arg similar to Java and Node
1 parent 5bf0cf4 commit 18946f1

File tree

5 files changed

+124
-94
lines changed

5 files changed

+124
-94
lines changed

‎firestore/pipeline.go‎

Lines changed: 96 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -323,11 +323,12 @@ func (p *Pipeline) Offset(offset int) *Pipeline {
323323
//
324324
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
325325
// regardless of any other documented package stability guarantees.
326-
func (p *Pipeline) Select(fieldpathsOrSelectables ...any) *Pipeline {
326+
func (p *Pipeline) Select(fieldpathOrSelectable any, fieldpathsOrSelectables ...any) *Pipeline {
327327
if p.err != nil {
328328
return p
329329
}
330-
stage, err := newSelectStage(fieldpathsOrSelectables...)
330+
all := append([]any{fieldpathOrSelectable}, fieldpathsOrSelectables...)
331+
stage, err := newSelectStage(all...)
331332
if err != nil {
332333
p.err = err
333334
return p
@@ -364,11 +365,12 @@ func (p *Pipeline) Distinct(fieldpathsOrSelectables ...any) *Pipeline {
364365
//
365366
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
366367
// regardless of any other documented package stability guarantees.
367-
func (p *Pipeline) AddFields(selectables ...Selectable) *Pipeline {
368+
func (p *Pipeline) AddFields(selectable Selectable, selectables ...Selectable) *Pipeline {
368369
if p.err != nil {
369370
return p
370371
}
371-
stage, err := newAddFieldsStage(selectables...)
372+
all := append([]Selectable{selectable}, selectables...)
373+
stage, err := newAddFieldsStage(all...)
372374
if err != nil {
373375
p.err = err
374376
return p
@@ -377,14 +379,16 @@ func (p *Pipeline) AddFields(selectables ...Selectable) *Pipeline {
377379
}
378380

379381
// RemoveFields removes fields from outputs from previous stages.
382+
// fieldpaths can be a string or a [FieldPath] or an expression obtained by calling [FieldOf].
380383
//
381384
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
382385
// regardless of any other documented package stability guarantees.
383-
func (p *Pipeline) RemoveFields(fieldpaths ...any) *Pipeline {
386+
func (p *Pipeline) RemoveFields(field any, fields ...any) *Pipeline {
384387
if p.err != nil {
385388
return p
386389
}
387-
stage, err := newRemoveFieldsStage(fieldpaths...)
390+
all := append([]any{field}, fields...)
391+
stage, err := newRemoveFieldsStage(all...)
388392
if err != nil {
389393
p.err = err
390394
return p
@@ -487,13 +491,51 @@ func (p *Pipeline) AggregateWithSpec(spec *AggregateSpec) *Pipeline {
487491
return p.append(aggStage)
488492
}
489493

490-
// UnnestOptions holds the configuration for the Unnest stage.
494+
// unnestSettings holds the configuration for the Unnest stage.
495+
type unnestSettings struct {
496+
IndexField any
497+
}
498+
499+
// UnnestOption is an option for executing a pipeline unnest stage.
491500
//
492501
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
493502
// regardless of any other documented package stability guarantees.
494-
type UnnestOptions struct {
495-
// IndexField specifies the name of the field to store the array index of the unnested element.
496-
IndexField any
503+
type UnnestOption interface {
504+
apply(*unnestSettings)
505+
}
506+
507+
type funcUnnestOption struct {
508+
f func(*unnestSettings)
509+
}
510+
511+
func (fuo *funcUnnestOption) apply(uo *unnestSettings) {
512+
fuo.f(uo)
513+
}
514+
515+
func newFuncUnnestOption(f func(*unnestSettings)) *funcUnnestOption {
516+
return &funcUnnestOption{
517+
f: f,
518+
}
519+
}
520+
521+
// WithUnnestIndexField specifies the name of the field to store the array index of the unnested element.
522+
//
523+
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
524+
// regardless of any other documented package stability guarantees.
525+
func WithUnnestIndexField(indexField any) UnnestOption {
526+
return newFuncUnnestOption(func(uo *unnestSettings) {
527+
uo.IndexField = indexField
528+
})
529+
}
530+
531+
func processUnnestOptions(opts ...UnnestOption) *unnestSettings {
532+
settings := &unnestSettings{}
533+
for _, opt := range opts {
534+
if opt != nil {
535+
opt.apply(settings)
536+
}
537+
}
538+
return settings
497539
}
498540

499541
// Unnest produces a document for each element in an array field.
@@ -504,11 +546,12 @@ type UnnestOptions struct {
504546
//
505547
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
506548
// regardless of any other documented package stability guarantees.
507-
func (p *Pipeline) Unnest(field Selectable, opts *UnnestOptions) *Pipeline {
549+
func (p *Pipeline) Unnest(field Selectable, opts ...UnnestOption) *Pipeline {
508550
if p.err != nil {
509551
return p
510552
}
511-
stage, err := newUnnestStageFromSelectable(field, opts)
553+
settings := processUnnestOptions(opts...)
554+
stage, err := newUnnestStage("Unnest", field, settings)
512555
if err != nil {
513556
p.err = err
514557
return p
@@ -521,7 +564,7 @@ func (p *Pipeline) Unnest(field Selectable, opts *UnnestOptions) *Pipeline {
521564
//
522565
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
523566
// regardless of any other documented package stability guarantees.
524-
func (p *Pipeline) UnnestWithAlias(fieldpath any, alias string, opts *UnnestOptions) *Pipeline {
567+
func (p *Pipeline) UnnestWithAlias(fieldpath any, alias string, opts ...UnnestOption) *Pipeline {
525568
if p.err != nil {
526569
return p
527570
}
@@ -533,11 +576,12 @@ func (p *Pipeline) UnnestWithAlias(fieldpath any, alias string, opts *UnnestOpti
533576
case FieldPath:
534577
fieldExpr = FieldOf(v)
535578
default:
536-
p.err = errInvalidArg(fieldpath, "string", "FieldPath")
579+
p.err = errInvalidArg("UnnestWithAlias", fieldpath, "string", "FieldPath")
537580
return p
538581
}
539582

540-
stage, err := newUnnestStage(fieldExpr, alias, opts)
583+
settings := processUnnestOptions(opts...)
584+
stage, err := newUnnestStage("UnnestWithAlias", fieldExpr.As(alias), settings)
541585
if err != nil {
542586
p.err = err
543587
return p
@@ -584,43 +628,51 @@ const (
584628
SampleModePercent SampleMode = "percent"
585629
)
586630

587-
// SampleSpec is used to define a sample operation.
631+
// Sampler is used to define a sample operation.
588632
//
589633
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
590634
// regardless of any other documented package stability guarantees.
591-
type SampleSpec struct {
635+
type Sampler struct {
592636
Size any
593637
Mode SampleMode
594638
}
595639

596-
// SampleByDocuments creates a SampleSpec for sampling a fixed number of documents.
640+
// ByDocuments creates a Sampler for sampling a fixed number of documents.
597641
//
598642
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
599643
// regardless of any other documented package stability guarantees.
600-
func SampleByDocuments(limit int) *SampleSpec {
601-
return &SampleSpec{Size: limit, Mode: SampleModeDocuments}
644+
func ByDocuments(limit int) *Sampler {
645+
return &Sampler{Size: limit, Mode: SampleModeDocuments}
646+
}
647+
648+
// ByPercentage creates a Sampler for sampling a percentage of documents.
649+
//
650+
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
651+
// regardless of any other documented package stability guarantees.
652+
func ByPercentage(percentage float64) *Sampler {
653+
return &Sampler{Size: percentage, Mode: SampleModePercent}
602654
}
603655

604656
// Sample performs a pseudo-random sampling of the documents from the previous stage.
605657
//
606-
// This stage will filter documents pseudo-randomly. The behavior is defined by the SampleSpec.
607-
// Use SampleByDocuments or SampleByPercentage to create a SampleSpec.
658+
// This stage will filter documents pseudo-randomly. The behavior is defined by the Sampler.
659+
// Use ByDocuments or ByPercentage to create a Sampler.
608660
//
609661
// Example:
610662
//
611663
// // Sample 10 books, if available.
612-
// client.Pipeline().Collection("books").Sample(SampleByDocuments(10))
664+
// client.Pipeline().Collection("books").Sample(ByDocuments(10))
613665
//
614666
// // Sample 50% of books.
615-
// client.Pipeline().Collection("books").Sample(&SampleSpec{Size: 0.5, Mode: SampleModePercent})
667+
// client.Pipeline().Collection("books").Sample(ByPercentage(0.5))
616668
//
617669
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
618670
// regardless of any other documented package stability guarantees.
619-
func (p *Pipeline) Sample(spec *SampleSpec) *Pipeline {
671+
func (p *Pipeline) Sample(sampler *Sampler) *Pipeline {
620672
if p.err != nil {
621673
return p
622674
}
623-
stage, err := newSampleStage(spec)
675+
stage, err := newSampleStage(sampler)
624676
if err != nil {
625677
p.err = err
626678
return p
@@ -709,19 +761,30 @@ func (p *Pipeline) FindNearest(vectorField any, queryVector any, measure Pipelin
709761
//
710762
// // Assume we don't have a built-in "where" stage
711763
// client.Pipeline().Collection("books").
712-
// RawStage(
713-
// NewRawStage("where").
714-
// WithArguments(
715-
// LessThan(FieldOf("published"), 1900),
716-
// ),
717-
// ).
764+
// RawStage("where", []any{LessThan(FieldOf("published"), 1900)}).
718765
// Select("title", "author")
719766
//
720767
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
721768
// regardless of any other documented package stability guarantees.
722-
func (p *Pipeline) RawStage(stage *RawStage) *Pipeline {
769+
func (p *Pipeline) RawStage(name string, args []any, opts ...RawStageOptions) *Pipeline {
723770
if p.err != nil {
724771
return p
725772
}
773+
774+
var mergedOptions RawStageOptions
775+
if len(opts) > 0 {
776+
mergedOptions = make(RawStageOptions)
777+
for _, opt := range opts {
778+
for k, v := range opt {
779+
mergedOptions[k] = v
780+
}
781+
}
782+
}
783+
784+
stage := &rawStage{
785+
stageName: name,
786+
args: args,
787+
options: mergedOptions,
788+
}
726789
return p.append(stage)
727790
}

‎firestore/pipeline_integration_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -499,7 +499,7 @@ func TestIntegration_PipelineStages(t *testing.T) {
499499
})
500500
t.Run("RawStage", func(t *testing.T) {
501501
// Using RawStage to perform a Limit operation
502-
iter := client.Pipeline().Collection(coll.ID).RawStage(NewRawStage("limit").WithArguments(3)).Execute(ctx).Results()
502+
iter := client.Pipeline().Collection(coll.ID).RawStage("limit", []any{3}).Execute(ctx).Results()
503503
defer iter.Stop()
504504
results, err := iter.GetAll()
505505
if err != nil {
@@ -510,7 +510,7 @@ func TestIntegration_PipelineStages(t *testing.T) {
510510
}
511511

512512
// Using RawStage to perform a Select operation with options
513-
iter = client.Pipeline().Collection(coll.ID).RawStage(NewRawStage("select").WithArguments(map[string]interface{}{"title": FieldOf("title")})).Limit(1).Execute(ctx).Results()
513+
iter = client.Pipeline().Collection(coll.ID).RawStage("select", []any{map[string]interface{}{"title": FieldOf("title")}}).Limit(1).Execute(ctx).Results()
514514
defer iter.Stop()
515515
doc, err := iter.Next()
516516
if err != nil {
@@ -582,7 +582,7 @@ func TestIntegration_PipelineStages(t *testing.T) {
582582
})
583583
t.Run("Sample", func(t *testing.T) {
584584
t.Run("SampleByDocuments", func(t *testing.T) {
585-
iter := client.Pipeline().Collection(coll.ID).Sample(SampleByDocuments(5)).Execute(ctx).Results()
585+
iter := client.Pipeline().Collection(coll.ID).Sample(ByDocuments(5)).Execute(ctx).Results()
586586
defer iter.Stop()
587587
var got []map[string]interface{}
588588
for {
@@ -604,7 +604,7 @@ func TestIntegration_PipelineStages(t *testing.T) {
604604
}
605605
})
606606
t.Run("SampleByPercentage", func(t *testing.T) {
607-
iter := client.Pipeline().Collection(coll.ID).Sample(&SampleSpec{Size: 0.6, Mode: SampleModePercent}).Execute(ctx).Results()
607+
iter := client.Pipeline().Collection(coll.ID).Sample(ByPercentage(0.6)).Execute(ctx).Results()
608608
defer iter.Stop()
609609
var got []map[string]interface{}
610610
for {
@@ -797,7 +797,7 @@ func TestIntegration_PipelineStages(t *testing.T) {
797797
t.Run("UnnestWithIndexField", func(t *testing.T) {
798798
iter := client.Pipeline().Collection(coll.ID).
799799
Where(Equal(FieldOf("title"), "The Hitchhiker's Guide to the Galaxy")).
800-
UnnestWithAlias("tags", "tag", &UnnestOptions{IndexField: "tagIndex"}).
800+
UnnestWithAlias("tags", "tag", WithUnnestIndexField("tagIndex")).
801801
Select("title", "tag", "tagIndex").
802802
Execute(ctx).Results()
803803
defer iter.Stop()

0 commit comments

Comments
 (0)