[realppl 6] offline ppl evaluation and tests #14852

wu-hui · 2025-05-13T20:01:29Z

No description provided.

gemini-code-assist · 2025-05-13T20:01:33Z

Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

google-oss-bot · 2025-05-13T20:02:25Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

ehsannas · 2025-09-19T18:11:04Z

/gemini review

Copilot

Pull Request Overview

This PR implements offline pipeline evaluation functionality and comprehensive testing for the Firestore C++ SDK. The changes enable data pipeline processing without requiring an active network connection, along with extensive test coverage for various pipeline scenarios.

Adds utility functions for creating array and map values from vectors and pairs to support testing
Implements comprehensive pipeline tests covering where, sort, limit, and various data semantics
Introduces shared utilities for pipeline test execution and validation

Reviewed Changes

Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`Firestore/core/test/unit/testutil/testutil.h`	Adds function declarations for `ArrayFromVector` and `MapFromPairs`
`Firestore/core/test/unit/testutil/testutil.cc`	Implements helper functions to create arrays and maps from C++ containers
`Firestore/core/test/unit/core/pipeline/where_test.cc`	Comprehensive where clause filtering tests with logical operations
`Firestore/core/test/unit/core/pipeline/utils.h`	Shared pipeline test utilities including serializer and document matchers
`Firestore/core/test/unit/core/pipeline/utils.cc`	Implementation of test utilities with database serializer
`Firestore/core/test/unit/core/pipeline/unicode_test.cc`	Tests for Unicode string handling in pipelines
`Firestore/core/test/unit/core/pipeline/sort_test.cc`	Comprehensive sorting tests with various data types and ordering
`Firestore/core/test/unit/core/pipeline/number_semantics_test.cc`	Tests for number comparison semantics including NaN and zero handling
`Firestore/core/test/unit/core/pipeline/null_semantics_test.cc`	Tests for null value handling in filtering and sorting operations
`Firestore/core/test/unit/core/pipeline/nested_properties_test.cc`	Tests for nested object property access and filtering
`Firestore/core/test/unit/core/pipeline/limit_test.cc`	Tests for result limiting functionality
`Firestore/core/test/unit/core/pipeline/inequality_test.cc`	Tests for inequality operators and range queries
`Firestore/core/test/unit/core/pipeline/error_handling_test.cc`	Tests for error scenarios and partial error handling

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Firestore/core/test/unit/core/pipeline/null_semantics_test.cc

Firestore/core/test/unit/core/pipeline/nested_properties_test.cc

gemini-code-assist

Code Review

This pull request introduces a major feature: offline evaluation for Firestore pipelines. It adds the necessary Evaluate methods to various pipeline stages and a RunPipeline function to execute them. A key part of this is the new RewriteStages utility, which ensures deterministic ordering by adding stable sorts where needed. The PR also includes a comprehensive suite of new unit tests covering a wide range of query scenarios, including collection group queries, complex filters, disjunctive queries, and various data type semantics. This is a substantial and well-tested addition to the codebase. My review focuses on a critical compilation issue in the new tests, a minor typo in a new API, and a couple of suggestions for simplification and consistency.

Firestore/core/test/unit/core/pipeline/collection_group_test.cc

gemini-code-assist · 2025-09-19T18:12:47Z

Firestore/core/src/api/realtime_pipeline.cc

+  return this->rewritten_stages_;
+}
+
+void RealtimePipeline::SetRewrittentStages(


There's a typo in the method name. It should be SetRewrittenStages.

Suggested change

void RealtimePipeline::SetRewrittentStages(

void RealtimePipeline::SetRewrittenStages(

gemini-code-assist · 2025-09-19T18:12:47Z

Firestore/core/src/api/realtime_pipeline.h

  const std::vector<std::shared_ptr<EvaluableStage>>& stages() const;
+  const std::vector<std::shared_ptr<EvaluableStage>>& rewritten_stages() const;
+
+  void SetRewrittentStages(std::vector<std::shared_ptr<EvaluableStage>>);


There's a typo in the method name. It should be SetRewrittenStages.

Suggested change

void SetRewrittentStages(std::vector<std::shared_ptr<EvaluableStage>>);

void SetRewrittenStages(std::vector<std::shared_ptr<EvaluableStage>>);

gemini-code-assist · 2025-09-19T18:12:48Z

Firestore/core/src/api/stages.h

+  absl::string_view name() const {
+    return "documents";
+  }


The name() method here is not marked virtual or override. While DocumentsSource doesn't inherit from EvaluableStage (which has the pure virtual name() method), it seems to follow a similar pattern. If the intent is for all stages to have a polymorphic name() method, consider adding virtual absl::string_view name() const to the base Stage class for consistency. If not, this is fine, but it's a point of potential inconsistency.

This change was applied in a later PR.

ehsannas · 2025-09-22T19:28:16Z

Firestore/core/src/api/realtime_pipeline.cc

+  return this->rewritten_stages_;
+}
+
+void RealtimePipeline::SetRewrittentStages(


ehsannas · 2025-09-22T19:28:57Z

Firestore/core/src/api/realtime_pipeline.h

  const std::vector<std::shared_ptr<EvaluableStage>>& stages() const;
+  const std::vector<std::shared_ptr<EvaluableStage>>& rewritten_stages() const;
+
+  void SetRewrittentStages(std::vector<std::shared_ptr<EvaluableStage>>);


ehsannas · 2025-09-22T19:35:55Z

Firestore/core/src/api/stages.cc

+model::PipelineInputOutputVector SortStage::Evaluate(
+    const EvaluateContext& context,
+    const model::PipelineInputOutputVector& inputs) const {
+  model::PipelineInputOutputVector input_copy = inputs;


why not take inputs by copy instead of taking by reference and copying it?

Good point. The reason is not every stage needs to copy the inputs, some of them can work with a reference.

Also worth pointing out, documents are optimized for copy, the actual document content are usually not copied (they are held via shared_ptr).

ehsannas · 2025-09-22T19:37:49Z

Firestore/core/src/api/stages.h

+  absl::string_view name() const {
+    return "documents";
+  }


ehsannas · 2025-09-22T19:48:02Z

Firestore/core/src/core/pipeline_util.h

+#include "Firestore/Protos/nanopb/google/firestore/v1/document.nanopb.h"
+#include "Firestore/core/src/api/expressions.h"
+#include "Firestore/core/src/api/realtime_pipeline.h"
+#include "Firestore/core/src/api/stages.h"
+#include "Firestore/core/src/model/mutable_document.h"
+#include "Firestore/core/src/nanopb/message.h"


IIUC This header file does not need any of these (23-28) headers. You should be able to remove all of these and only add a "forward declaration" on line 33:

class api::EvaluableStage;

This makes compilation faster, minimizes coupling of header files, and avoids false circular dependencies in the future. You will have to do the same imports in pipeline_util.cc instead.

Removed the redundant headers instead.

google-oss-bot added the api: firestore label May 13, 2025

wu-hui force-pushed the wuandy/RealPpl_6 branch from 7f35bbd to 0c3cc29 Compare May 14, 2025 23:38

wu-hui force-pushed the wuandy/RealPpl_5 branch from 89aa149 to 16f3645 Compare May 14, 2025 23:38

wu-hui force-pushed the wuandy/RealPpl_5 branch from 16f3645 to f0b8f3e Compare May 27, 2025 06:49

wu-hui force-pushed the wuandy/RealPpl_6 branch from 0c3cc29 to 00b7820 Compare May 27, 2025 06:49

wu-hui force-pushed the wuandy/RealPpl_5 branch from f0b8f3e to d7e061b Compare June 3, 2025 05:43

wu-hui force-pushed the wuandy/RealPpl_6 branch from 00b7820 to 96a4345 Compare June 3, 2025 05:43

wu-hui force-pushed the wuandy/RealPpl_5 branch from d7e061b to d32e2ba Compare June 30, 2025 18:37

wu-hui force-pushed the wuandy/RealPpl_6 branch from 96a4345 to 63d75fb Compare June 30, 2025 18:37

wu-hui force-pushed the wuandy/RealPpl_5 branch from d32e2ba to 3a7418a Compare July 3, 2025 17:38

wu-hui force-pushed the wuandy/RealPpl_6 branch 2 times, most recently from 18f7d5c to f3752fa Compare July 7, 2025 17:19

wu-hui force-pushed the wuandy/RealPpl_5 branch from 3a7418a to 3e452b5 Compare July 7, 2025 17:19

wu-hui force-pushed the wuandy/RealPpl_5 branch from 3e452b5 to 1fe6a33 Compare September 10, 2025 18:44

wu-hui force-pushed the wuandy/RealPpl_6 branch from f3752fa to 655d8cd Compare September 10, 2025 19:05

wu-hui requested a review from ehsannas September 10, 2025 19:06

wu-hui assigned ehsannas Sep 10, 2025

wu-hui force-pushed the wuandy/RealPpl_5 branch from 1fe6a33 to e6ecc47 Compare September 12, 2025 17:30

wu-hui force-pushed the wuandy/RealPpl_6 branch 2 times, most recently from aea7b2d to 06aaef3 Compare September 12, 2025 18:25

wu-hui force-pushed the wuandy/RealPpl_5 branch from e6ecc47 to 3f5c55e Compare September 12, 2025 18:25

wu-hui force-pushed the wuandy/RealPpl_6 branch from 06aaef3 to 8efed9e Compare September 12, 2025 18:30

wu-hui force-pushed the wuandy/RealPpl_5 branch 2 times, most recently from 6c0a698 to 9051e4f Compare September 15, 2025 15:40

wu-hui force-pushed the wuandy/RealPpl_6 branch from 8efed9e to 7c0150f Compare September 15, 2025 15:40

wu-hui force-pushed the wuandy/RealPpl_5 branch from 9051e4f to e67b0ac Compare September 15, 2025 15:42

wu-hui force-pushed the wuandy/RealPpl_6 branch 2 times, most recently from 4e3240d to 9706f02 Compare September 15, 2025 15:44

wu-hui force-pushed the wuandy/RealPpl_5 branch from e67b0ac to 569cc6f Compare September 15, 2025 15:44

wu-hui force-pushed the wuandy/RealPpl_6 branch from 9706f02 to edf7e9a Compare September 17, 2025 15:06

wu-hui force-pushed the wuandy/RealPpl_5 branch from 569cc6f to 6add223 Compare September 17, 2025 15:06

wu-hui force-pushed the wuandy/RealPpl_6 branch from edf7e9a to cb6e366 Compare September 19, 2025 16:43

wu-hui force-pushed the wuandy/RealPpl_5 branch from 6add223 to 9b9e2d9 Compare September 19, 2025 16:43

Base automatically changed from wuandy/RealPpl_5 to cheryllin/ppl September 19, 2025 16:44

wu-hui force-pushed the wuandy/RealPpl_6 branch from cb6e366 to 3abafda Compare September 19, 2025 16:45

ehsannas requested a review from Copilot September 19, 2025 18:10

Copilot AI reviewed Sep 19, 2025

View reviewed changes

gemini-code-assist bot reviewed Sep 19, 2025

View reviewed changes

ehsannas approved these changes Sep 22, 2025

View reviewed changes

[realppl 6] offline ppl evaluation and tests

28cf5fa

wu-hui force-pushed the wuandy/RealPpl_6 branch from 3abafda to 28cf5fa Compare September 29, 2025 15:33

wu-hui merged commit 2787ede into cheryllin/ppl Sep 29, 2025
7 of 9 checks passed

wu-hui deleted the wuandy/RealPpl_6 branch September 29, 2025 15:34

	void RealtimePipeline::SetRewrittentStages(
	void RealtimePipeline::SetRewrittenStages(

	void SetRewrittentStages(std::vector<std::shared_ptr<EvaluableStage>>);
	void SetRewrittenStages(std::vector<std::shared_ptr<EvaluableStage>>);

[realppl 6] offline ppl evaluation and tests #14852

[realppl 6] offline ppl evaluation and tests #14852

Conversation

wu-hui commented May 13, 2025

Uh oh!

gemini-code-assist bot commented May 13, 2025

Uh oh!

google-oss-bot commented May 13, 2025

Uh oh!

ehsannas commented Sep 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!