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

Prevented unnecessary distinct() call in SearchFilter. #3938

Merged
merged 2 commits into from Jun 23, 2016

Conversation

charettes
Copy link
Contributor

Calling distinct can have disastrous performance impact. This patch makes sure we only call it if required.

@charettes charettes changed the title Prevented unnecessary distint() call in SearchFilter. Prevented unnecessary distinct() call in SearchFilter. Feb 16, 2016
@xordoquy
Copy link
Collaborator

Looks like a very promising change. Thanks for work !


def must_call_distinct(self, opts, lookups):
"""
Return True if 'distinct()' should be used to query the given lookups.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a link here to the Django WONTFIX ticket, or documentation, or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's undergoing work to allow using subqueries to filter m2m but I'm unsure about which ticket you are referring to. This was mostly inspired by how the Django's admin handle a similar case.

Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure about which ticket you are referring to.

No, I'm not sure either - I thought I'd remembered a django ticket referring to this that closed as WONTFIX, but I don't see anything like that now.

@codecov-io
Copy link

Current coverage is 91.49%

No coverage report found for master at 79dad01.

Powered by Codecov. Last updated by 79dad01...93897b9

@tomchristie tomchristie merged commit 90bb0c5 into encode:master Jun 23, 2016
@tomchristie
Copy link
Member

Great stuff, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants