feat: add support for Partition API #1320

schmidt-sebastian · 2020-10-01T18:29:52Z

This adds support for the Partition API.

codecov · 2020-10-02T21:05:35Z

Codecov Report

Merging #1320 into master will decrease coverage by 0.01%.
The diff coverage is 97.58%.

@@            Coverage Diff             @@
##           master    #1320      +/-   ##
==========================================
- Coverage   98.51%   98.49%   -0.02%     
==========================================
  Files          30       32       +2     
  Lines       18760    19111     +351     
  Branches     1451     1464      +13     
==========================================
+ Hits        18481    18823     +342     
- Misses        276      285       +9     
  Partials        3        3

Impacted Files	Coverage Δ
dev/src/collection-group.ts	`95.13% <95.13%> (ø)`
dev/src/bulk-writer.ts	`99.76% <100.00%> (ø)`
dev/src/document-change.ts	`100.00% <100.00%> (ø)`
dev/src/document.ts	`98.77% <100.00%> (ø)`
dev/src/field-value.ts	`97.06% <100.00%> (ø)`
dev/src/index.ts	`97.58% <100.00%> (+<0.01%)`	⬆️
dev/src/query-partition.ts	`100.00% <100.00%> (ø)`
dev/src/reference.ts	`99.89% <100.00%> (+<0.01%)`	⬆️
dev/src/transaction.ts	`97.03% <100.00%> (ø)`
dev/src/types.ts	`99.37% <100.00%> (+0.01%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b493baf...e8dcaa6. Read the comment docs.

thebrianchen

Mostly left some commenting nits along with some questions

thebrianchen · 2020-10-08T19:05:53Z

dev/src/query-partition.ts

+  constructor(
+    private readonly _firestore: Firestore,
+    private readonly _collectionId: string,
+    private readonly _converter:


Why does this converter have to be undefined here? QueryPartition's converter comes from QueryOptions, which always has FirestoreDataConverter defined.

Good catch. Simplified.

thebrianchen · 2020-10-08T19:29:34Z

dev/src/query-partition.ts

+  private _memoizedStartAt: unknown[] | undefined;
+  private _memoizedEndBefore: unknown[] | undefined;
+
+  constructor(


Add hideconstructor.

thebrianchen · 2020-10-08T21:40:31Z

dev/src/collection-group.ts

+   * @return {AsyncIterable<QueryPartition>} An AsyncIterable of
+   * `QueryPartition`s.
+   */
+  async *getPartitionsAsync(


getPartitionsAsync()'s name was initially confusing to me, since I thought once the non-async one would be a function that had a : void return signature. Both getPartitionsAsync() and getPartitions() are essentially async methods, which added to my confusion.

How about getPartititionsIterator and getPartitionsPromise, or something more along those lines (granted, those 2 names are the best)? Is there any other established pattern for methods that return an iterator vs. a promise?

This follows the precedent in the generated Gapic library. I am kind of leaning towards removing the "non-async" method (which is also async) altogether. Streaming iterators are very easy to use, so maybe there is no benefit of exposing both methods. I will ask for opinions before merging.

thebrianchen · 2020-10-08T21:52:32Z

dev/src/query-partition.ts

+   * The cursor that defines the first result for this partition or `undefined`
+   * if this is the first partition.
+   *
+   * @return a cursor value that can be used with {@link Query#startAt} or


Consider adding other annotations (@return, @type, etc.) like in other getters. As it stands, the docs file looks pretty bare: https://screenshot.googleplex.com/mNvFsQh4X2E2msJ.png.

Done. We should always do this for JSDoc.

thebrianchen · 2020-10-08T21:53:55Z

dev/src/query-partition.ts

+  /**
+   * Returns a query that only returns the documents for this partition.
+   *
+   * @return a query partitioned by a {@link Query#startAt} and {@link


Consider adding return type: {Query<T>}.

thebrianchen · 2020-10-08T22:26:15Z

dev/src/query-partition.ts

+   * The cursor that defines the first result for this partition or `undefined`
+   * if this is the first partition.
+   *
+   * @return a cursor value that can be used with {@link Query#startAt} or


uber-nit: s/a cursor/A cursor

thebrianchen · 2020-10-08T22:30:14Z

dev/src/query-partition.ts

+  }
+
+  /**
+   * Returns a query that only returns the documents for this partition.


Consider: Returns a query that only contains documents for/inside this partition.

Technically, a query doesn't "return" the documents.

Used encapsulates. Let me know what you think.

thebrianchen · 2020-10-08T22:39:26Z

dev/test/partition-query.ts

+  expect(actual).to.deep.eq(query);
+}
+
+export function result(documentId: string): api.IRunQueryResponse {


Delete unused function

thebrianchen · 2020-10-08T22:49:06Z

dev/src/query-partition.ts

+      queryOptions = queryOptions.with({
+        startAt: {before: true, values: this._startAt},
+      });
+    }


Not sure how important it is to test L105-108, but codecov is complaining about missing tests here. Flagging this in-case you missed it.

Added a test that should cover this.

thebrianchen · 2020-10-09T14:29:23Z

types/firestore.d.ts

+     * The cursor that defines the first result for this partition or
+     * `undefined` if this is the first partition.
+     *
+     * @return cursor values that can be used with {@link Query#startAt} or


The "destructure" comment was hard for me to follow, consider adding an example of how to do so properly?

Also, consider adding test for query.startAt(..queryPartition.start)?

Added examples for all methods and expanded on the description of the return type. Also added test

thebrianchen

lgtm, those were some pretty sleek tests

thebrianchen · 2020-10-11T13:31:39Z

dev/src/collection-group.ts

+/**
+ * A `CollectionGroup` refers to all documents that are contained in a
+ * collection or subcollection with a specific collection ID.
+ */


I think you need add the CollectionGroup after @class, or else the constructor will still appear. Not sure why, but I regenerated the docs and still see the constructor with your current PR.

thebrianchen · 2020-10-11T13:32:19Z

dev/src/query-partition.ts

+ * A split point that can be used in a query as a starting and/or end point for
+ * the query results. The cursors returned by {@link #startAt} and {@link
+ * #endBefore} can only be used in a query that matches the constraint of query
+ * that produced this partition.


same as above.

thebrianchen · 2020-10-11T13:45:31Z

dev/system-test/firestore.ts

@@ -194,6 +196,126 @@ describe('Firestore class', () => {
  });
 });

+describe('CollectionGroup class', () => {
+  const desiredPartitionCount = 3;
+  const documentCount = 2 * 128 + 127; // Minimum partition size is 128.


Btw, just caught this, but why is the minimum partition size 128? I didn't see this requirement elsewhere in the code. Also, is this something the developer needs to know?

This is not the partition size, but rather the minimum result set size in order for partitions to work. If a collection group has fewer than 128 documents, then it will not be partitioned. This is deemed acceptable with the current API as the provided partition count value is documented to be best effort.

schmidt-sebastian · 2020-10-12T16:54:54Z

@BenWhitehead Can you take a look at this as well?
@bcoe Do you think it is acceptable to only provide the AsyncIterator<QueryPartition> API or do we also need to have an API that returns Promise<QueryPartition[]>

BenWhitehead

Superficial LGTM.

The signatures look in line with the design set forward in the Java PR. But I don't know the knitty gritty of typescript so can't comment on that.

schmidt-sebastian · 2020-10-13T18:15:23Z

Discussed offline with @bcoe. We may add a listPartitions API that returns a Promise<QueryPartition[]> at a later point. This would match listDocuments and listCollections.

schmidt-sebastian requested review from a team as code owners October 1, 2020 18:29

schmidt-sebastian marked this pull request as draft October 1, 2020 18:29

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 1, 2020

product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Oct 2, 2020

feat: add support for Partition API

5438615

schmidt-sebastian force-pushed the mrschmidt/partitionapi branch from 4f8d7b4 to 5438615 Compare October 2, 2020 20:59

schmidt-sebastian changed the title ~~WIP: Partition API~~ feat: add support for Partition API Oct 2, 2020

schmidt-sebastian marked this pull request as ready for review October 2, 2020 21:01

Merge branch 'master' into mrschmidt/partitionapi

5a647d0

Lint

972c2b3

schmidt-sebastian requested a review from thebrianchen October 2, 2020 21:34

schmidt-sebastian assigned thebrianchen Oct 2, 2020

thebrianchen reviewed Oct 9, 2020

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Oct 9, 2020

Address feedback

ed023e2

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Oct 9, 2020

thebrianchen approved these changes Oct 11, 2020

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Oct 11, 2020

Add class names

ea5ccd3

BenWhitehead reviewed Oct 12, 2020

View reviewed changes

Fix test

e8dcaa6

schmidt-sebastian merged commit 51961c3 into master Oct 13, 2020

thebrianchen pushed a commit that referenced this pull request Oct 28, 2020

feat: add support for Partition API (#1320)

181a7a5

crwilcox mentioned this pull request Jun 3, 2021

feat(firestore): Add support for PartitionQuery googleapis/google-cloud-go#4206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for Partition API #1320

feat: add support for Partition API #1320

schmidt-sebastian commented Oct 1, 2020 •

edited

codecov bot commented Oct 2, 2020 •

edited

thebrianchen left a comment

thebrianchen Oct 8, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen Oct 8, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen Oct 8, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen Oct 8, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen Oct 8, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen Oct 8, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen Oct 8, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen Oct 8, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen Oct 8, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen Oct 9, 2020

schmidt-sebastian Oct 9, 2020

thebrianchen left a comment

thebrianchen Oct 11, 2020

thebrianchen Oct 11, 2020

thebrianchen Oct 11, 2020

schmidt-sebastian Oct 12, 2020

schmidt-sebastian commented Oct 12, 2020

BenWhitehead left a comment

schmidt-sebastian commented Oct 13, 2020

feat: add support for Partition API #1320

feat: add support for Partition API #1320

Conversation

schmidt-sebastian commented Oct 1, 2020 • edited

codecov bot commented Oct 2, 2020 • edited

Codecov Report

thebrianchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebrianchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Oct 12, 2020

BenWhitehead left a comment

Choose a reason for hiding this comment

schmidt-sebastian commented Oct 13, 2020

schmidt-sebastian commented Oct 1, 2020 •

edited

codecov bot commented Oct 2, 2020 •

edited