Conversation
… aggregation helpers PipelineFunctions.arrayMaximum, arrayMaximumN, arrayMinimum, arrayMinimumN and arraySum emitted array_maximum, array_maximum_n, array_minimum, array_minimum_n and array_sum, which the backend rejects with INVALID_ARGUMENT. Node and the fluent PipelineExpression methods emit maximum, maximum_n, minimum, minimum_n and sum. The static helpers now delegate to maximum/maximumN/minimum/minimumN/sum, and the fluent methods call the same-named static helpers, so both forms share one code path.
addFields sent each aliased expression as its own 'alias' function-call argument, which the backend rejects: 'add_fields' takes exactly one MapValue keyed by alias. Build it with the same projection map used by select, distinct and aggregate, matching the Node SDK.
The backend collection_group(ancestor, collection_id) stage requires two arguments. PipelineSource.collectionGroup (and createFrom on a collection group query) sent only the collection id, failing with INVALID_ARGUMENT. Prepend an empty referenceValue naming the database root, matching the Node SDK's CollectionGroupSource.
The backend rejects 'double' for is_type; the accepted name (and the Node SDK Type union) is 'float64'. Also add the remaining members of the Node Type union that the backend accepts: int32, decimal128, maxKey, minKey, objectId and regex.
The backend only accepts join(array, delimiter) (or a 3-arg null_text
variant the Node SDK does not expose), so the optional separator made
PipelineFunctions.join('tags') fail with INVALID_ARGUMENT. Make the
delimiter a required positional parameter, matching
PipelineExpression.join and the Node SDK.
PipelineResult.get only did a flat top-level lookup and only accepted a
String, so get('metadata.lang') returned null. It now accepts a String or
FieldPath, validates it like DocumentSnapshot.get, and walks nested maps,
matching the Node SDK. Missing segments or non-map intermediates yield null.
…e and distinct
Pipeline.select, aggregate (accumulators and groups) and distinct built
their stage maps with Map.fromEntries, so a repeated field name or alias
silently replaced the earlier entry. Mirror the Node SDK's
selectablesToObject / aliasedAggregateToMap and throw an ArgumentError
("Duplicate alias or field '<name>'.") at stage-construction time. A
plain field name collides with an alias of the same name; accumulators
and groups are checked independently, as in Node.
PipelineExpression.substring and substringLiteral named their second argument `end` and required it, although the value is sent to the backend as a length (as in PipelineFunctions.substring and the Node.js SDK). Rename to (position, [length]), make length optional so it is omitted from the wire when null, and document that it is a count, not an end index like String.substring.
The top-level variable() helper was defined next to field() and constant() and documented in the README, but missing from the barrel's show list, so it was unreachable from package:google_cloud_firestore. Export it, mirroring the Node.js SDK's top-level variable(), and add a regression test that uses it via the public import.
PipelineExpression.round() took no argument, unlike PipelineFunctions.round, PipelineExpression.trunc and Node's Expression.round(decimalPlaces?). Add an optional [decimalPlaces] (number or expression) that forwards to PipelineFunctions.round, encoding round(x) or round(x, places) as Node does. Also rename PipelineExpression.trunc's positional parameter from decimals to decimalPlaces (non-breaking, matches Node and the static form) and forward it to PipelineFunctions.trunc instead of building the raw function by hand.
PipelineFunctions.split took an optional delimiter and dropped it when null, emitting a one-argument split the backend rejects. Make it a required positional argument like PipelineExpression.split and the Node SDK, and always send it (a null delimiter encodes as a null constant).
PipelineFunctions.mapRemove and PipelineExpression.mapRemove took an
Iterable of keys and spread them into a variadic map_remove call. The
backend contract (and the Node SDK) is map_remove(map, key) with exactly
one key, so both now take a single String or expression key and reject
an Iterable with an ArgumentError. Chain calls to remove several keys.
BREAKING CHANGE: mapRemove(['a']) becomes mapRemove('a').
…nctions cosineDistance, dotProduct, euclideanDistance and the findNearest stage encoded a plain list of numbers as an ARRAY, which the backend rejects with "requires `Vector` but got `ARRAY`". Mirror the Node SDK's vectorToExpr and convert numeric iterables to a VectorValue, keeping VectorValue and expression arguments as they were.
addFields now builds its map with the shared projection helper, so it inherits the duplicate alias check select, aggregate and distinct gained, matching the Node SDK's selectablesToMap.
…ions
Lists and maps passed in a value position were encoded as literal
ArrayValue/MapValue messages, which the backend rejects when they hold
an expression, e.g. PipelineFunctions.equalAny('rating', [field('score'), 5])
failed with "Value type is not supported: FIELD_REFERENCE_VALUE".
Mirror the Node SDK's valueToDefaultExpr: every catalog helper (static,
fluent and top-level) now sends an Iterable as an array(...) function and
a Map as a map(...) function, recursively. equalAny, notEqualAny,
arrayContainsAll and arrayContainsAny share one search-space helper that,
like Node, keeps a list of plain values as a literal array and switches
to array(...) only when it holds an expression, so the static and fluent
forms now encode identically. constant(...) and the raw function
builders are unchanged.
Lyokone
added this pull request to stack #338
October 2, 2026 13:08
Lyokone
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request aligns the Firestore Pipeline API with the Node Admin SDK, introducing breaking changes to 'join', 'split', and 'mapRemove', fixing backend function names for array operations, and adding support for nested paths in 'PipelineResult.get'. It also adds validation to prevent duplicate fields or aliases. The review feedback highlights a compile-time error in '_projectionMap' where 'ArgumentError' is incorrectly called with two arguments, and suggests a more robust type check ('is! Map') in 'PipelineResult.get' to handle maps reified with different type parameters.
| for (final selection in selections) { | ||
| final MapEntry(:key, :value) = _projectionEntry(selection); | ||
| if (result.containsKey(key)) { | ||
| throw ArgumentError("Duplicate alias or field '$key'.", argumentName); |
Contributor
There was a problem hiding this comment.
The default constructor for ArgumentError only accepts a single message argument. Passing a second argument (argumentName) will result in a compile-time error. To associate the error with a specific argument name, use ArgumentError.value instead.
throw ArgumentError.value(\n selection,\n argumentName,\n \"Duplicate alias or field '$key'.\",\n );| Object? get(Object field) { | ||
| Object? value = _data; | ||
| for (final segment in FieldPath.from(field).segments) { | ||
| if (value is! Map<String, Object?>) return null; |
Contributor
There was a problem hiding this comment.
Using value is! Map<String, Object?> can cause unexpected null returns if a nested map is reified as Map<dynamic, dynamic> or another map type (since Map<dynamic, dynamic> is not a subtype of Map<String, Object?> in Dart). Checking value is! Map is safer and more robust for defensive programming.
if (value is! Map) return null;The backend reads a select alias as a field path, so aliases with spaces were rejected with INVALID_ARGUMENT.
Coverage Report✅ Coverage 75.72% meets 40% threshold Total Coverage: 75.72% Package Breakdown
Minimum threshold: 40% |
Checking for Map<String, Object?> would return null for a nested map reified with other type arguments.
Lyokone
marked this pull request as ready for review
October 2, 2026 14:06
dlarocque
approved these changes
Oct 2, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes all 14 findings from the Firestore Pipelines review. Each fix is checked against the Node Admin SDK (
@google-cloud/firestore9.3.1) and is a separate commit.Requests the backend rejected
PipelineFunctions.arrayMaximum,arrayMaximumN,arrayMinimum,arrayMinimumNandarraySumnow sendmaximum,maximum_n,minimum,minimum_nandsum. They used to sendarray_*names.addFieldssends a single MapValue keyed by alias.collectionGroup, andcreateFromon a collection group query, send the leading rootreferenceValue: ''.PipelineValueType.doubleis sent asfloat64. Addedint32,decimal128,maxKey,minKey,objectIdandregex.cosineDistance,dotProduct,euclideanDistanceorfindNearestis sent as a VectorValue.PipelineFunctions.joinandsplitrequire their delimiter.array(...)/map(...), as Node'svalueToDefaultExprdoes. This coversequalAny,notEqualAny,arrayContains*,mapMergeand the other value positions. The static and fluent forms now encode identically.Behaviour and parity
PipelineResult.getaccepts aStringor aFieldPathand resolves nested paths, the same way asDocumentSnapshot.get.select,addFields,aggregateanddistinctthrow anArgumentErroron a duplicate field or alias. They used to keep the last one silently.PipelineExpression.substringandsubstringLiteraltake(position, [length]).PipelineExpression.roundtakes[decimalPlaces].PipelineExpression.trunc's parameter is renamed todecimalPlaces. It is positional, so callers are unaffected.PipelineFunctions.mapRemoveandPipelineExpression.mapRemovetake a single key.variable()is exported.Breaking changes
PipelineFunctions.join(array)andPipelineFunctions.split(value)no longer compile without a delimiter. The backend always rejected these calls.mapRemove(Iterable keys)becomesmapRemove(key). Node only supports a single key. To remove several keys, chain.mapRemove('a').mapRemove('b').Testing
dart analyzeis clean across the workspace.dart testingoogle_cloud_firestorepasses.bundle_test.dartfailures. They also fail onmainbecause the tests try to reachmetadata.google.internal.test/e2e/pipeline_e2e_test.darthas new cases that mirror each repro from the review. They pass against the live database in theE2E Pipelineworkflow.