Firestore: add support for CollectionGroup queries. #7758

crwilcox · 2019-04-19T20:56:12Z

CollectionGroup queries add support for queries to Firestore that query documents by collection ID rather than by collection path.

Java: googleapis/google-cloud-java#4652
Node: googleapis/nodejs-firestore#578

googlebot · 2019-04-19T20:56:15Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

schmidt-sebastian

This looks sane to me, but please have someone from the Python team look at this as well. Thanks so much!

schmidt-sebastian · 2019-04-19T21:01:37Z

firestore/google/cloud/firestore_v1/query.py

+
+
+def _collection_group_query_response_to_snapshot(response_pb, collection):
+    """Parse a query response protobuf to a document snapshot.


I am a little surprised that this is a different method from _query_response_to_snapshot.

This was related to expectations built into this method. @tseaver made this to address that. It is possible we can combine them by refactoring that code.

@schmidt-sebastian The implementation of _query_response_to_snapshot mandates (via _helpers.get_doc_id) that the result document is a direct child of the query's parent collection. I'm unsure whether this restriction is "important", or just an artifact of prior development iterations. If it is an artifact, then we could just use the new implementation in _collection_group_query_response_to_snapshot for all queries.

schmidt-sebastian · 2019-04-19T21:02:34Z

firestore/google/cloud/firestore_v1/query.py

+        is not set, the snapshot will be :data:`None`.
+    """
+    if not response_pb.HasField("document"):
+        return None


Does Python not return empty DocumentSnapshots for non-existing documents? In either case, this is a query and this condition should never trigger (fingers crossed).

@schmidt-sebastian DocumentReference.get does return an empty snapshot for a non-existent document. For the query case, the back-end can return a response without a document when reporting partial results: see #6924 and PR #7206 for the history.

My bad: It looks like the expected behavior is that it can get skipped (see https://cs.corp.google.com/piper///depot/google3/google/firestore/v1beta1/firestore.proto?q=runqueryrequest+firestore&dr=CSs&l=619)

firestore/tests/system.py

danoscarmike · 2019-04-22T18:24:05Z

@tseaver can you give this one your blessing too? Thanks!

tseaver · 2019-04-22T20:44:00Z

@danoscarmike I will look at the branch, and get the tests / coverage passing.

crwilcox · 2019-04-29T20:10:42Z

The backend has deployed. So removing the 'do not merge'

Blacken.

Skip the one for 'where', because it requires a custom index.

See: https://github.com/googleapis/nodejs-firestore/pull/578/files#diff-6b8febc8d51ea01205628091b3611eacR1188

tseaver · 2019-04-29T20:50:28Z

@schmidt-sebastian I updated the "start at / end at" test as you requested in 115234c. However, for the "filters" test, when I change to filter on __name__ (which would match the Node test), the following query:

(Pdb) pp query._to_protobuf()
from {
  collection_id: "b-1556570226919"
  all_descendants: true
}
where {
  composite_filter {
    op: AND
    filters {
      field_filter {
        field {
          field_path: "__name__"
        }
        op: GREATER_THAN_OR_EQUAL
        value {
          string_value: "a/b"
        }
      }
    }
    filters {
      field_filter {
        field {
          field_path: "__name__"
        }
        op: LESS_THAN_OR_EQUAL
        value {
          string_value: "a/b0"
        }
      }
    }
  }
}

yields this error from the backend:

E   google.api_core.exceptions.InvalidArgument: 400 a filter on __name__ must be a document resource name

Note that this still doesn't pass, so remains skipped.

wilhuff · 2019-04-29T21:21:24Z

In terms of the protos, I think you need to specify the value as reference_value. Note comments there on the format.

tseaver · 2019-04-30T17:14:49Z

Unrelated pubsub failure.

tseaver · 2019-04-30T17:16:45Z

Same oddity with logging due to pubsub.

tseaver · 2019-04-30T17:18:14Z

@crwilcox, @busunkim96 please flip the CLA bit.

googlebot · 2019-04-30T17:34:37Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

Skip the one for 'where', because it requires a custom index.

* Initial plumbing for collection group queries * Don't assume direct children for collection group queries. * trim document path to DocumentReference * add unit tests * ensure all_descendants is set after calling other query methods * port test for node impl * port tests from node impl * Fix collection group test on Python 2.7. Blacken. * Use '_all_descendants' in 'Query.__eq__'. * Ensure '_all_descendants' propagates when narrowing query. * Refactor collection group system tests. Skip the one for 'where', because it requires a custom index. * Match node test's collection group ID / expected output. See: https://github.com/googleapis/nodejs-firestore/pull/578/files#diff-6b8febc8d51ea01205628091b3611eacR1188 * Match Node test for filter on '__name__'. Note that this still doesn't pass, so remains skipped. * Blacken. * Fix / unskip systest for collection groups w/ filter on '__name__'. * Blacken * 100% coverage. * Lint

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Apr 19, 2019

crwilcox added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 19, 2019

schmidt-sebastian approved these changes Apr 19, 2019

View reviewed changes

danoscarmike requested a review from tseaver April 22, 2019 18:23

tseaver force-pushed the firestore-collection-group branch from 6ea0076 to 9d2d401 Compare April 23, 2019 18:51

yoshi-automation added the 🚨 This issue needs some love. label Apr 26, 2019

crwilcox removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 29, 2019

crwilcox changed the title ~~feature: Adding CollectionGroup queries (DO NOT MERGE)~~ feature: Adding CollectionGroup queries Apr 29, 2019

crwilcox and others added 12 commits April 29, 2019 16:40

Initial plumbing for collection group queries

564da9d

Don't assume direct children for collection group queries.

84a3684

trim document path to DocumentReference

bf36c61

add unit tests

796e59c

ensure all_descendants is set after calling other query methods

c79aba4

port test for node impl

b606d88

port tests from node impl

10df4e9

Fix collection group test on Python 2.7.

89b97b4

Blacken.

Use '_all_descendants' in 'Query.__eq__'.

4a643a2

Ensure '_all_descendants' propagates when narrowing query.

e699f33

Refactor collection group system tests.

3d9b901

Skip the one for 'where', because it requires a custom index.

Match node test's collection group ID / expected output.

115234c

See: https://github.com/googleapis/nodejs-firestore/pull/578/files#diff-6b8febc8d51ea01205628091b3611eacR1188

tseaver force-pushed the firestore-collection-group branch from f2b44dc to 115234c Compare April 29, 2019 20:44

tseaver added 2 commits April 29, 2019 17:01

Match Node test for filter on '__name__'.

8925d62

Note that this still doesn't pass, so remains skipped.

Blacken.

2689108

Fix / unskip systest for collection groups w/ filter on '__name__'.

1223f34

tseaver added the api: firestore Issues related to the Firestore API. label Apr 30, 2019

tseaver changed the title ~~feature: Adding CollectionGroup queries~~ Firestore: Add support for CollectionGroup queries. Apr 30, 2019

tseaver changed the title ~~Firestore: Add support for CollectionGroup queries.~~ Firestore: add support for CollectionGroup queries. Apr 30, 2019

tseaver added 3 commits April 30, 2019 10:50

Blacken

412aade

100% coverage.

7297f10

Lint

2ee7bbb

busunkim96 added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. 🚨 This issue needs some love. labels Apr 30, 2019

yoshi-automation added the 🚨 This issue needs some love. label Apr 30, 2019

crwilcox merged commit f5aec2d into master Apr 30, 2019

crwilcox deleted the firestore-collection-group branch April 30, 2019 18:28

anguillanneuf mentioned this pull request May 1, 2019

Clean up bebugging code #7843

Merged

tseaver referenced this pull request May 14, 2019

Refactor collection group system tests.

f2b44dc

Skip the one for 'where', because it requires a custom index.

quartzmo mentioned this pull request May 24, 2019

Firestore Collection Group queries googleapis/google-cloud-ruby#3416

Merged



		def _collection_group_query_response_to_snapshot(response_pb, collection):
		"""Parse a query response protobuf to a document snapshot.

Firestore: add support for CollectionGroup queries. #7758

Firestore: add support for CollectionGroup queries. #7758

Uh oh!

Conversation

crwilcox commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Apr 19, 2019

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

crwilcox Apr 20, 2019

Choose a reason for hiding this comment

Uh oh!

tseaver Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

tseaver Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danoscarmike commented Apr 22, 2019

Uh oh!

tseaver commented Apr 22, 2019

Uh oh!

crwilcox commented Apr 29, 2019

Uh oh!

tseaver commented Apr 29, 2019

Uh oh!

wilhuff commented Apr 29, 2019

Uh oh!

tseaver commented Apr 30, 2019

Uh oh!

tseaver commented Apr 30, 2019

Uh oh!

tseaver commented Apr 30, 2019

Uh oh!

googlebot commented Apr 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

crwilcox commented Apr 19, 2019 •

edited

Loading

schmidt-sebastian Apr 23, 2019 •

edited

Loading