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

Consider breaking out SelectExpression internal logic to the translation visitors #31049

Open
3 tasks done
Tracked by #34524
roji opened this issue Jun 7, 2023 · 2 comments
Open
3 tasks done
Tracked by #34524

Comments

@roji
Copy link
Member

roji commented Jun 7, 2023

SelectExpression is currently a non-extensible type with private state, and code closed behind APIs such as ApplyDistinct, ClearOrderings etc. which are invoked from e.g. RelationalQueryableMethodTranslatingExpressionVisitor. This makes it impossible to extend the logic in providers.

As an example, during translation we currently create a SQL Server OPENWITH/WITH expression, with an ordering to preserve the JSON array's natural ordering. We then have a post-processing visitor which detects if the ordering is still there, and replaces OPENWITH/WITH with OPENWITH without WITH (since OPENWITH/WITH doesn't support natural ordering). A better design here would be to create the ordered version (without WITH), and when ClearOrderings is called (e.g. because IN/EXISTS/COUNT are applied), transform the OPENWITH at that point.

But since ClearOrderings() is a SelectExpression API, we cannot override it and add this SQL Server-specific logic. If the ClearOrderings() logic were moved to RelationalQueryableMethodTranslatingEV, we could - but that would also require moving out all SelectExpression logic which depends on it (e.g. ApplyDistinct). We'd need to very carefully consider SelectExpression's private state, and whether that remains on SelectExpression (with specific APIs to manipulate it), or whether it moves to the visitor as well.

Note that we have other examples where we visit the query tree in pre-processing - we ideally shouldn't need to do that, but rather perform the function as part of translation.

Work:

@roji
Copy link
Member Author

roji commented Jun 15, 2023

Similar example: in postprocessing, we have SelectExpressionPruningExpressionVisitor which calls SelectExpression.Prune, which removes unreferenced projected columns from SelectExpression. Ideally we'd also remove e.g. the OPENWITH WITH fragments for unused columns. This would mean the logic needs to move out to the visitor, which the SQL Server provider would then override and augment.

Note that the current architecture - where the logic is encapsulated inside SelectExpression rather than inside a real visitor - also leads to pruning not working well, see #31083.

@ajcvickers ajcvickers added this to the Backlog milestone Jun 19, 2023
@roji
Copy link
Member Author

roji commented Jun 26, 2023

Another case: when we add a translated SqlExpression to a SelectExpression (e.g. as a predicate), we call AssignUniqueAliases on it; this runs the AliasUniquifier visitor on it, taking the SelectExpression's _usedAliases into account, and uniquifies any aliases within the SqlExpression.

However, #31078 was filed because we weren't performing that uniquification on SqlExpressions in UPDATE setters. Since these aren't on SelectExpression, AssignUniqueAliases was made pubternal so it could be called from RelationalQueryableMethodTranslatingEV.

If the used aliases were kept as state on RelationalQueryableMethodTranslatingEV (instead of inside SelectExpression), we could also implement this as part of translation rather than doing an additional visitation on each SqlExpression just for alias uniquification.

roji added a commit to roji/efcore that referenced this issue Dec 25, 2023
Instead of on a private list on SelectExpression.
Part of dotnet#31049
roji added a commit to roji/efcore that referenced this issue Dec 25, 2023
Instead of on a private list on SelectExpression.
Part of dotnet#31049
roji added a commit to roji/efcore that referenced this issue Dec 29, 2023
Instead of on a private list on SelectExpression.
Part of dotnet#31049
roji added a commit that referenced this issue Dec 30, 2023
Instead of on a private list on SelectExpression.
Part of #31049
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants