-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Extract SelectExpression construction out to RelationalQueryableMethodTranslatingEV #32877
Conversation
/// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
/// </summary> | ||
[EntityFrameworkInternal] | ||
SelectExpression Select(SqlExpression? projection, SqlAliasManager sqlAliasManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this PR removes Select factory methods from SqlExpressionFactory - at least for now. There's no point in simply duplicating SelectExpression constructors on the factory, just as an additional indirect thing. We may re-introduce them later at some point if that makes sense.
/// <summary> | ||
/// Used to create a root <see cref="SelectExpression" /> representing a query of the given entity type. | ||
/// </summary> | ||
protected virtual SelectExpression CreateSelect(IEntityType entityType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly a copy-paste (plus small reorganize/refactor) of the SelectExpression constructor which accepted an IEntityType. SelectExpression should ultimately be oblivious of our model, representing a pure server-side SQL construct.
* - Principal can be any type in TPH/TPT or leaf type in TPC | ||
* - Dependent side can be TPH or TPT but not TPC | ||
***/ | ||
private void AddConditions(SelectExpression selectExpression, IEntityType entityType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this additional logic which was added on top of the SelectExpression constructor, and which lived in SqlExpressionFactory for some reason. After this PR it is all now concentrated inside QueryableMethodTranslatingEV.
@@ -4412,24 +3837,6 @@ private static Expression MakeNullable(Expression expression, bool nullable) | |||
} | |||
: expression; | |||
|
|||
private static IEnumerable<IProperty> GetAllPropertiesInHierarchy(ITypeBase structuralType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also have those sprinkled around in QueryableMethodTranslatingExpressionVisitors, consider DRYing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will do.
3fbcf31
to
9776d35
Compare
This continues the query architecture work, starting to extract closed logic out of SelectExpression and into RelationalQueryableMethodTranslatingExpressionVisitor, specifically the logic around constructing a root SelectExpression for an entity type (with TPC/TPT/TPH/entity splitting logic/etc.). After this, only regular/direct constructors are left on SelectExpression, which accept its various clauses directly ((nothing that knows about IEntityType; these are also organized/simplified).
This is mostly easy moving around of code, no actual meaningful changes.