Conversation
product-auto-label
bot
added
the
api: firestore
This reverts commit 9edc6af.
dlarocque
force-pushed
the
dl/subq
branch
2 times, most recently
from
850ba24 to
dd1a205
Compare
cherylEnkidu
left a comment
cherylEnkidu
left a comment
There was a problem hiding this comment.
can you add this test as well: https://github.com/googleapis/java-firestore/pull/2323/changes#diff-566df7e9507530a3a2b0c886b9408ac028ed2f741506e4ed33a3b4075ea36672R986
Also implement the check when calling subcollection in Union stage
MarkDuckworth
left a comment
MarkDuckworth
left a comment
There was a problem hiding this comment.
LGTM with suggestions for ref doc cleanup
|
|
||
| /** | ||
| * @public | ||
| * Creates an expression that returns the value of a field from a document that results from the evaluation of the expression. |
There was a problem hiding this comment.
consider something like
Creates an expression that gets a field from a map (object).
Firestore documentation uses "map" to refer to the data type, not document/sub-document.
There was a problem hiding this comment.
| * getField(field("address"), "city") | ||
| * ``` | ||
| * | ||
| * @param expression The expression representing the document. |
There was a problem hiding this comment.
"an expression evaluating to the map from which the field will be extracted"
| * firestore.pipeline().collection("books") | ||
| * .define(currentDocument().as("doc")) | ||
| * // Access a field from the defined document variable | ||
| * .select(variable("doc").mapGet("title")); |
There was a problem hiding this comment.
should we use getField('title') instead of mapGet?
|
|
||
| /** | ||
| * @public | ||
| * Defines one or more variables in the pipeline's scope. `define` is used to bind a value to a |
There was a problem hiding this comment.
I still think it would be valuable to state that it binds the variable to the pipeline's current document
For example considering the following
db.pipeline().collection("foo")
.define(currentTimestamp().as("executionTime"))
.addField(variable("executionTime").as("executionTime"))
If the customer reads this documentation as "define binds a variable to the pipeline", they they may assume that .define(currentTimestamp().as("executionTime")) is executed once for the pipeline.
But the reality is that .define(currentTimestamp().as("executionTime")) is executed for each document and binds a different value of executionTime for each document.
|
|
||
| /** | ||
| * @public | ||
| * Converts this Pipeline into an expression that evaluates to an array of results. |
There was a problem hiding this comment.
Consider "array of map (objects)" for consistent data types.
There was a problem hiding this comment.
"Converts this Pipeline into an expression that evaluates to an array of maps (objects), where each result document of the pipeline is represented as a map in the returned array."
| * .define(field("id").as("book_id")) | ||
| * .addFields( | ||
| * db.pipeline().collection("reviews") | ||
| * .where(field("book_id").equal(variable("book_id"))) |
There was a problem hiding this comment.
might clarify things to choose a field name that does not match the variable name.
...
.define(field("id").as("current_book_id"))
...
.where(field("book_id").equal(variable("current_book_id")))
| * // Calculate average rating for a restaurant | ||
| * db.pipeline().collection("restaurants").addFields( | ||
| * db.pipeline().collection("reviews") | ||
| * .where(field("restaurant_id").equal(variable("rid"))) |
There was a problem hiding this comment.
where is rid defined?
| * // Calculate average rating AND count for a restaurant | ||
| * db.pipeline().collection("restaurants").addFields( | ||
| * db.pipeline().collection("reviews") | ||
| * .where(field("restaurant_id").equal(variable("rid"))) |
There was a problem hiding this comment.
Any changes you decide to make in the ref doc above would also need to be changed here
MarkDuckworth
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
5 participants
Adds support for Pipeline subqueries.
Ported from: firebase/firebase-android-sdk#7736
API Proposal: go/firestore-pipelines-subquery