Conversation
product-auto-label
bot
added
size: m
Summary of ChangesHello @Linchin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new literals pipeline stage, which allows injecting a fixed set of documents into a pipeline. No security vulnerabilities were identified. However, there are a couple of areas for improvement: the docstrings for the new functionality are currently placeholders and should be filled out, and a minor inconsistency in type hints for the new method should be corrected for clarity and consistency.
| """ | ||
| TODO: add docstring. | ||
| """ |
There was a problem hiding this comment.
The docstring for the literals method is a TODO placeholder. Please add a complete docstring explaining what the method does, its parameters, and providing examples of its usage. This is important for maintainability and for other developers to understand how to use this new pipeline stage.
"""Starts a pipeline with a fixed set of documents.
This stage replaces the current set of documents in the pipeline with a
new set of literal documents. This is useful for testing or for pipelines
that operate on a predefined set of data.
The documents can be provided directly as a `Constant` expression or
indirectly via a field reference from the input documents.
Example (with a `Constant`):
>>> from google.cloud.firestore_v1.pipeline_expressions import Constant
>>>
>>> documents = [
... {"name": "Alice", "score": 95},
... {"name": "Bob", "score": 87},
... ]
>>> pipeline = client.pipeline().literals(Constant.of(documents))
Example (with a field reference):
>>> # Assumes input documents have a field 'document_list' containing an array of documents.
>>> pipeline = client.pipeline().collection("sources").literals("document_list")
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).
Returns:
A new Pipeline object with this stage appended to the stage list.
"""There was a problem hiding this comment.
The docstring for the Literals class is a TODO placeholder. Please add a docstring explaining what the stage does.
| """TODO: add docstring.""" | |
| """Starts a pipeline with a fixed set of documents.""" |
There was a problem hiding this comment.
The type hint for documents is Selectable, but the implementation in Literals.__init__ also handles str. To be consistent with the implementation and other methods in this class (like select), the type hint should be str | Selectable.
| documents: Selectable, | |
| documents: str | Selectable, |
There was a problem hiding this comment.
The type hint for documents is Selectable, but the implementation in __init__ also handles str. To be consistent with the implementation, the type hint should be str | Selectable.
| def __init__(self, documents: Selectable): | |
| def __init__(self, documents: str | Selectable): |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new literals pipeline stage, which allows providing a fixed set of documents as the starting point for a pipeline. The implementation looks good and is accompanied by comprehensive unit and system tests. My feedback focuses on improving the clarity and correctness of the type hints and docstrings for the new literals method and Literals stage, as they are currently a bit misleading.
| stages.FindNearest(field, vector, distance_measure, options) | ||
| ) | ||
|
|
||
| def literals(self, *documents: str | Selectable) -> "_BasePipeline": |
There was a problem hiding this comment.
The type hint for *documents is a bit too restrictive. It doesn't account for passing dict objects directly to represent documents, which is a common and valid use case. Please consider broadening it to include dict.
| def literals(self, *documents: str | Selectable) -> "_BasePipeline": | |
| def literals(self, *documents: dict | str | Selectable) -> "_BasePipeline": |
| 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). |
There was a problem hiding this comment.
The docstring for the Args section is a bit misleading and incomplete.
- It refers to a single
documentsparameter, but the signature uses*documents(varargs). - It claims that a
stris "treated as a field path", which is incorrect. A raw string is treated as a string literal. To use a field path, one must wrap it inField.of(). - It doesn't mention that
dictobjects can be passed to represent documents.
I suggest updating the docstring for clarity and correctness.
| 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). | |
| Args: | |
| *documents: A sequence of documents or expressions. Each argument can be: | |
| - A `dict` representing a document object. | |
| - A `str` representing a string literal. To use a field path, | |
| wrap the string with `Field.of()`. | |
| - A `Selectable` expression. For example, a `Constant` | |
| containing an array of documents (as dictionaries). |
| class Literals(Stage): | ||
| """Returns documents from a fixed set of predefined document objects.""" | ||
|
|
||
| def __init__(self, *documents: str | Selectable): |
There was a problem hiding this comment.
The type hint for *documents is too restrictive. The implementation correctly handles dict objects (via encode_value), but the type hint doesn't reflect this. This is inconsistent with the usage shown in system tests. Please consider broadening the type hint to include dict.
| def __init__(self, *documents: str | Selectable): | |
| def __init__(self, *documents: dict | str | Selectable): |
2 participants
TODO: