[feat] ppl #1770
[feat] ppl #1770
Conversation
setup nix for idx
…uandy/PipelineResult
pipeline result
| @@ -29,6 +30,8 @@ | |||
| import com.google.api.gax.rpc.StreamController; | |||
| import com.google.api.gax.rpc.UnaryCallable; | |||
| import com.google.cloud.Timestamp; | |||
| import com.google.cloud.firestore.Transaction.AsyncFunction; | |||
There was a problem hiding this comment.
nit: I am inclined not to import nested classes. In this case, the qualifier Transaction helps understand that this is not a lambda, such as java.util.function.Function.
| * }</pre> | ||
| */ | ||
| @BetaApi | ||
| public final class Pipeline { |
There was a problem hiding this comment.
I think this class can be simplified. Here is a suggestion: #1786
There was a problem hiding this comment.
Thanks, that PR looks very helpful.
| } | ||
| if (response.getResultsCount() > 0) { | ||
| numDocuments += response.getResultsCount(); | ||
| if (numDocuments % 100 == 0) { |
There was a problem hiding this comment.
This condition seems wrong.
Maybe you should output trace when numDocuments > 100, and then reset numDocuments?
The trace message could add return numDocuments value instead of 100.
Also, text still says Firestore.Query and should be changed.
There was a problem hiding this comment.
Yeah, copy/paste from query does not work for pipeline here.
google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/stages/Stage.java
Show resolved
Hide resolved
| * @return A new {@code Expr} representing the addition operation. | ||
| */ | ||
| @BetaApi | ||
| default Add add(Object other) { |
There was a problem hiding this comment.
Ambiguous types in overloads cause problems. The danger is that an Expr is cast to Object causing wrong method to be called.
You could add:
| default Add add(Object other) { | |
| default Add add(Object other) { | |
| if (other instanceof Expr) { | |
| return add((Expr) other); | |
| } |
But can we instead come up with an API surface that doesn't require inspection of type?
There was a problem hiding this comment.
I don't think this is a problem. Constant.of should take care of the type inspection?
| @BetaApi | ||
| public static Constant ofVector(double[] value) { | ||
| // Convert double array to List<Double> | ||
| return new Constant(Arrays.stream(value).boxed().collect(Collectors.toList())); |
There was a problem hiding this comment.
Pull from main and use Marks work on VectorValues.
|
|
||
| @BetaApi | ||
| public static <T> Constant of(T[] value) { | ||
| return new Constant(Arrays.asList(value)); // Convert array to list |
There was a problem hiding this comment.
We should make a copy of array, thereby preventing accidental mutation.
| return new Constant(Arrays.asList(value)); // Convert array to list | |
| return new Constant(Arrays.asList(value.clone())); // Convert array to list |
| private final String name; | ||
| private final List<Expr> params; | ||
|
|
||
| protected Function(String name, List<? extends Expr> params) { |
There was a problem hiding this comment.
I think we should prefer using ImmutableList instead of wrapping with unmodifiableList. Using ImmutableList prevents both upstream changes as well as downstream changes. Whereas, simply wrapping a list, still allows the caller to modify the list. Alternatively, we can always makes copies, but that seems wasteful.
The customer facing API can still use a mutable List as parameter, but we should always make a copy internally into a ImmutableList internally. Once we have an ImmutableList, we no longer need to make copies, and can safely pass them around by reference.
| protected Function(String name, List<? extends Expr> params) { | |
| protected Function(String name, ImmutableList<? extends Expr> params) { |
# Conflicts: # README.md # google-cloud-firestore/pom.xml # google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java # proto-google-cloud-firestore-v1/src/main/java/com/google/firestore/v1/DocumentProto.java
…vaPplPP # Conflicts: # README.md
| @@ -29,6 +30,8 @@ | |||
| import com.google.api.gax.rpc.StreamController; | |||
| import com.google.api.gax.rpc.UnaryCallable; | |||
| import com.google.cloud.Timestamp; | |||
| import com.google.cloud.firestore.Transaction.AsyncFunction; | |||
| * }</pre> | ||
| */ | ||
| @BetaApi | ||
| public final class Pipeline { |
There was a problem hiding this comment.
Thanks, that PR looks very helpful.
| } | ||
| if (response.getResultsCount() > 0) { | ||
| numDocuments += response.getResultsCount(); | ||
| if (numDocuments % 100 == 0) { |
There was a problem hiding this comment.
Yeah, copy/paste from query does not work for pipeline here.
|
|
||
| @BetaApi | ||
| public static <T> Constant of(T[] value) { | ||
| return new Constant(Arrays.asList(value)); // Convert array to list |
| * @return A new {@code Expr} representing the addition operation. | ||
| */ | ||
| @BetaApi | ||
| default Add add(Object other) { |
There was a problem hiding this comment.
I don't think this is a problem. Constant.of should take care of the type inspection?
| private final String name; | ||
| private final List<Expr> params; | ||
|
|
||
| protected Function(String name, List<? extends Expr> params) { |
google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/stages/Stage.java
Show resolved
Hide resolved
# Conflicts: # google-cloud-firestore/src/main/java/com/google/cloud/firestore/Pipeline.java
* Implement new stages. * Replace toList() with older languages feature. * Pretty * Pretty * Comment out under development API surface * Pretty
* Refactor pipelines * Pretty * Simplify
No description provided.