Skip to content

Ppl API #14513

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 40 commits into
base: cheryllin/ppl
Choose a base branch
from
Open

Ppl API #14513

wants to merge 40 commits into from

Conversation

cherylEnkidu
Copy link
Contributor

No description provided.

@google-oss-bot
Copy link

google-oss-bot commented Feb 28, 2025 •
edited
Loading

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

Generated by 🚫 Danger

@cherylEnkidu cherylEnkidu changed the base branch from cheryl/pipeline to cheryllin/ppl February 28, 2025 03:34
wu-hui
wu-hui reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, some minor nits.

Firestore/core/src/api/pipeline.cc
@@ -40,6 +40,20 @@ void Pipeline::execute(util::StatusOrCallback<PipelineSnapshot> callback) {
this->firestore_->RunPipeline(*this, std::move(callback));
}

google_firestore_v1_Value Pipeline::to_proto() const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used?

Serializer::EncodePipeline does the same thing. If you want to move it here, we should update the logic there.

Firestore/core/src/api/stages.cc Outdated
: expr_(std::move(expr)), field_name_(absl::nullopt) {
}
ReplaceWith::ReplaceWith(std::string field_name)
: expr_(nullptr), field_name_(std::move(field_name)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this stage work? I am not sure i understand the implementation. Is it:

  1. if expr_ is set, and if it is a map, it will replace the input document?
  2. if field_name_ is set, it is evaluated against the document, and if it is a map it will replace the input document?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I make some mistakes here and make it overly complicated. I have change the implementation in the latest commit, which basically convert "field_name" to Expr in the Swift side.

Firestore/core/src/api/stages.h
: orders_(std::move(orders)) {
}
~SortStage() override = default;

google_firestore_v1_Pipeline_Stage to_proto() const override;

private:
std::vector<Ordering> orders_;
std::vector<std::shared_ptr<Ordering>> orders_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a pointer is needed here? I think we only needed if you want polymorphism?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since in objective C file, if the c++ class is a class member field, it can only be created using pointer type. For example,

@implementation FIRFunctionExprBridge {
  Ordering cpp_order;
}

will lead to compile error.
So use shared_ptr will make code a lot easier.

Firestore/Swift/Tests/Integration/PipelineTests.swift
.collection(path: "/foo")
.where(eq(field("foo"), constant("bar")))
.collection("/foo")
.where(Field("foo") == Constant("bar"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we would move away from operator overload?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have changed it in the new commit. However, I still want to keep && , ||, and !. I will discuss with Nick later this week.

Firestore/Swift/Tests/Integration/PipelineTests.swift
"awards": ["hugo": true, "nebula": true],
],
]

@available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *)
class PipelineIntegrationTests: FSTIntegrationTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these tests pass locally at least?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all three tests passed test against nightly.

Firestore/Swift/Tests/Integration/PipelineApiTests.swift

import FirebaseFirestore

final class PipelineTests: FSTIntegrationTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we need to keep this file around. It serves a purpose as a way to check for compilation errors, but we can just move them to the test below and actually run them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree these tests here can be convert to integration tests which verify the result. However, I would like the Swift integration test file consistent with other platforms in order for easy maintenance in the future.

Firestore/Swift/Source/Helper/PipelineHelper.swift
// See the License for the specific language governing permissions and
// limitations under the License.

enum Helper {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why an enum instead of a struct or class? Is this a swift convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Helper class only contains "static" methods, it should not be construct by itself. Since it is used internally, if we have other use case in the future, we can change it to struct.

Firestore/Swift/Source/SwiftAPI/Pipeline/Expr.swift
import Foundation

// TODO: the implementation of `Expr` is not complete
public protocol Expr: Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these should have public documentation, with code samples.

Firestore/Swift/Source/SwiftAPI/Pipeline/Pipeline.swift
}
}

/// Adds new fields to outputs from previous stages.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe other platforms include code samples of how to use this stage. Please add them here as well.

@wu-hui wu-hui assigned cherylEnkidu and unassigned wu-hui Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-hui wu-hui wu-hui left review comments

Assignees

@cherylEnkidu cherylEnkidu

Labels
api: analytics api: appcheck api: appdistribution api: auth api: core api: crashlytics api: database api: dynamiclinks api: firestore api: functions api: inappmessaging Firebase In App Messaging api: installations api: messaging api: performance api: remoteconfig api: storage api: vertexai public-api-change release-tooling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cherylEnkidu @google-oss-bot @wu-hui