Stop remapping lambda bodies to deal with parameters

[roji]

When EF translates a lambda (e.g. Where(b => b.Name == foo), before actually translating the lambda body, we currently run "RemapLambdaBody"; this does a recursive search-and-replace pass, replacing instances of the ParameterExpression (b) with e.g. StructuralTypeShaperExpression. This way, when RelationalSqlTranslatingExpressionVisitor runs, it encounters the shaper rather than the ParameterExpression.

This extra path is bad for performance: the tree is almost always rewritten (it's not just a read-only pass), and multiple lambdas can be nested, leading to the same fragment being rewritten again and again (e.g. blogs.Where(b => b.Posts.Where(p => p.Comments.Where(c => ...)))).

In addition, this means that RelationalSqlTranslatingExpressionVisitor lacks context during visitation, and cannot know that it's visiting what used to be a lambda parameter. This is blocking work on #32980.

Note the similarity with #31309, which tracks removing another pre-visitation pass for handling owned navigations inside the lambda body.

[tuespetre]

this issue has piqued my curiosity.

i have called this "expanding parameters" in my work, and i feel it is one of the foundational techniques in transforming expression trees, a very LISP-like time-tested concept. if you don't use an expansion/remapping pass at all, won't that mean your other visitors will have to check in nearly every place for a parameter and then perform a lookup for some data corresponding to that parameter anyway?

it will be exciting to see how the query pipeline evolves from here!

[roji]

if you don't use an expansion/remapping pass at all, won't that mean your other visitors will have to check in nearly every place for a parameter and then perform a lookup for some data corresponding to that parameter anyway?

Yep - though the crucial point is that there's (in principle) only one step where parameters need to be expanded - the translation step. In other words, the original expression goes through several preprocessing visitors which do some sort of transformation on the tree, but which don't need parameters to be expanded. Then, we reach the "translation" point, where LINQ expression which represent .NET code (method calls, member accesses) are translated to extension expression types which represent a SQL tree (SelectExpression, subqueries...). Once translation has occurred, there are no longer parameters to be expanded; there are SQL parameters (e.g. @p), but there are placeholders where different data can be plugged in as the same query is re-executed (so unrelated to this).

In other words, parameter expansion (in principle) only needs to occur in translation. It's true that translation does involve multiple visitors - all of which would need to be able to expand parameters - so the expansion logic will need to be extracted out, to be reusable across the different visitors.

Note that I'm still exploring this direction - does the above make sense?

[tuespetre]

i think so -- that sounds pretty much like what i have been doing. say i'm translating a query that has a Where call in the tree. when i get to the Where call, i visit the source argument. and if the visited source has become a RelationalQueryExpression, i replace the predicate lambda's parameter in the lambda's body, reduce member accesses, apply translation visitors (SQL parameterization, function calls, etc.) and check to see if the lambda body has now become an expression that can be represented as SQL. if so, i can produce a new RelationalQueryExpression to replace the call to Where. i assumed this is roughly what you had been doing too since moving on from Relinq

[roji]

Yep, pretty much. The only thing is that the expansion of the parameter doesn't have to occur as a separate, pre-translation visitation pass, which rewrites the entire tree just before it's translated (see OP above) - it can simply be done by the visitor doing the translation of the lambda. So instead of first visiting the lambda to find parameters and replace them, and then run a separate visitor to translate to SQL, the visitor that translates to SQL can just do the replacing when it sees a parameter.

[tuespetre]

again, curious to see how it plays out. i would think you might end up with a sort of "god visitor" handling all BCL method translation, which could be tedious to develop unless you end up creating a sort of DSL for it. but then you have things like anonymous type projections, and... well, keep me updated 😅 i will follow this issue out of curiosity

[roji]

would think you might end up with a sort of "god visitor" handling all BCL method translation

I don't think it's going that way - BCL method translation is already split across multiple visitors (RelationalQueryableMethodTranslatingExpressionVisitor, RelationalSqlTranslatingExpressionVisitor, RelationalProjectionBindingExpressionVisitor), with currently an extra visitor that does the parameter replacing and the member access reduce (that's ReplacingExpressionVisitor). The proposal is simply to have the different visitors be capable of visiting/replacing ParameterExpression directly instead of relying on ReplacingExpressionVisitor to do that for them in a separate, earlier pass.

This does mean that parameter replace would now be done in multiple places (each visitor) instead of just one (ReplacingExpressionVisitor), but that doesn't seem problematic... There are also some tricky details around member access reduction and especially NewExpression - I do suspect we'll end up still needing a separate MemberAccessReducer visitor, but one that's only used when needed, specifically to reduce member accesses.