feat: Add Rand and Trunc expressions#16037
Conversation
Rand and Trunc expressionRand and Trunc expressions
Summary of ChangesHello, 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 enhances the Firestore client library by introducing a new Highlights
Changelog
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
|
There was a problem hiding this comment.
Code Review
This pull request adds a Rand expression for Firestore pipelines, along with corresponding unit and system tests. The implementation of the Rand expression itself looks good.
However, I found a logical flaw in the system test for Rand, where it uses two separate calls to Rand() to check if the value is within a range, which doesn't guarantee that a single random number is within that range. I've left a suggestion to improve the test.
Also, the pull request title mentions adding a Trunc expression, but it doesn't seem to be included in the changes. This might be intentional, but it's worth clarifying.
Note: Security Review did not run due to the size of the PR.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/google-cloud-firestore/tests/system/pipeline_e2e/math.yaml (316-322)
This test is logically flawed because it calls Rand() twice within the And expression. This means two different random numbers will be generated, and the test is checking if rand1 >= 0.0 and rand2 < 1.0. This doesn't correctly verify that a single random number is within the [0.0, 1.0) range.
For example, if the first Rand() returned 2.0 and the second returned 0.5, the expression (2.0 >= 0.0) AND (0.5 < 1.0) would evaluate to true, even though 2.0 is outside the expected range.
A more robust test would use the result of a single Rand() call. Since the PR title mentions Trunc, I assume a Trunc expression is available or will be soon. You could use it to verify the range with a single Rand() call, which would be a much stronger guarantee of correctness.
- FunctionExpression.equal:
- FunctionExpression.trunc:
- Rand: []
- Constant: 0.0
No description provided.