Skip to content

Conversation

@wu-hui
Copy link
Contributor

@wu-hui wu-hui commented Dec 12, 2025

No description provided.

@wu-hui wu-hui requested review from a team as code owners December 12, 2025 15:54
@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Dec 12, 2025
@generated-files-bot
Copy link

generated-files-bot bot commented Dec 12, 2025

Warning: This pull request is touching the following templated files:

  • google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1/stub/FirestoreStubSettings.java

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Dec 12, 2025
@wu-hui wu-hui force-pushed the feat/pipeline/private-preview branch 3 times, most recently from 3aece03 to bd317e2 Compare December 12, 2025 20:59
@wu-hui wu-hui force-pushed the feat/pipeline/private-preview branch from f5e464b to 651bd3d Compare December 12, 2025 21:09
@wu-hui wu-hui force-pushed the feat/pipeline/private-preview branch from 651bd3d to 6ae12ec Compare December 12, 2025 21:10
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.

PR title will be in the change log, so be sure to update this

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.

Initial review. It looks like there are a few API changes that need to be ported from Android

* documents, execution time, and explain stats.
*/
@BetaApi
public static final class Snapshot {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question on this SDK about Pipeline.Snapshot vs PipelineSnapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replied from android review.

* @return A new Pipeline object with this stage appended to the stage list.
*/
@BetaApi
public Pipeline addFields(Selectable... fields) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not have the same signature as Android: fun addFields(field: Selectable, vararg additionalFields: Selectable) ...

It looks like this is true throughout the API for adding stages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

// @BetaApi
// public Pipeline unnest(Selectable field) {
// return append(new Unnest(field));
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this API be added for feature parity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one was a duplicate of below, deleted it instead.

// @BetaApi
// public Pipeline unnest(Selectable field, UnnestOptions options) {
// return append(new Unnest(field, options));
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

same, should this be uncommented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,456 @@
/*
* Copyright 2017 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

update date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually we also want to javadoc the apis in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

eventual javadoc

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 this might be just internal.

Copy link
Contributor

Choose a reason for hiding this comment

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

eventual javadoc

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to port BooleanExpression changes from Android. same probably for nodejs-firestore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we should.

* @return A new {@link Expression} representing the generic function.
*/
@BetaApi
public static Expression generic(String name, Expression... expr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to rawExpression

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

public AggregateHints withForceStreamableEnabled() {
return with("force_streamable", true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this hint, are we sure this is current?

Copy link
Contributor

Choose a reason for hiding this comment

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

force_streamable is not listed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

}

public CollectionHints withIgnoreIndexFields(String... values) {
return with("ignore_index_fields", values);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also no longer listed, that i'm aware of

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

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.

Another round of comments on the remaining files. I think we need to determine what to do with the differences in the API before release

<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-firestore-bom</artifactId>
<!-- Tell the application to use the private preview BOM of cloud SDK -->
<version>99.99.0-PRIVATEPREVIEW</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this version need to be updated for the release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

this.firestore = firestore;
}

void queryExplain() throws ExecutionException, InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

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

This snippet does not appear related to pipelines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, this is Morgan's change, i suppose it does not matter.

tools/api.txt Outdated
method @NonNull public static com.google.cloud.firestore.AggregateField.AverageAggregateField average(@NonNull com.google.cloud.firestore.FieldPath);
method @NonNull public static com.google.cloud.firestore.AggregateField.AverageAggregateField average(@NonNull String);
method @NonNull public static com.google.cloud.firestore.AggregateField.CountAggregateField count();
method public static com.google.cloud.firestore.AggregateField.AverageAggregateField average(com.google.cloud.firestore.FieldPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about @nonnull attribute being dropped throughout the API. I can't determine if this is only dropped in this api.txt, or if there was some config / source change. If it was actually dropped from the API surface, that would be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. This api.txt is just for convenience to conduct api audit using gemini, it is not used anywhere else.

* @return An {@link Expression} constant instance.
*/
@BetaApi
public static Expression vector(VectorValue value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure we dropped this way to create a constant from a vector, because it is also listed above Expression constant(VectorValue vector)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* @return An {@link Expression} constant instance.
*/
@BetaApi
public static Expression vector(double[] value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we dropped both overloads of vector(x) to create a constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@wu-hui wu-hui changed the title feat: ppl pri-pre feat: Pipelines enters beta Jan 2, 2026
@wu-hui wu-hui changed the title feat: Pipelines enters beta feat: Pipelines enters public preview Jan 2, 2026
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: xl Pull request size is extra large. labels Jan 2, 2026
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/java-firestore API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants