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

Refactor RelationalProjectionExpressionVisitor #7767

Conversation

tuespetre
Copy link
Contributor

More accurate marking of RequiresClientProjection (less false positives), which results in less duplicate visitation of child query models in many cases

Note that the few tests producing larger SQL queries are due to unnecessary subquery pushdown, because now that RequiresClientProjection is marked more accurately, LiftSubQuery is able to lift them. See the related issue: #7766

@dnfclas
Copy link

dnfclas commented Mar 2, 2017

@tuespetre,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@tuespetre tuespetre force-pushed the relational-projection-expression-visitor branch 2 times, most recently from ece6c15 to 6e838e7 Compare March 7, 2017 15:42
@tuespetre tuespetre mentioned this pull request Mar 7, 2017
@tuespetre tuespetre force-pushed the relational-projection-expression-visitor branch 3 times, most recently from 4ec2d54 to 1ba0c94 Compare March 10, 2017 22:01
@tuespetre tuespetre force-pushed the relational-projection-expression-visitor branch 3 times, most recently from 6ce36d4 to f654e1b Compare March 15, 2017 03:48
@tuespetre
Copy link
Contributor Author

@smitpatel 🆙📅, rebased

More accurate marking of RequiresClientProjection (less false positives), which results in less duplicate visitation of child query models in many cases

Fixes dotnet#7844
@tuespetre tuespetre force-pushed the relational-projection-expression-visitor branch from f654e1b to 4442d1a Compare March 16, 2017 17:05
@tuespetre
Copy link
Contributor Author

@smitpatel @maumar @anpete any chance of looking at this one soon? Waiting on it for #7697 and frequently needing to rebase, etc.

@maumar
Copy link
Contributor

maumar commented Mar 17, 2017

@tuespetre At present @smitpatel is working on a significant refactoring of SelectExpression - it's probably best to hold off on the GROUP BY work for now (constant rebasing etc.). In fact, I would hold of on anything that touches SelectExpression - lot of stuff is going to change in that area, referential integrity of SelectExpression trees will be vastly improved and you won't be able to make adhoc changes to ColumnExpression, aliases etc. If you have some ideas for query model optimizations, those would be the best to focus on for now (like the several QM optimizations that you recently submitted - I will look at them as soon as I can)

@smitpatel
Copy link
Contributor

From #7950 description

Investigate visiting NewExpression in RelationalProjectionExpressionVisitor. At present we visit it twice. The only info we get out of first visit is about client projection. We throw away translated expression. (this would fix #7844 )

Just tweaking around VisitNew to avoid visiting twice would fix #7844 and other changes in tests. It seems unnecessary to split out Member/MethodCall visiting part.

@smitpatel
Copy link
Contributor

Closing this PR in favour of #8072
The test Where_subquery_anon fails to translate mainly due to RequireClientProjection flag being true due to anonymous projection. This blocks lifting of the SelectExpression. The flag is only set in the first visit of NewExpression where we don't utilize the translated expression. We need to call VisitNew method to save the member bindings so that we can lift the select expression and bind with it further. We get better aliases if we visit only once by avoid same work. Also for NewExpression we do not need client evaluation flag. The projection shaper knows how to project them out on top level. And on inner level if their properties being used somewhere then we fail to bind it causing client eval anyway. Setting RequireClientProjection is not needed.

@smitpatel smitpatel closed this Apr 4, 2017
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.

4 participants