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

Fix index hint for nested queries #284

Closed
wants to merge 1 commit into from

Conversation

VladimirKuzmin
Copy link

@VladimirKuzmin VladimirKuzmin commented May 2, 2016

Summary:
Using index hint on a query where indexed table represented only in subquery (e.g. if you use Paginator, some tables might be ommited in count() query) causes syntax error (the hint added before table alias).

class Post(Model):
    author = ForeignKey('User')
    text = TextField()
    published = DateTimeField()

class TaggedPost(Post):
    tags = ManyToManyField('Tag')

# use subquery instead of distinct() for optimization purposes; distinct() causes filesort
post_ids = TaggedPost.objects.filter(tags__in=tags_list).values('pk')
posts = TaggedPost.objects.filter(pk__in=post_ids).use_index('published_idx', table_name='post')
# next line generate invalid query
count = posts.count()

Fix rely on weak assumption that django uses aliases for tables in nested queries and these aliases match [A-Z]+\d+. From now on use_index() doesn't apply hint to any aliased tables.

Checklist:

  • Line added to HISTORY.rst
  • All commits squashed into one.
  • Self added to AUTHORS.rst (or you can opt out)

Test Plan:
Tested in my project with and without nested queries. Added test case.

@adamchainz
Copy link
Owner

Hey, this looks awesome. Could you please add a line to HISTORY.rst explaining that the fix was added, and add yourself to AUTHORS.rst if you like!

I'm going to have a bit of a test before I merge this too.

@adamchainz adamchainz self-assigned this May 3, 2016
@adamchainz
Copy link
Owner

Hey, just had a closer look. This needs more testing. Specifically I'd like to see:

  • a test with the query hints in the inner query
  • a test with hints in the inner and outer queries
  • a test with the same table in the inner and outer queries, each with hints, and the query hint from the inner query applying to the inner one, the hint from the outer one applying to the outer one (I suspect this won't work :( )
  • tests at the QuerySet level too - probably versions of all the above test cases. It's fine if this works for the SQL that Django generates for subqueries atm, but it might not be in the future, and we need to be protected against that. Such tests are in test_models.py::QueryHintTests

@VladimirKuzmin
Copy link
Author

Hey!
I'll add more tests.

@VladimirKuzmin
Copy link
Author

VladimirKuzmin commented May 8, 2016

Hey, Adam!
You're right. This solution won't work for any inner queries.
I considered better solution for this problem. Have a look at PR #286.

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.

2 participants