Optimize multiple consecutive LIMITs#35384
Conversation
|
I believe the same optimization should be done for Cosmos. |
| } | ||
| else if (Limit != null) | ||
| { | ||
| PushdownIntoSubquery(); |
There was a problem hiding this comment.
we could actually aim for never doing a pushdown and instead replace the limit with LEAST(Limit, sqlExpression, but I am unsure if this is easy to do here (in this context we don't have access to TranslateMin)
There was a problem hiding this comment.
And you can't guarantee LEAST is supported at this stage
There was a problem hiding this comment.
I am experimenting with:
- adding a
SelectExpression.SetLimitmethod - replacing the
ApplyLimitinvocations withSetLimits as they are done from a context that also invokesGenerateLeast(and if that returnsnull, I can easily fall back toApplyLimit)
There was a problem hiding this comment.
I'm probably in the minority but it probably will break EFCore.Jet. I can only have a constant expression for TOP. And since I don't have LIMIT/OFFSET, TOP= LIMIT. And I do have a translation for LEAST
There was a problem hiding this comment.
uhm... how does that work with non-constant Take()? (parameters are likely the most common, but in theory any kind of expression is allowed there)
There was a problem hiding this comment.
I'm probably in the minority but it probably will break EFCore.Jet. I can only have a constant expression for TOP. And since I don't have LIMIT/OFFSET, TOP= LIMIT. And I do have a translation for LEAST
Also, note that as long as the code is only LIMITing on constants, you still get a (combined) constant as the new LIMIT.
LEAST/MIN are emitted when combining non-constants (or a constant and a non-constant), so it might be possible to handle it in the same way as you are doing now (I would assume something with row numbers)
There was a problem hiding this comment.
I throw a Not Translated error (no row_number or equivlalent in ms access).
But I'm happy as long as a constant expression stays as a plain constant expression
There was a problem hiding this comment.
uhm... how does that work with non-constant
Take()? (parameters are likely the most common, but in theory any kind of expression is allowed there)
Just before I sent it off to Oledb/ODBC, the query is modified to inline the parameter. If it refers to a column, well it just fails
There was a problem hiding this comment.
Guys, one of my major gripes with the current query pipeline design is the non-openness of SelectExpression, i.e. code there isn't overridable by providers; this case with integrating LEAST() instead of pushing down is a great example of why that's needed. I do have a plan in mind: I'd like to first make SelectExpression fully mutable, and then extract most/all actual logic out of it, so that SelectExpression becomes a regular, pure, immutable expression just like any other. The logic could go to QueryableMethodTranslatingEV, but some of these operations are probably also needed in postprocessing, so possible a separate service is maybe needed, that would be accessible from everywhere and be overridable by the provider. I hope to make at least some progress in this general direction in 10 (e.g. making SelectExpression fully immutable).
Until then, we're a bit stuck hacking around... @ranma42 what you've done in this draft looks good to me - let me know when this is ready for review.
Fixes dotnet#35383 for trivial cases (constants/literals and repeated expressions).
The new translation makes it work also in Sqlite.
ranma42
force-pushed
the
simplify-collection-trivial
branch
from
e0e91fd to
08c8569
Compare
| /// Sets a new limit of the <see cref="SelectExpression" /> to limit the number of rows returned in the result set. | ||
| /// </summary> | ||
| /// <param name="sqlExpression">An expression representing limit row count.</param> | ||
| public void SetLimit(SqlExpression sqlExpression) |
There was a problem hiding this comment.
Please add [EntityFrameworkInternal] as while this looks good for now, I don't see this API sticking around (e.g. once SelectExpression is immutable).
There was a problem hiding this comment.
Indeed I assume that making SelectExpression immutable will change its API quite a bit (which might also be a very good opportunity to re-organize some code around it 😉 )
There was a problem hiding this comment.
Absolutely... That's high up on my list for 10.
| FROM [EntityOnes] AS [e] | ||
| OUTER APPLY ( | ||
| SELECT TOP(1) [s].[Id], [s].[Name] | ||
| LEFT JOIN ( |
There was a problem hiding this comment.
Interesting change... The LEFT JOIN is obviously better than the OUTER APPLY (and also unlocks SQLite which doesn't support APPLY), but then we get the row number inside... Looks OK in any case.
There was a problem hiding this comment.
I am afraid we have that in multiple places; I might eventually investigate, but seems like something that is already happening :\
There was a problem hiding this comment.
Yeah. BTW CROSS/OUTER APPLY vs. ROW_NUMBER() is something that's already tracked in #30450, it's the kind of deep performance investigations we haven't gotten around to yet. I hope that once the basic query architecture refactoring is farther ahead, all this will be much easier to tackle.
3 participants
Fixes #35383 for the simple cases of constants/literals and repeated expressions.