dotnet / dotnet/efcore

Consider doing property/navigation binding entirely from RelationalSqlTranslatingExpressionVisitor

Open
#31,309 1 comment 0 reactions 0 assignees View on GitHub
area-query needs-design
Dominant language
C#
Stars
14.8k
Forks
3.4k
PR merge metrics
PR metrics pending

Description

In our current query pipeline design, before RelationalQueryableMethodTranslatingEV sends a lambda body to translation in RelationalSqlTranslatingEV, it pre-visits the lambda (ExpandSharedTypeEntities, invoked from RemapLambdaBody). This pre-visitation pass "expands" property accesses into owned entities, effectively performing the binding at that point. When the lambda body later reaches SQL translation, the property access has already been dealt with.

It would be more natural to pass the lambda body as-is and do all translation inside RelationalSqlTranslatingEV, including binding of owned navigations etc. Aside from being more understandable, this would also reduce an additional visitation into all lambda bodies. I suspect this is done this way since RelationalSqlTranslatingEV currently doesn't know about the SelectExpression being processed; it only receives the lambda body as input.

Regular properties, on the other hand, are bound inside RelationalSqlTranslatingEV as expected. However, there as well, since the SelectExpression isn't available, it's not possible to produce the ColumnExpression at that point. Instead, EntityProjectionMapping contains a dictionary mapping all the entity's properties to ColumnExpressions; this dictionary is created in SelectExpression wherever an EntityProjectionMapping is instantiated, and is used from RelationalSqlTranslatingEV when binding a property.

The fix here would be for ProjectionBindingExpression to have a mapping between relational ITableBase and the TableReferenceExpressions of its SelectExpression. This would allow it to bind properties (and navigations) upon demand - given an IProperty, it would simply find the right ITable for it, get the TableReferenceExpression for that for the current SelectExpression, and construct a ColumnExpression over that.

Doing this would remove the property and owned navigation maps from EntityProjectionExpression, and hopefully simplify a lot of the logic for maintaining these.

/cc @maumar

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.