-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Remove ORDER BY applied to singleton queries #35214
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
Conversation
I started working on this again after #34097 was merged :) |
eaaaa40
to
a588de8
Compare
I added a test and based this on top of #35384, so that it actually performs the desired simplifications (as visible in |
a588de8
to
a3c23ca
Compare
a3c23ca
to
50dc57b
Compare
@ranma42 thanks as always for the work and sorry it took so long to review. I'm coming out of a very intensive work period working on other stuff and will try to review PRs etc. This change seems fine to me, though IIUC the kind of queries this optimize are "silly", if written by an end user; in other words, a LINQ query (EF or otherwise) containing We generally avoid going too much into this kind of optimization, where EF tries to catch badly-written queries and improve their translations; it's better for users to write better LINQ queries. It's true that in some cases such optimizations can kick in as a result of processing we ourselves do in the query pipeline (and not because of "silly" input queries); that's indeed valuable, though is that the case here? Bottom line: I'm totally fine merging this as the change adds minimal complexity, but I'm just asking in case I'm missing something (or you want to discuss). Let me know what you think. |
@ranma42 any comment on the above? |
oops, sorry for the delayed reply! I agree that these queries are unlikely in a normal codebase: OTOH this is a simple check that brings the expressions a little more towards a "normalized" form.
It was not designed for this, although given the code that ended up interacting with (see both the changes and the interaction with #34097) it might indeed trigger in some split queries 🤔 I will try and have a look (I am thinking about something like I will investigate the split query case and report back; in any case, either decision (merge or close) works for me 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ranma42, makes sense. Merging this in any case.
Fixes #34831.