Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
73 commits
Select commit Hold shift + click to select a range
efec02f
feat: support query profiling
Linchin Jul 17, 2024
886c3b8
collection
Linchin Jul 17, 2024
4dde816
fix unit tests
Linchin Jul 18, 2024
431db7f
unit tests
Linchin Jul 21, 2024
1ebcb56
vector get and stream, unit tests
Linchin Jul 21, 2024
7338db9
aggregation get and stream, unit tests
Linchin Jul 21, 2024
ca6adc1
docstring
Linchin Jul 22, 2024
05d7578
query profile unit tests
Linchin Jul 22, 2024
dc97653
update base classes' method signature
Linchin Jul 22, 2024
75377fc
documentsnapshotlist unit tests
Linchin Jul 22, 2024
60acc88
func signatures
Linchin Jul 23, 2024
422d966
undo client.py change
Linchin Jul 23, 2024
bb12421
transaction.get()
Linchin Jul 23, 2024
83e62bb
lint
Linchin Jul 24, 2024
b53805d
system test
Linchin Jul 24, 2024
c11f382
fix shim test
Linchin Jul 24, 2024
841a754
fix sys test
Linchin Jul 24, 2024
a5748b2
fix sys test
Linchin Jul 24, 2024
920db4b
system test
Linchin Jul 24, 2024
7023417
another system test
Linchin Jul 25, 2024
1842709
skip system test in emulator
Linchin Jul 25, 2024
74db4fe
stream generator unit tests
Linchin Jul 25, 2024
8a4c5e4
coverage
Linchin Jul 26, 2024
e7dda7c
add system tests
Linchin Jul 26, 2024
e61815f
Merge branch 'main' into query-profiling-3
Linchin Jul 26, 2024
23b88b9
small fixes
Linchin Jul 26, 2024
7912c95
undo document change
Linchin Jul 29, 2024
aa7b3d9
add system tests
Linchin Jul 29, 2024
46e5139
vector query system tests
Linchin Jul 29, 2024
299af43
format
Linchin Jul 29, 2024
41fd646
fix system test
Linchin Jul 29, 2024
ca631ec
comments
Linchin Jul 29, 2024
10cc536
add system tests
Linchin Jul 29, 2024
697775d
improve stream generator
Linchin Jul 31, 2024
30e5efc
type checking
Linchin Aug 1, 2024
981a644
adding stars
Linchin Aug 1, 2024
28abbf2
delete comment
Linchin Aug 1, 2024
9ce86be
remove coverage requirements for type checking part
Linchin Aug 1, 2024
dff947e
add explain_options to StreamGenerator
Linchin Aug 1, 2024
4019d64
yield tuple instead
Linchin Aug 5, 2024
978268b
raise exception when explain_metrics is absent
Linchin Aug 6, 2024
c293787
refactor documentsnapshotlist into queryresultslist
Linchin Aug 6, 2024
6fc1600
add comment
Linchin Aug 6, 2024
a4e87bf
improve type hint
Linchin Aug 6, 2024
cb693f8
lint
Linchin Aug 6, 2024
7439e76
move QueryResultsList to stream_generator.py
Linchin Aug 6, 2024
bb60ce0
aggregation related type annotation
Linchin Aug 6, 2024
6f86854
transaction return type hint
Linchin Aug 7, 2024
05424bf
refactor QueryResultsList
Linchin Aug 7, 2024
5e15e6a
change stream generator to return ExplainMetrics instead of yield
Linchin Aug 12, 2024
0906d65
update aggregation query to use the new generator
Linchin Aug 13, 2024
ccbb623
update query to use the new generator
Linchin Aug 13, 2024
843dc05
update vector query to use the new generator
Linchin Aug 13, 2024
066ead5
lint
Linchin Aug 13, 2024
f542ac1
type annotations
Linchin Aug 15, 2024
5fb71dd
Merge branch 'main' into query-profiling-3
Linchin Aug 15, 2024
8b82957
fix type annotation to be python 3.9 compatible
Linchin Aug 15, 2024
a400f6f
fix type hint for python 3.8
Linchin Aug 15, 2024
12d18c0
fix system test
Linchin Aug 15, 2024
9bf8b00
add test coverage
Linchin Aug 16, 2024
7ae1028
use class method get_explain_metrics() instead of property explain_me…
Linchin Aug 19, 2024
f08a35d
Merge branch 'main' into query-profiling-3
Linchin Aug 26, 2024
560ba95
address comments
Linchin Aug 27, 2024
4955a0b
remove more Optional
Linchin Aug 27, 2024
1293a36
add type hint for async stream generator
Linchin Aug 28, 2024
bd18af2
simplify yield in aggregation stream
Linchin Aug 29, 2024
ab9e3bf
stream generator type annotation
Linchin Aug 29, 2024
fa45e05
more type hints
Linchin Aug 30, 2024
bb172be
remove "Integer"
Linchin Aug 30, 2024
d231424
docstring format
Linchin Aug 30, 2024
765420a
mypy
Linchin Sep 4, 2024
71bde2a
add more input verification for query_results.py
Linchin Sep 5, 2024
cf89254
Merge branch 'main' into query-profiling-3
Linchin Sep 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions google/cloud/firestore_v1/async_stream_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,28 @@
Firestore API.
"""

from collections import abc
from typing import Any, AsyncGenerator, Awaitable, TypeVar


class AsyncStreamGenerator(abc.AsyncGenerator):
T = TypeVar("T")


class AsyncStreamGenerator(AsyncGenerator[T, Any]):
"""Asynchronous generator for the streamed results."""

def __init__(self, response_generator: abc.AsyncGenerator):
def __init__(self, response_generator: AsyncGenerator[T, Any]):
self._generator = response_generator

def __aiter__(self):
return self._generator
def __aiter__(self) -> AsyncGenerator[T, Any]:
return self

def __anext__(self):
def __anext__(self) -> Awaitable[T]:
return self._generator.__anext__()

def asend(self, value=None):
def asend(self, value=None) -> Awaitable[Any]:
return self._generator.asend(value)

def athrow(self, exp=None):
def athrow(self, exp=None) -> Awaitable[Any]:
return self._generator.athrow(exp)

def aclose(self):
Expand Down
24 changes: 5 additions & 19 deletions google/cloud/firestore_v1/base_aggregation.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,7 @@

import abc
from abc import ABC
from typing import (
TYPE_CHECKING,
Any,
Awaitable,
List,
Optional,
Tuple,
Union,
)
from typing import TYPE_CHECKING, List, Optional, Tuple, Union

from google.api_core import gapic_v1
from google.api_core import retry as retries
Expand Down Expand Up @@ -239,10 +231,7 @@ def get(
timeout: float | None = None,
*,
explain_options: Optional[ExplainOptions] = None,
) -> (
QueryResultsList[AggregationResult]
| Awaitable[Any, Any, List[AggregationResult]]
):
) -> QueryResultsList[List[AggregationResult]] | List[List[AggregationResult]]:
"""Runs the aggregation query.

This sends a ``RunAggregationQuery`` RPC and returns a list of aggregation results in the stream of ``RunAggregationQueryResponse`` messages.
Expand All @@ -264,9 +253,8 @@ def get(
explain_metrics will be available on the returned generator.

Returns:
QueryResultsList[AggregationResult] | Awaitable[Any, Any, List[AggregationResult]]:
(QueryResultsList[List[AggregationResult]] | List[List[AggregationResult]]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I think about it, I don't think get() returns a coroutine here, so I changed it to the list.

The reason it inlcludes that Coroutine part is because this method is overridden by the async method in the async subclass, which implicitly wraps its output in a coroutine. Mypy raises an issue for this issue in async_aggregation:

google/cloud/firestore_v1/async_aggregation.py:49: error: Signature of "get" incompatible with supertype "BaseAggregationQuery"  [override]
google/cloud/firestore_v1/async_aggregation.py:49: note:      Superclass:
google/cloud/firestore_v1/async_aggregation.py:49: note:          def get(self, transaction: Any = ..., retry: Retry | _MethodDefault | None = ..., timeout: float | None = ..., *, explain_options: ExplainOptions | None = ...) -> QueryResultsList[list[AggregationResult]] | list[list[AggregationResult]]
google/cloud/firestore_v1/async_aggregation.py:49: note:      Subclass:
google/cloud/firestore_v1/async_aggregation.py:49: note:          def get(self, transaction: Any = ..., retry: AsyncRetry | _MethodDefault | None = ..., timeout: float | None = ...) -> Coroutine[Any, Any, list[AggregationResult]]

But that said, I did some research, and I don't think the old solution of adding Coroutine or Awaitable as a return option actually works for mypy. I'd say maybe you should leave the return annotation mostly how it was for now, and we can find a better solution when we fix types later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, but after trying it out I think bothCoroutine[Any, Any, List[List[AggregationResult]]] and Awaitable[List[List[AggregationResult]]] work? (maybe I'm missing something)

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 what I see:

google/cloud/firestore_v1/async_aggregation.py:49: error: Signature of "get" incompatible with supertype "BaseAggregationQuery"  [override]
google/cloud/firestore_v1/async_aggregation.py:49: note:      Superclass:
google/cloud/firestore_v1/async_aggregation.py:49: note:          def get(self, transaction: Any = ..., retry: Retry | _MethodDefault | None = ..., timeout: float | None = ..., *, explain_options: ExplainOptions | None = ...) -> QueryResultsList[list[AggregationResult]] | Coroutine[Any, Any, list[AggregationResult]]
google/cloud/firestore_v1/async_aggregation.py:49: note:      Subclass:
google/cloud/firestore_v1/async_aggregation.py:49: note:          def get(self, transaction: Any = ..., retry: AsyncRetry | _MethodDefault | None = ..., timeout: float | None = ...) -> Coroutine[Any, Any, list[AggregationResult]]

But if you found it works for you, that's fine with me too! Either way, I think this can fall into the known-broken mypy issues to fix later

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 see, I will revert the change and save this for later haha

The aggregation query results.

"""

@abc.abstractmethod
Expand All @@ -279,10 +267,7 @@ def stream(
timeout: Optional[float] = None,
*,
explain_options: Optional[ExplainOptions] = None,
) -> (
StreamGenerator[List[AggregationResult]]
| AsyncStreamGenerator[List[AggregationResult], Any]
):
) -> StreamGenerator[List[AggregationResult]] | AsyncStreamGenerator:
"""Runs the aggregation query.

This sends a``RunAggregationQuery`` RPC and returns a generator in the stream of ``RunAggregationQueryResponse`` messages.
Expand All @@ -302,5 +287,6 @@ def stream(
explain_metrics will be available on the returned generator.

Returns:
StreamGenerator[List[AggregationResult]] | AsyncStreamGenerator:
A generator of the query results.
"""
5 changes: 2 additions & 3 deletions google/cloud/firestore_v1/base_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
from google.cloud.firestore_v1.document import DocumentReference
from google.cloud.firestore_v1.field_path import FieldPath
from google.cloud.firestore_v1.query_profile import ExplainOptions
from google.cloud.firestore_v1.query_results import QueryResultsList
from google.cloud.firestore_v1.transaction import Transaction
from google.cloud.firestore_v1.vector import Vector
from google.cloud.firestore_v1.vector_query import VectorQuery
Expand Down Expand Up @@ -497,9 +498,7 @@ def get(
timeout: Optional[float] = None,
*,
explain_options: Optional[ExplainOptions] = None,
) -> Union[
Generator[DocumentSnapshot, Any, Any], AsyncGenerator[DocumentSnapshot, Any]
]:
) -> QueryResultsList[DocumentSnapshot]:
raise NotImplementedError

def stream(
Expand Down
7 changes: 4 additions & 3 deletions google/cloud/firestore_v1/query_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@
QueryExplainError,
)

from typing import List, Optional, TypeVar

from typing import List, Optional
T = TypeVar("T")


class QueryResultsList(list):
class QueryResultsList(List[T]):
"""A list of received query results from the query call.

This is a subclass of the built-in list. A new property `explain_metrics`
Expand Down Expand Up @@ -68,4 +69,4 @@ def get_explain_metrics(self) -> ExplainMetrics:
if self._explain_options is None:
raise QueryExplainError("explain_options not set on query.")
else:
return self._explain_metrics
return self._explain_metrics # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is caused by mypy being unaware of the inner logic of the function. It either returns ExplainMetrics, or raises an exception, so there's no way it returns None. I will add # type: ignore to this line.

I don't think that's true. There's nothing stopping someone from passing explain_options, but not explain_metrics:

instance = QueryResultList([], object(), None)
instance.get_explain_metrics() == None

There are a couple ways we can address this. Maybe we want to ensure that if one is set, they both are, and store them together as a tuple? Or maybe we should just add a separate exception here if self._explain_metrics is None as well? I can't remember off the top of my head if it makes sense to have one but not the other here


Either way, I think adding # type: ignore should be a last resort. We can sometimes use typing.cast if mypy ever has trouble following the invariants. But I think in this case, it was actually a good catch in the logic

Copy link
Contributor Author

@Linchin Linchin Sep 3, 2024

Choose a reason for hiding this comment

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

You are right, mypy did have a good point! When explain_options is set, ultimately server will return explain_metrics, and it should have already been gathered before QueryResultList is initialized. To make QueryResultsList more robust, I can do the following:

  1. When initializing, throw an error when explain_options is not None and explain_metrics is None. (maybe even the case explain_options is None and explain_metrics is not None?)
  2. Add a check in get_explain_metrics() like the following in case self._explain_metrics got wiped out somewhere
    def get_explain_metrics(self) -> ExplainMetrics:
        if self._explain_options is None:
            raise QueryExplainError("explain_options not set on query.")
        elif self._explain_metrics is None:
            raise QueryExplainError("explain_metrics is empty despite explain_options is set.")
        else:
            return self._explain_metrics

Copy link
Contributor

Choose a reason for hiding this comment

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

That works, or you can require them to be passed as a tuple, which would solve the same typing problem with less checks:

class QueryResultsList:

    def __init__(
        self,
        docs: List,
        profile_data: tuple[ExplainOptions, ExplainMetrics] | None = None
    ):

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

2 changes: 1 addition & 1 deletion google/cloud/firestore_v1/stream_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
T = TypeVar("T")


class StreamGenerator(Generator[T, Any, None]):
class StreamGenerator(Generator[T, Any, Optional[ExplainMetrics]]):
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 aware of the overload incompatibility here, but if we change the method signature now, we risk being backward incompatible with the version we just released 😅

It looks like this passes the check:

def throw(self, exp=None, value=None, traceback=None) -> T:
    return self._generator.throw(exp, value, traceback)

This should be backwards compatible, since we're just adding new optional arguments. All existing code should be unaffected


Honestly though, I think removing the =None from exp=None would be best. exp is meant to be an exception, so passing None just raises a TypeError. Usually we should avoid potential breaking changes, but I don't think that applies in this case, where the default itself broken. Especially since this was such a recent addition. Let me know what you think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, we shouldn't let exp be optional. Just to track our discussion offline, I tried to add type annotations for all the parameters here, but it seems mypy has trouble matching it with the annotations that are present in the parent class Generator. So we will skip the annotation here and let mypy infer it from the parent class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, python 3.12 has deprecated parameters value and traceback in generator.throw(), so to be compatible with both 3.12 and older versions, we will just use throw(*args, **kwargs).

"""Generator for the streamed results.
Args:
Expand Down