Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: New Pipeline Stages #2236

Merged
merged 11 commits into from
Jan 17, 2025
Merged

feature: New Pipeline Stages #2236

merged 11 commits into from
Jan 17, 2025

Conversation

tom-andersen
Copy link
Contributor

No description provided.

@tom-andersen tom-andersen requested review from a team as code owners November 26, 2024 18:21
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Nov 26, 2024
@tom-andersen tom-andersen changed the title New Pipeline Stages feature: New Pipeline Stages Nov 29, 2024
dev/src/pipeline.ts Outdated Show resolved Hide resolved
dev/src/pipeline.ts Outdated Show resolved Hide resolved
dev/src/pipeline.ts Outdated Show resolved Hide resolved
dev/src/pipeline.ts Outdated Show resolved Hide resolved
* @param fieldName The name of the field containing the array.
* @param options The {@code UnnestOptions} options.
* @return A new {@code Pipeline} object with this stage appended to the stage list.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document the use case for indexField? Or otherwise why someone would use this overload?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should focus on describing the mechanics of how the stage works, as opposed to why they may want to use this stage. The why belongs more in tutorials, guides or blogs, thereby keeping Stage descriptions more concise.

Here is the Stage Schemas as described by Query team: https://docs.google.com/document/d/1pvF6JH8Ulc2GSmFwC0XOjH1KdTZ_sg779XAN6NoRR7s/edit?resourcekey=0-gmt2NHpJrHPS-z4InY4ufw&tab=t.0#heading=h.7b8hsu3i0pnv

If we don't think there is value in an option/stage, we should raise it with Query team.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on documenting the mechanics. My comment was poorly worded, but I think my concern still stands. IMO, we don't clearly document what happens when indexField is set. At least it wasn't clear to me until I read the linked doc.

We could add something like this to the docs for this method or for UnnestOptions:

When indexField is provided, a new field is added to the emitted documents, storing the offset (starting at zero) into the array the expanded element is from.

types/firestore.d.ts Outdated Show resolved Hide resolved
dev/src/expression.ts Show resolved Hide resolved
dev/system-test/pipeline.ts Outdated Show resolved Hide resolved
dev/system-test/pipeline.ts Show resolved Hide resolved
Copy link
Contributor Author

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

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

I have updated PR. @MarkDuckworth, please take another look.

dev/src/expression.ts Show resolved Hide resolved
dev/src/pipeline.ts Outdated Show resolved Hide resolved
* @param fieldName The name of the field containing the array.
* @param options The {@code UnnestOptions} options.
* @return A new {@code Pipeline} object with this stage appended to the stage list.
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should focus on describing the mechanics of how the stage works, as opposed to why they may want to use this stage. The why belongs more in tutorials, guides or blogs, thereby keeping Stage descriptions more concise.

Here is the Stage Schemas as described by Query team: https://docs.google.com/document/d/1pvF6JH8Ulc2GSmFwC0XOjH1KdTZ_sg779XAN6NoRR7s/edit?resourcekey=0-gmt2NHpJrHPS-z4InY4ufw&tab=t.0#heading=h.7b8hsu3i0pnv

If we don't think there is value in an option/stage, we should raise it with Query team.

types/firestore.d.ts Outdated Show resolved Hide resolved
dev/system-test/pipeline.ts Outdated Show resolved Hide resolved
dev/src/pipeline.ts Outdated Show resolved Hide resolved
dev/system-test/pipeline.ts Show resolved Hide resolved
Copy link
Contributor

@MarkDuckworth MarkDuckworth left a comment

Choose a reason for hiding this comment

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

LGTM with one remaining comment about documentation for indexField.

dev/src/expression.ts Show resolved Hide resolved
* @param fieldName The name of the field containing the array.
* @param options The {@code UnnestOptions} options.
* @return A new {@code Pipeline} object with this stage appended to the stage list.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on documenting the mechanics. My comment was poorly worded, but I think my concern still stands. IMO, we don't clearly document what happens when indexField is set. At least it wasn't clear to me until I read the linked doc.

We could add something like this to the docs for this method or for UnnestOptions:

When indexField is provided, a new field is added to the emitted documents, storing the offset (starting at zero) into the array the expanded element is from.

dev/src/pipeline.ts Outdated Show resolved Hide resolved
@tom-andersen tom-andersen merged commit db8a43b into wuandy/PplPP Jan 17, 2025
11 of 13 checks passed
@tom-andersen tom-andersen deleted the tomandersen/newStages branch January 17, 2025 01:40
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 googleapis/nodejs-firestore API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants