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

Index hint for nested queries (bound to alias) #286

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VladimirKuzmin
Copy link

Summary:

Index hint should remember which table it was applied.
Solution:

  • on each aliases changing in query also change alias in index hint
  • at rewriting stage modify SQL according the alias

Checklist:

  • Docs updated, or N/A
  • Line added to HISTORY.rst, or N/A
  • Added extra items to checklist as applicable
  • All commits squashed into one.

Test Plan:

  • added tests for nested queries up to 3 level generated by Django ORM
  • added test for raw SQL query

Known issue:
For nested queries like this (without any JOIN):

SomeModel.objects.filter(pk__in=SomeModel.objects.all())

Django will not assign alias to any table, so index hint will be always applied to outer query.
But it looks like queries like this are useless.

@VladimirKuzmin VladimirKuzmin changed the title Index hint for nested queries Index hint for nested queries (bound to alias) May 8, 2016
@adamchainz
Copy link
Owner

This is nice and looks good. A quick glance doesn't reveal anything problematic, but I'll give it a bit more testing during the week!

sf-victor-lee pushed a commit to sendbird-graveyard/django-mysql that referenced this pull request Jul 28, 2018
@thebarbershop
Copy link

Can someone share the status of this patch? Is the issue addressed by some other PR, or is it just left unfixed for some reason?

@adamchainz
Copy link
Owner

I've been meaning to review this but it's quite a big chunk of stuff to understand at once (and after this time, rebase). And I've never had the issue myself, and I guess I've always had higher impact things to work on.

If you want to rebase it that would be a helpful start; even more helpful would be running it in your app (dev, or staigng, or even prod) and seeing if you encounter any issues not addressed by tests.

@thebarbershop
Copy link

Thanks for the update, Adam!

I'm currently working to upgrade a project from ancient times (Django 1.9 + A forked django-mysql 1.0.9 with this PR applied) so I can't contribute to the upstream at this point. But I'll be happy to return to this issue when I can.

Base automatically changed from master to main February 5, 2021 08:35
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.

3 participants