Firestore: add support for CollectionGroup queries.

[crwilcox]

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]

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]

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

[crwilcox]

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.

[tseaver]

@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]

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).

[tseaver]

@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.

[schmidt-sebastian]

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)

[danoscarmike]

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

[tseaver]

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

[crwilcox]

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

[tseaver]

@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

[wilhuff]

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

[tseaver]

Unrelated pubsub failure.

[tseaver]

Same oddity with logging due to pubsub.

[tseaver]

@crwilcox, @busunkim96 please flip the CLA bit.

[googlebot]

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.