-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(firestore): literals pipeline stage #16028
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
2a8213c
28acee9
a2e11ed
a97cb13
cc1b737
3861223
7bad13c
36e45bf
816498e
09f59f6
11c22ee
941101f
ccb159a
6aae503
fa30d5f
db98e44
4a85ee6
5627c75
0dbfc77
837e52c
fc2880d
a515903
eee827e
c4c8398
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -275,6 +275,71 @@ def find_nearest( | |
| stages.FindNearest(field, vector, distance_measure, options) | ||
| ) | ||
|
|
||
| def literals(self, *documents: str | Selectable) -> "_BasePipeline": | ||
| """ | ||
| Returns documents from a fixed set of predefined document objects. | ||
|
|
||
| This stage is commonly used for testing other stages in isolation, | ||
| though it can also be used as inputs to join conditions. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to conflict with the statement later:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| Example: | ||
| >>> from google.cloud.firestore_v1.pipeline_expressions import Constant | ||
| >>> documents = [ | ||
| ... {"name": "joe", "age": 10}, | ||
| ... {"name": "bob", "age": 30}, | ||
| ... {"name": "alice", "age": 40} | ||
| ... ] | ||
| >>> pipeline = client.pipeline() | ||
| ... .literals(Constant.of(documents)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the code, it seems like:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching this! I spent some time to check the internal docs, and I think |
||
| ... .where(field("age").lessThan(35)) | ||
|
|
||
| Output documents: | ||
| ```json | ||
| [ | ||
| {"name": "joe", "age": 10}, | ||
| {"name": "bob", "age": 30} | ||
| ] | ||
| ``` | ||
|
|
||
| Behavior: | ||
| The `literals(...)` stage can only be used as the first stage in a pipeline (or | ||
| sub-pipeline). The order of documents returned from the `literals` matches the | ||
|
Linchin marked this conversation as resolved.
Outdated
|
||
| order in which they are defined. | ||
|
|
||
| While literal values are the most common, it is also possible to pass in | ||
| expressions, which will be evaluated and returned, making it possible to test | ||
| out different query / expression behavior without first needing to create some | ||
| test data. | ||
|
|
||
| For example, the following shows how to quickly test out the `length(...)` | ||
| function on some constant test sets: | ||
|
|
||
| Example: | ||
| >>> from google.cloud.firestore_v1.pipeline_expressions import Constant | ||
| >>> documents = [ | ||
| ... {"x": Constant.of("foo-bar-baz").char_length()}, | ||
| ... {"x": Constant.of("bar").char_length()} | ||
| ... ] | ||
| >>> pipeline = client.pipeline().literals(Constant.of(documents)) | ||
|
|
||
| Output documents: | ||
| ```json | ||
| [ | ||
| {"x": 11}, | ||
| {"x": 3} | ||
| ] | ||
| ``` | ||
|
|
||
| Args: | ||
| documents: A `str` or `Selectable` expression. If a `str`, it's | ||
| treated as a field path to an array of documents. | ||
| If a `Selectable`, it's usually a `Constant` | ||
| containing an array of documents (as dictionaries). | ||
|
Linchin marked this conversation as resolved.
Outdated
|
||
| Returns: | ||
| A new Pipeline object with this stage appended to the stage list. | ||
| """ | ||
| return self._append(stages.Literals(*documents)) | ||
|
|
||
| def replace_with( | ||
| self, | ||
| field: Selectable, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -684,4 +684,23 @@ tests: | |
| - args: | ||
| - fieldReferenceValue: awards | ||
| - stringValue: full_replace | ||
| name: replace_with | ||
| name: replace_with | ||
| - description: literals | ||
| pipeline: | ||
| - Literals: | ||
| - title: "The Hitchhiker's Guide to the Galaxy" | ||
| author: "Douglas Adams" | ||
|
daniel-sanche marked this conversation as resolved.
|
||
| assert_results: | ||
| - title: "The Hitchhiker's Guide to the Galaxy" | ||
| author: "Douglas Adams" | ||
| assert_proto: | ||
| pipeline: | ||
| stages: | ||
| - args: | ||
| - mapValue: | ||
| fields: | ||
| author: | ||
| stringValue: "Douglas Adams" | ||
| title: | ||
| stringValue: "The Hitchhiker's Guide to the Galaxy" | ||
| name: literals | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also have tests here that cover the different input types we support
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I added additional type to test.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we still need more examples. Both of these are dict-like, so it seems like a pretty basic test. What if someone passes in It would also be good to add some extra stages, to make sure this works like others You can use gemini to create a few extra test scenarios. I usually don't include assert_proto on all of them, because it can be excessive
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Referring to the golang implementation, the only type accepted is a list of key-value pairs. So I don't think But I think it's a good idea to add more tests. I will also use |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type hint for
*documentsis incomplete. It should includedictas documents are often passed as dictionaries, which is not covered bystr | Selectable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably ignore this, unless that's how other languages handle it
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, looking at go, it seems like it accepts dicts, but not strings?
I don't know much about this stage, but from what I've seen, it's supposed to deal with maps. So maybe this should be
def literals(self, *documents: Map | dict[str, CONSTANT_TYPE] | Selectable)?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon further thoughts, I think it should be
def literals(self, *documents: dict | Expression):. In this case bothConstantandMapare child classes ofExpression.