Skip to content

[3/4] Integrate realtime ppl into SDK cache management and tests #8910

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 1 commit into
base: wuandy/RealPpl_2
Choose a base branch
from
Open

[3/4] Integrate realtime ppl into SDK cache management and tests #8910

wants to merge 1 commit into from

Conversation

@wu-hui
Copy link
Contributor

@wu-hui wu-hui commented Apr 8, 2025

No description provided.

@wu-hui wu-hui requested review from a team as code owners April 8, 2025 22:08
@changeset-bot
Copy link

changeset-bot bot commented Apr 8, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: e64cf0b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@wu-hui wu-hui force-pushed the wuandy/RealPpl_2 branch from 6710057 to 0c570ab Compare April 14, 2025 18:34
@wu-hui wu-hui force-pushed the wuandy/RealPpl_3 branch from 33caa24 to 5065573 Compare April 14, 2025 18:34
MarkDuckworth
MarkDuckworth reviewed Apr 14, 2025
View reviewed changes
packages/firestore/src/api/pipeline_impl.ts
*/
export function _onRealtimePipelineSnapshot(
pipeline: RealtimePipeline,
options: SnapshotListenOptions,
Copy link
Contributor

@MarkDuckworth MarkDuckworth Apr 14, 2025

Choose a reason for hiding this comment

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

Our design doc was updated to use PipelineListenOptions, which will add serverTimestamps config and remove source. Though, if we want to support source, we can. go/firestore-api-realtime-pipelines

Copy link
Contributor

@MarkDuckworth MarkDuckworth Apr 14, 2025

Choose a reason for hiding this comment

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

This can be refactored in a future PR, as long as we track it.

Copy link
Contributor

@MarkDuckworth MarkDuckworth Apr 14, 2025

Choose a reason for hiding this comment

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

More generally, we will need to put together a design for how pipeline options are passed to onSnapshot.

Copy link
Contributor Author

@wu-hui wu-hui May 29, 2025

Choose a reason for hiding this comment

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

Right, I forgot about serverTimestamp here. Let's proceed without this for now. I added one item to the tracking sheet.

packages/firestore/src/api/reference_impl.ts Outdated
let observer: PartialObserver<ViewSnapshot>;
let firestore: Firestore;
let internalQuery: InternalQuery;
let internalQuery: QueryOrPipeline;
Copy link
Contributor

@MarkDuckworth MarkDuckworth Apr 14, 2025

Choose a reason for hiding this comment

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

This type can be reverted back, since we have separated the implementations of listeners for pipelines and query

Copy link
Contributor Author

@wu-hui wu-hui May 29, 2025

Choose a reason for hiding this comment

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

Done.

packages/firestore/src/api/reference_impl.ts
export function onPipelineSnapshot<
AppModelType,
DbModelType extends DocumentData
>(
Copy link
Contributor

@MarkDuckworth MarkDuckworth Apr 14, 2025

Choose a reason for hiding this comment

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

We need to drop generics from this new api

Copy link
Contributor Author

@wu-hui wu-hui May 29, 2025

Choose a reason for hiding this comment

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

Done.

packages/firestore/src/local/local_documents_view.ts
return this.retrieveMatchingLocalDocuments(
overlays,
remoteDocuments,
doc => pipelineMatches(pipeline, doc as MutableDocument)
Copy link
Contributor

@MarkDuckworth MarkDuckworth Apr 14, 2025

Choose a reason for hiding this comment

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

It isn't clear to me, but do we remove offsets and limits from the pipeline argument? If not, could those cause false negatives?

Copy link
Contributor Author

@wu-hui wu-hui May 29, 2025

Choose a reason for hiding this comment

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

We do not support offset in offline eval.

Limit will work as intended here, all docs matching the pipeline without limit will return, and limit will be applied later at view layer.

We had discussion on whether we should move that logic into here, but no action had been taken so far.

packages/firestore/src/local/local_store_impl.ts Outdated
queryCollectionGroup(query),
documents
);
// TODO(pipeline): this needs to be adapted to support pipelines as well
Copy link
Contributor

@MarkDuckworth MarkDuckworth Apr 14, 2025

Choose a reason for hiding this comment

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

reminder on this TODO

Copy link
Contributor Author

@wu-hui wu-hui May 29, 2025

Choose a reason for hiding this comment

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

Addressed.

@wu-hui wu-hui force-pushed the wuandy/RealPpl_2 branch from 0c570ab to 6f0e635 Compare April 24, 2025 13:59
@wu-hui wu-hui force-pushed the wuandy/RealPpl_3 branch from 5065573 to 5cb4499 Compare April 24, 2025 13:59
@wu-hui wu-hui force-pushed the wuandy/RealPpl_2 branch from 6f0e635 to 58cfd1d Compare April 29, 2025 19:45
@wu-hui wu-hui force-pushed the wuandy/RealPpl_3 branch from 5cb4499 to 90b6e29 Compare April 29, 2025 19:45
@wu-hui wu-hui force-pushed the wuandy/RealPpl_3 branch 2 times, most recently from 5cbd519 to d1b4016 Compare May 16, 2025 18:09
@wu-hui wu-hui force-pushed the wuandy/RealPpl_2 branch from 58cfd1d to c9fce63 Compare May 29, 2025 06:05
@wu-hui wu-hui force-pushed the wuandy/RealPpl_3 branch from d1b4016 to 3ef4e6f Compare May 29, 2025 06:05
@wu-hui wu-hui force-pushed the wuandy/RealPpl_3 branch 3 times, most recently from a6e4c5f to aa27adc Compare October 17, 2025 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarkDuckworth MarkDuckworth MarkDuckworth left review comments

Assignees

@MarkDuckworth MarkDuckworth

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wu-hui @MarkDuckworth