Skip to content
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.

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

Remove SimpleQueryEngine #4117

Merged
merged 3 commits into from Nov 25, 2020
Merged

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Nov 24, 2020

This PR addresses a long outstanding TODO and removes SimpleQueryEngine, which existed to make IndexFreeQueryEngine optional during development. This PR makes IndexFreeQueryEngine the only QueryEngine.

@changeset-bot
Copy link

changeset-bot bot commented Nov 24, 2020

⚠️ No Changeset found

Latest commit: 335b391

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Nov 24, 2020

Size Analysis Report

Affected Products

No changes between base commit (c76e1ff) and head commit (059facc).

Test Logs

@schmidt-sebastian
Copy link
Contributor Author

Note: The diff here is kinda bad, but I renamed index_free_query_engine.ts to query_engine.ts and edited some comments. The diff is clearer in IntelliJ/Git directly.

Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

LGTM, with two ultra-minor nits.

* - Limit queries where a document edit may cause the document to sort below
* another document that is in the local cache.
*
* - Queries that have never been CURRENT or free of Limbo documents.
Copy link
Contributor

Choose a reason for hiding this comment

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

ultranit: I think that the "L" in "Limbo" can be lowercase, since "limbo" is not a proper noun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I do consider "Limbo" more of a concept, rather than just the state that it refers to. Fixed either way though.

@@ -67,25 +66,27 @@ const MISSING_LAST_LIMBO_FREE_SNAPSHOT = SnapshotVersion.min();
* `getDocumentsMatchingQuery()` to detect index-free execution.
*/
class TestLocalDocumentsView extends LocalDocumentsView {
expectIndexFreeExecution: boolean | undefined;
exceptFullCollectionScan: boolean | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "exceptFullCollectionScan" should be "expectFullCollectionScan".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@schmidt-sebastian schmidt-sebastian merged commit 9c61afe into master Nov 25, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/removesimplequeryengine branch November 25, 2020 18:31
@firebase firebase locked and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants