Skip to content

Conversation

@zrisher
Copy link

@zrisher zrisher commented May 24, 2018

Resolves #4271
Impact: minor
Type: performance

Issue

The query produced by createAggregate() performs its projection before limiting the result set, and its sort can't use indices. See #4271

Solution

Reorder the query.

Breaking changes

None.

Testing

  1. Test the query speed for a limited query with a large collection of large orders with and without this change.

@zrisher
Copy link
Author

zrisher commented May 25, 2018

The snyk-security test failed with this error:

snyk test requires an authenticated account. Please run `snyk auth` and try again.

Is this something I need to fix?

@aldeed aldeed changed the base branch from master to release-1.13.0 June 1, 2018 20:38
@spencern
Copy link
Contributor

@zrisher thanks for this PR. The snyk failure is because this PR is coming from your fork of the project I think, and you shouldn't need to worry about that.

@brent-hoover brent-hoover changed the base branch from release-1.13.0 to release-1.14.0 June 25, 2018 21:52
@brent-hoover
Copy link
Collaborator

@zrisher I'd like to get this performance improvement in the next release but it looks like you need to resolve some conflicts

Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

I did not do any testing, but the code change LGTM

@spencern
Copy link
Contributor

One conflict here. Would love to get this merged

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Need conflicts resolved

@zrisher zrisher force-pushed the fix-orders-aggregate-order branch from c02097c to fee9791 Compare July 25, 2018 23:29
@zrisher zrisher force-pushed the fix-orders-aggregate-order branch from fee9791 to 04d723d Compare July 25, 2018 23:43
@zrisher
Copy link
Author

zrisher commented Jul 25, 2018

@zenweasel Rebased to latest release-1.14.0

@aldeed
Copy link
Contributor

aldeed commented Jul 30, 2018

Still LGTM

@brent-hoover
Copy link
Collaborator

@zrisher I'm still not able to fast-forward a local branch to test this locally. Could you possibly update this again?

@aldeed aldeed changed the base branch from release-1.14.0 to release-1.15.0 August 2, 2018 03:13
@aldeed
Copy link
Contributor

aldeed commented Aug 16, 2018

I made a new PR for this change: #4555

@aldeed aldeed closed this Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants