Skip to content

Fix: Aggregate query order-by normalization #7507

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
Merged
merged 8 commits into from
Aug 4, 2023
Merged

Fix: Aggregate query order-by normalization #7507

merged 8 commits into from
Aug 4, 2023

Conversation

MarkDuckworth
Copy link
Contributor

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

  • Read the contribution guidelines (CONTRIBUTING.md).
  • If this has been discussed in an issue, make sure to link to the issue here.
    If not, go file an issue about this before creating a pull request to discuss.

Testing

  • Make sure all existing tests in the repository pass after your change.
  • If you fixed a bug or added a feature, add a new test to cover your code.

API Changes

  • At this time we cannot accept changes that affect the public API. If you'd like to help
    us make Firebase APIs better, please propose your change in an issue so that we
    can discuss it together.

@MarkDuckworth MarkDuckworth requested review from a team as code owners July 31, 2023 23:51
@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 84fe871

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/firestore Patch
firebase Patch
@firebase/firestore-compat Patch

Not sure what this means? Click here to learn what changesets are.

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

@MarkDuckworth MarkDuckworth requested a review from a team as a code owner July 31, 2023 23:54
@MarkDuckworth MarkDuckworth changed the title Fix: Aggregate query order by normalization Fix: Aggregate query order-by normalization Jul 31, 2023
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 1, 2023 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (80a7f7c)Merge (7107aac)Diff
    browser370 kB371 kB+243 B (+0.1%)
    esm5356 kB356 kB+86 B (+0.0%)
    main569 kB569 kB+455 B (+0.1%)
    module370 kB371 kB+243 B (+0.1%)
    react-native371 kB371 kB+243 B (+0.1%)

  • @firebase/firestore-lite

    TypeBase (80a7f7c)Merge (7107aac)Diff
    browser110 kB110 kB+203 B (+0.2%)
    esm5107 kB107 kB+86 B (+0.1%)
    main151 kB152 kB+415 B (+0.3%)
    module110 kB110 kB+203 B (+0.2%)
    react-native110 kB110 kB+203 B (+0.2%)

  • bundle

    12 size changes

    TypeBase (80a7f7c)Merge (7107aac)Diff
    firestore (Persistence)301 kB301 kB+25 B (+0.0%)
    firestore (Query Cursors)239 kB239 kB+25 B (+0.0%)
    firestore (Query)236 kB236 kB+25 B (+0.0%)
    firestore (Read data once)224 kB224 kB+25 B (+0.0%)
    firestore (Realtime updates)226 kB226 kB+25 B (+0.0%)
    firestore (Transaction)203 kB203 kB+25 B (+0.0%)
    firestore (Write data)203 kB203 kB+25 B (+0.0%)
    firestore-lite (Query Cursors)83.1 kB83.1 kB+28 B (+0.0%)
    firestore-lite (Query)79.2 kB79.3 kB+28 B (+0.0%)
    firestore-lite (Read data once)61.3 kB61.3 kB+15 B (+0.0%)
    firestore-lite (Transaction)86.1 kB86.1 kB+15 B (+0.0%)
    firestore-lite (Write data)70.8 kB70.9 kB+15 B (+0.0%)

  • firebase

    TypeBase (80a7f7c)Merge (7107aac)Diff
    firebase-compat.js774 kB774 kB+16 B (+0.0%)
    firebase-firestore-compat.js340 kB340 kB+16 B (+0.0%)
    firebase-firestore-lite.js118 kB118 kB+203 B (+0.2%)
    firebase-firestore.js430 kB430 kB+243 B (+0.1%)

Test Logs

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

@google-oss-bot
Copy link
Contributor

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (80a7f7c)Merge (f355a46)Diff
    browser370 kB371 kB+243 B (+0.1%)
    esm5356 kB356 kB+86 B (+0.0%)
    main569 kB569 kB+455 B (+0.1%)
    module370 kB371 kB+243 B (+0.1%)
    react-native371 kB371 kB+243 B (+0.1%)

  • @firebase/firestore-lite

    TypeBase (80a7f7c)Merge (f355a46)Diff
    browser110 kB110 kB+203 B (+0.2%)
    esm5107 kB107 kB+86 B (+0.1%)
    main151 kB152 kB+415 B (+0.3%)
    module110 kB110 kB+203 B (+0.2%)
    react-native110 kB110 kB+203 B (+0.2%)

  • bundle

    12 size changes

    TypeBase (80a7f7c)Merge (f355a46)Diff
    firestore (Persistence)301 kB301 kB+25 B (+0.0%)
    firestore (Query Cursors)239 kB239 kB+25 B (+0.0%)
    firestore (Query)236 kB236 kB+25 B (+0.0%)
    firestore (Read data once)224 kB224 kB+25 B (+0.0%)
    firestore (Realtime updates)226 kB226 kB+25 B (+0.0%)
    firestore (Transaction)203 kB203 kB+25 B (+0.0%)
    firestore (Write data)203 kB203 kB+25 B (+0.0%)
    firestore-lite (Query Cursors)83.1 kB83.1 kB+28 B (+0.0%)
    firestore-lite (Query)79.2 kB79.3 kB+28 B (+0.0%)
    firestore-lite (Read data once)61.3 kB61.3 kB+15 B (+0.0%)
    firestore-lite (Transaction)86.1 kB86.1 kB+15 B (+0.0%)
    firestore-lite (Write data)70.8 kB70.9 kB+15 B (+0.0%)

  • firebase

    TypeBase (80a7f7c)Merge (f355a46)Diff
    firebase-compat.js774 kB774 kB+16 B (+0.0%)
    firebase-firestore-compat.js340 kB340 kB+16 B (+0.0%)
    firebase-firestore-lite.js118 kB118 kB+203 B (+0.2%)
    firebase-firestore.js430 kB430 kB+243 B (+0.1%)

Test Logs

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 1, 2023 •
edited
Loading

Size Analysis Report 1

This report is too large (388,630 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/An9R6Elx6s.html

ehsannas
ehsannas approved these changes Aug 1, 2023
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.

LGTM. A few minor improvements pointed out below.

egilmorez
egilmorez reviewed Aug 2, 2023
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Couple of nits, thanks!

@MarkDuckworth MarkDuckworth merged commit b395277 into master Aug 4, 2023
@MarkDuckworth MarkDuckworth deleted the markduckworth/non-normalized-aggregation-order-by branch August 4, 2023 14:53
@google-oss-bot google-oss-bot mentioned this pull request Aug 15, 2023
Merged
@firebase firebase locked and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@egilmorez egilmorez egilmorez left review comments

@ehsannas ehsannas ehsannas approved these changes

Assignees

@MarkDuckworth MarkDuckworth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MarkDuckworth @google-oss-bot @ehsannas @egilmorez