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

Query Engine: Performance Improvement by removing implicit ORDER BY #178

Closed
idan opened this issue Nov 12, 2019 · 6 comments · Fixed by #223
Closed

Query Engine: Performance Improvement by removing implicit ORDER BY #178

idan opened this issue Nov 12, 2019 · 6 comments · Fixed by #223
Assignees
Milestone

Comments

@idan
Copy link

idan commented Nov 12, 2019

After prod/eng discussion, decision is to not ORDER BY unless explicitly requested in photon.

See relevant notion: https://www.notion.so/prismaio/Pagination-Perf-Improvements-77fb5b839a0a4c79bd05a746686e88f3

Spec: Here is the relevant part of the Photon JS spec.

Description: Currently we add an implicit order by on the id field to every query that does not specify an explicit order by. On some databases this can be a huge performance impact that we do not want to incur on every request. Hence we will not use an implicit order by anymore.

@mavilein
Copy link
Member

Initial Breakdown of Tasks

  • remove implicit order by in connector implementation
  • adjust tests that probably break as a regression because they rely on implicit ordering
  • verify gains through benchmarking

@mavilein mavilein changed the title Pagination Perf Improvements: Do not ORDER BY Query Engine: Performance Improvement by removing implicit ORDER BY Nov 18, 2019
@janpio janpio added this to the Preview 18 milestone Nov 22, 2019
@do4gr
Copy link
Member

do4gr commented Nov 26, 2019

We now removed the implicit orderBy: id_ASC that was included in all queries.

For these cases we are still doing it:

  • when there is a explicit orderBy on a non-unique field, we use it as a tiebreaker
  • when any of the other query arguments that call for a stable ordering are used (first, last, skip, before, after)
    • in these cases the user would always have to manually put it in order to get usable results, therefore we just keep doing it implicitly

Basically, the intention is to only use it to ensure stable ordering if the user is clearly caring about the ordering as far as we can tell from the query arguments, so using anything but filter from this:

pub struct QueryArguments {
    pub skip: Option<i64>,
    pub after: Option<GraphqlId>,
    pub first: Option<i64>,
    pub before: Option<GraphqlId>,
    pub last: Option<i64>,
    pub filter: Option<Filter>,
    pub order_by: Option<OrderBy>,
}

@matthewmueller
Copy link
Contributor

matthewmueller commented Nov 26, 2019

Hey @do4gr – thanks for fixing this up! Is there already a PR/commit for this?

when there is a explicit orderBy on a non-unique field, we use it as a tiebreaker

Can you give me a quick example of this? I'm a bit confused by what's special about explicit orderBy on a non-unique field vs. explicit orderBy in the normal case.

when any of the other query arguments that call for a stable ordering are used (first, last, skip, before, after)

I think this makes sense, but to solidify my understanding, do explicit orderBy's override the implicit id_ASC?

For example:

Use cases with first/skip/last/etc
no orderBy implicit ASC by ID
explicit ASC orderBy explicit ASC by orderBy column
explicit DESC orderBy explicit DESC by orderBy column

@do4gr
Copy link
Member

do4gr commented Nov 27, 2019

Re: your first question: If you order by first_name and you would repeat the query several times you could get different orders since first_name is not unique i.e. (Paul Smith, Paul Walker) and then (Paul Walker, Paul Smith). In these cases we will add an implicit ordering by id as tiebreaker, so the sql will be Order By first_name ASC, id ASC. This ensures that the ordering is stable. If you order by something unique i.e. SSN the secondary ordering is not necessary as a tiebreaker and we therefore leave it out.

@do4gr
Copy link
Member

do4gr commented Nov 27, 2019

Use cases with first/skip/etc without first/skip/etc
no orderBy ASC by ID no ordering
explicit ASC orderBy unique explicit ASC by orderBy column explicit ASC by orderBy column
explicit ASC orderBy non-unique explicit ASC by orderBy column, ASC by ID explicit ASC by orderBy column, ASC by ID

I think this better represents the different cases. Is this now clearer?

ASC, DESC on the orderBy field are not affected by the implicit ID ordering.

The implicit ordering is only there as tiebreaker AND only if there can be ties AND if you seem to care (because you used first/last...).

@matthewmueller
Copy link
Contributor

Sorry for the delayed response! yes that's perfect, I just wanted to double-check that the implicit cases weren't overriding the explicit cases 👍 💯

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 a pull request may close this issue.

5 participants