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.

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

Add composite filters in support of OR queries #6385

Merged
merged 6 commits into from
Jul 26, 2022
Merged

Add composite filters in support of OR queries #6385

merged 6 commits into from
Jul 26, 2022

Conversation

MarkDuckworth
Copy link
Contributor

Add composite filters. Port of firebase/firebase-android-sdk#3290

@changeset-bot
Copy link

changeset-bot bot commented Jun 24, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: ed6a600

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 Jun 24, 2022 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (8260149)Merge (b58224b)Diff
    browser262 kB263 kB+1.17 kB (+0.4%)
    esm5325 kB327 kB+1.57 kB (+0.5%)
    main522 kB523 kB+1.83 kB (+0.4%)
    module262 kB263 kB+1.17 kB (+0.4%)
    react-native262 kB264 kB+1.17 kB (+0.4%)

  • @firebase/firestore-lite

    TypeBase (8260149)Merge (b58224b)Diff
    browser80.5 kB80.9 kB+373 B (+0.5%)
    esm596.2 kB96.7 kB+467 B (+0.5%)
    main135 kB136 kB+478 B (+0.4%)
    module80.5 kB80.9 kB+373 B (+0.5%)
    react-native80.7 kB81.1 kB+373 B (+0.5%)

  • bundle

    11 size changes

    TypeBase (8260149)Merge (b58224b)Diff
    firestore (Persistence)273 kB275 kB+1.06 kB (+0.4%)
    firestore (Query Cursors)210 kB211 kB+1.05 kB (+0.5%)
    firestore (Query)211 kB212 kB+1.16 kB (+0.5%)
    firestore (Read data once)200 kB201 kB+1.05 kB (+0.5%)
    firestore (Realtime updates)202 kB203 kB+1.05 kB (+0.5%)
    firestore (Transaction)184 kB185 kB+1.05 kB (+0.6%)
    firestore (Write data)183 kB184 kB+1.05 kB (+0.6%)
    firestore-lite (Query Cursors)67.9 kB67.9 kB+22 B (+0.0%)
    firestore-lite (Query)71.1 kB71.5 kB+376 B (+0.5%)
    firestore-lite (Transaction)80.1 kB80.0 kB-14 B (-0.0%)
    firestore-lite (Write data)65.3 kB65.2 kB-12 B (-0.0%)

  • firebase

    TypeBase (8260149)Merge (b58224b)Diff
    firebase-compat.js794 kB795 kB+1.15 kB (+0.1%)
    firebase-firestore-compat.js314 kB315 kB+1.15 kB (+0.4%)
    firebase-firestore-lite.js267 kB268 kB+506 B (+0.2%)
    firebase-firestore.js845 kB847 kB+2.13 kB (+0.3%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/VlOjMw1BJZ.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 24, 2022 •
edited
Loading

Size Analysis Report 1

This report is too large (546,853 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/0yhu7W56Y8.html

@MarkDuckworth MarkDuckworth changed the title Add composite filters Add composite filters in support of OR queries Jun 27, 2022
@MarkDuckworth MarkDuckworth marked this pull request as ready for review June 27, 2022 15:45
@MarkDuckworth MarkDuckworth requested a review from ehsannas June 27, 2022 15:45
@MarkDuckworth MarkDuckworth mentioned this pull request Jun 29, 2022
Merged
ehsannas
ehsannas requested changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

This is a great start, and generally looks good to me. Some nits/comments outlined below.

packages/firestore/src/core/target.ts Outdated Show resolved Hide resolved
packages/firestore/src/core/target.ts Outdated Show resolved Hide resolved
packages/firestore/src/core/target.ts Outdated Show resolved Hide resolved
packages/firestore/src/core/target.ts Show resolved Hide resolved
packages/firestore/src/core/target.ts Outdated Show resolved Hide resolved
packages/firestore/src/core/target.ts Outdated Show resolved Hide resolved
packages/firestore/src/lite-api/query.ts Show resolved Hide resolved
packages/firestore/src/lite-api/query.ts Outdated Show resolved Hide resolved
packages/firestore/src/lite-api/query.ts Show resolved Hide resolved
packages/firestore/src/lite-api/query.ts
* created with calls to {@link where}, {@link or}, or {@link and}.
* @returns The created {@link QueryConstraint}.
*/
export function or(...queryConstraints: QueryConstraint[]): QueryConstraint {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way to not expose and() and or() publicly yet (and still be able to invoke them in the tests) ?

Android makes that possible using @RestrictTo(RestrictTo.Scope.LIBRARY). But not sure if JS has a way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is by keeping it out of api.ts, it's still internal, but can be used for testing. Please correct me if you see a gap in my understanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep; just looked into it a bit more. (as a side, we also have "lite" APIs for the "Lite Web SDK").

Copy link
Contributor

Choose a reason for hiding this comment

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

@MarkDuckworth , IIUC based on @hsubox76 's comment, this comment still stands

@github-actions
Copy link
Contributor

github-actions bot commented Jul 18, 2022 •
edited
Loading

Changeset File Check ✅

  • No modified packages are missing from the changeset file.
  • No changeset formatting errors detected.

@MarkDuckworth MarkDuckworth requested a review from egilmorez as a code owner July 18, 2022 14:47
hsubox76
hsubox76 reviewed Jul 18, 2022
View reviewed changes
Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

Is there a reason the changeset was removed? I see a commit comment saying it doesn't require a release but I think it's good to add a changeset for any code changes that could theoretically affect how the code runs. And I see from the api-review md changes that it's actually affected the public types.

ehsannas
ehsannas reviewed Jul 18, 2022
View reviewed changes
Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

Sorry I missed this in my first review. Given that we do want to use 'and' and 'or', but we don't want to expose them publicly, we shouldn't be updating the .md files.

common/api-review/firestore-lite.api.md Show resolved Hide resolved
common/api-review/firestore.api.md Show resolved Hide resolved
@MarkDuckworth MarkDuckworth changed the base branch from master to markduckworth/or-queries July 25, 2022 21:42
@MarkDuckworth MarkDuckworth merged commit a722abc into markduckworth/or-queries Jul 26, 2022
@MarkDuckworth MarkDuckworth deleted the markduckworth/or-queries-pr-2 branch July 26, 2022 13:35
@firebase firebase locked and limited conversation to collaborators Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hsubox76 hsubox76 hsubox76 left review comments

@ehsannas ehsannas ehsannas approved these changes

@allspain allspain Awaiting requested review from allspain

@dwyfrequency dwyfrequency Awaiting requested review from dwyfrequency

@egilmorez egilmorez Awaiting requested review from egilmorez

Assignees

@ehsannas ehsannas

@dwyfrequency dwyfrequency

@hsubox76 hsubox76

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MarkDuckworth @google-oss-bot @ehsannas @hsubox76 @dwyfrequency