[realppl 2] Minimalistic ppl offline evaluation #14827

wu-hui · 2025-05-07T19:26:17Z

No description provided.

gemini-code-assist · 2025-05-07T19:26:21Z

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-07T19:27:17Z

	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-07-08T21:57:34Z

Firestore/core/src/api/expressions.h

 public:
-  explicit Field(std::string name) : name_(std::move(name)) {
+  ~Selectable() override = default;


It's better to use virtual ~Selectable() override = default;

the rule of thumb is: if a class has at least one virtual function, it should have a virtual destructor

ehsannas · 2025-07-08T22:14:13Z

Firestore/core/src/api/realtime_pipeline.h

+  RealtimePipeline(std::vector<std::shared_ptr<EvaluableStage>> stages,
+                   remote::Serializer serializer);
+
+  RealtimePipeline AddingStage(std::shared_ptr<EvaluableStage> stage);


should this be AddStage?

ehsannas · 2025-07-08T22:16:10Z

Firestore/core/src/api/stages.h

@@ -43,24 +49,55 @@ class Stage {
  virtual google_firestore_v1_Pipeline_Stage to_proto() const = 0;
 };

-class CollectionSource : public Stage {
+class EvaluateContext {


Is there going to be more stuff added to the EvaluateContext?

As of right now it's just a wrapper around remote::Serializer and it's not clear why it's needed. For example, EvaluableStage constructor could just take a remote::Serializer parameter rather than an EvaluateContext parameter.

ehsannas · 2025-07-08T22:19:34Z

Firestore/core/src/api/stages.h

+  remote::Serializer* serializer_;
+};
+
+class EvaluableStage : public Stage {


Please add a brief comment above each class to highlight its purpose and intended usage.

ehsannas · 2025-07-08T22:27:31Z

Firestore/core/src/api/stages.h

+      const model::PipelineInputOutputVector& inputs) const = 0;
+};
+
+class CollectionSource : public EvaluableStage {


It's not clear why some stages below are EvaluableStages but some aren't. Is it just because you haven't implemented them yet?

ehsannas · 2025-07-08T23:47:38Z

Firestore/core/src/core/expressions_eval.h

+    return EvaluateResult(ResultType::kUnset,
+                          nanopb::Message<google_firestore_v1_Value>());


ehsannas · 2025-07-08T23:58:51Z

Firestore/core/src/core/expressions_eval.cc

+  if (model::GetTypeOrder(*left.value()) !=
+      model::GetTypeOrder(*right.value())) {
+    return EvaluateResult::NewValue(nanopb::MakeMessage(model::FalseValue()));
+  }
+  if (model::IsNaNValue(*left.value()) || model::IsNaNValue(*right.value())) {
+    return EvaluateResult::NewValue(nanopb::MakeMessage(model::FalseValue()));
+  }
+
+  // TODO(pipeline): Port strictEquals from web
+  if (model::Equals(*left.value(), *right.value())) {
+    return EvaluateResult::NewValue(nanopb::MakeMessage(model::TrueValue()));
+  } else {
+    return EvaluateResult::NewValue(nanopb::MakeMessage(model::FalseValue()));
+  }


ideally we should reuse the logic from model::Equals, and replace this whole block with:

if (model::Equals(*left.value(), *right.value())) { return EvaluateResult::NewValue(nanopb::MakeMessage(model::TrueValue())); } else { return EvaluateResult::NewValue(nanopb::MakeMessage(model::FalseValue())); }

I see some discrepancy. For example: on line 169 we are saying NaN is not equal to any other value, but value comparison logic says NaN is equal to NaN.

ehsannas · 2025-07-09T00:06:40Z

Firestore/core/src/core/expressions_eval.cc

+    return EvaluateResult(ResultType::kBoolean, std::move(value));
+  } else if (model::IsInteger(*value)) {
+    return EvaluateResult(ResultType::kInt, std::move(value));
+  } else if (model::IsDouble(*value)) {
+    return EvaluateResult(ResultType::kDouble, std::move(value));
+  } else if (value->which_value_type ==
+             google_firestore_v1_Value_timestamp_value_tag) {
+    return EvaluateResult(ResultType::kTimestamp, std::move(value));
+  } else if (value->which_value_type ==
+             google_firestore_v1_Value_string_value_tag) {
+    return EvaluateResult(ResultType::kString, std::move(value));
+  } else if (value->which_value_type ==
+             google_firestore_v1_Value_bytes_value_tag) {
+    return EvaluateResult(ResultType::kBytes, std::move(value));
+  } else if (value->which_value_type ==
+             google_firestore_v1_Value_reference_value_tag) {
+    return EvaluateResult(ResultType::kReference, std::move(value));
+  } else if (value->which_value_type ==
+             google_firestore_v1_Value_geo_point_value_tag) {
+    return EvaluateResult(ResultType::kGeoPoint, std::move(value));
+  } else if (model::IsArray(*value)) {
+    return EvaluateResult(ResultType::kArray, std::move(value));
+  } else if (model::IsVectorValue(*value)) {
+    // vector value must be before map value
+    return EvaluateResult(ResultType::kVector, std::move(value));
+  } else if (model::IsMap(*value)) {
+    return EvaluateResult(ResultType::kMap, std::move(value));
+  } else {
+    return EvaluateResult(ResultType::kError, {});


nit: you can use brace initialization for all of EvaluateResult(...) here. e.g.

return {ResultType::kBoolean, std::move(value)};

ehsannas · 2025-07-09T00:11:07Z

Firestore/core/src/core/expressions_eval.h

+/** Represents the result of evaluating an expression. */
+class EvaluateResult {
+ public:
+  enum class ResultType {


Presumably we'll need to add the Extended Types here in the future? :'-)

ehsannas · 2025-07-09T00:13:09Z

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

+
+  auto x = results.size();
+  EXPECT_EQ(x, 1);
+  // EXPECT_THAT(RunPipeline(ppl, {doc1, doc2, doc3}), Returns({doc1}));


commented code.

Also need to add a unit test for each kind of EvaluableExpr

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

wu-hui force-pushed the wuandy/RealPpl_2 branch 2 times, most recently from 7c6e249 to f7cc868 Compare May 14, 2025 23:38

wu-hui force-pushed the wuandy/RealPpl_2 branch from f7cc868 to f6ae9c2 Compare May 27, 2025 06:49

wu-hui force-pushed the wuandy/RealPpl_1 branch from 757acda to 838a5dc Compare May 27, 2025 06:49

wu-hui force-pushed the wuandy/RealPpl_2 branch 2 times, most recently from e43ea96 to c950299 Compare July 3, 2025 17:38

wu-hui force-pushed the wuandy/RealPpl_1 branch from 838a5dc to c401c69 Compare July 3, 2025 17:38

[realppl 2] Minimalistic ppl offline evaluation

21facb1

wu-hui force-pushed the wuandy/RealPpl_2 branch from c950299 to 21facb1 Compare July 7, 2025 17:19

wu-hui requested review from cherylEnkidu and ehsannas July 7, 2025 17:19

wu-hui assigned cherylEnkidu and ehsannas Jul 7, 2025

ehsannas reviewed Jul 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[realppl 2] Minimalistic ppl offline evaluation #14827

[realppl 2] Minimalistic ppl offline evaluation #14827

Uh oh!

wu-hui commented May 7, 2025

Uh oh!

gemini-code-assist bot commented May 7, 2025

Uh oh!

google-oss-bot commented May 7, 2025

Uh oh!

ehsannas Jul 8, 2025

Uh oh!

ehsannas Jul 8, 2025

Uh oh!

ehsannas Jul 8, 2025

Uh oh!

ehsannas Jul 8, 2025

Uh oh!

ehsannas Jul 8, 2025

Uh oh!

ehsannas Jul 8, 2025

Uh oh!

ehsannas Jul 8, 2025

Uh oh!

ehsannas Jul 9, 2025

Uh oh!

ehsannas Jul 9, 2025

Uh oh!

ehsannas Jul 9, 2025

Uh oh!

Uh oh!

		return EvaluateResult(ResultType::kUnset,
		nanopb::Message<google_firestore_v1_Value>());

[realppl 2] Minimalistic ppl offline evaluation #14827

Are you sure you want to change the base?

[realppl 2] Minimalistic ppl offline evaluation #14827

Uh oh!

Conversation

wu-hui commented May 7, 2025

Uh oh!

gemini-code-assist bot commented May 7, 2025

Uh oh!

google-oss-bot commented May 7, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!