Query: Lift more subqueries in the QueryOptimizer #7822
Conversation
|
@tuespetre, |
| @@ -76,6 +76,8 @@ public QueryOptimizer([NotNull] IModel model) | |||
| _queryAnnotations = queryAnnotations; | |||
|
|
|||
| VisitQueryModel(queryModel); | |||
|
|
|||
| new MainFromClauseFlatteningQueryModelVisitor(queryAnnotations).VisitQueryModel(queryModel); | |||
There was a problem hiding this comment.
How about overriding VisitMainFromClause here?
There was a problem hiding this comment.
Doesn't work, the SubQueryFlattener base class will not hit all of the SubQueryExpressions and it won't hit the MainFromClause unless its own conditions are met (no result operators, etc.)
There was a problem hiding this comment.
You can always override VisitMainFromClause apply your logic and if it doesn't work then call base. Question becomes if we want to do this merging in MainFromClause only or on AdditionalFromClause too (later may be better but probably needs some more tests to be added).
We are already overriding FlattenSubquery there you should be getting all subquery expressions.
There was a problem hiding this comment.
@smitpatel that is what I tried first. It doesn't get all of the subquery expressions, and the SubQueryFromClauseFlattener base class won't even visit the MainFromClause if there are result operators on the query model.
There was a problem hiding this comment.
@tuespetre - You are right. It does not get all subquery expressions. The dual dilemma of ExpressionVisitor & QueryModelVisitor (which forces @maumar to make recursive visitors)
There was a problem hiding this comment.
Since we are using Expression/QueryModelVisitor pattern, can you move visiter creation & entry calls to EntityQueryModelVisitor.OptimizeQueryModel method. QueryOptimizer is a visitor on query model. So it is a stand-alone thing. If we need any other visitor on QMV then we add visitor in optimize query model just like SubQueryMemberPushDown or EntityEqualityRewriting.
Rest of changes look good to accommodate it. Its really unfortunate that QueryOptimizer is suboptimal at present to deal with such cases.
tuespetre
force-pushed
the
query-optimizer
branch
2 times, most recently
from
f405bab to
4cd141d
Compare
Query models following a simple pattern of selecting from a subquery only to create a projection can often be flattened by lifting the subquery. These changes do that, which results in more efficient queries for such cases, and in the case of Relational, more compact SQL statements.
|
🆙📅 |
|
|
||
| if (querySourceReferenceExpression != null | ||
| && querySourceReferenceExpression.ReferencedQuerySource == groupJoinClause) | ||
| if (queryModel.BodyClauses.ElementAtOrDefault(index + 1) is AdditionalFromClause additionalFromClause |
There was a problem hiding this comment.
Just a side note: can we avoid having such cleanups in the PRs which fixes certain bugs/issues? It makes harder to review when finding what code path actually changed. We generally run clean up on whole code together so that we don't have to review it in detail.
There was a problem hiding this comment.
Want me to cut this part?
|
@smitpatel which test is regressing that you had mentioned on #6647? |
|
This PR is superseded by #7843 |
|
OK. There are still cases that the |
|
Thanks for pointing out. I will investigate into it. |
Query models following a simple pattern of selecting from a subquery
only to create a projection can often be flattened by lifting the
subquery. These changes do that, which results in more efficient
queries for such cases, and in the case of Relational, more compact
SQL statements.