Redo query complex type support for TPC and other hierarchy scenarios #37410
Conversation
roji
force-pushed
the
ComplexTypeTpcRedo
branch
from
4ebbe6d to
3e20b17
Compare
roji
force-pushed
the
ComplexTypeTpcRedo
branch
from
3e20b17 to
084f81f
Compare
roji
changed the title
test/EFCore.SqlServer.FunctionalTests/LazyLoadProxySqlServerTest.cs
Outdated
Show resolved
Hide resolved
...qlServer.FunctionalTests/Query/Inheritance/TPTInheritanceTableSplittingQuerySqlServerTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR enhances support for complex types on TPC-mapped entity types by refactoring the query infrastructure from a lazy to an eager approach for building property maps. Previously, complex type properties were resolved lazily using table maps, which broke down with TPC due to column name uniquification in UNION queries. The new implementation eagerly builds all property maps recursively across complex properties when a SelectExpression is first constructed, similar to how regular properties are handled.
Key Changes:
- Refactored complex type query support to use eager property map construction instead of lazy binding
- Fixed complex type equality comparisons to include all nested properties
- Reorganized test files into inheritance-specific namespaces
- Added comprehensive test coverage for TPC, TPH, and TPT inheritance patterns with complex types
Reviewed changes
Copilot reviewed 195 out of 196 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| SqliteComplianceTest.cs | Added ignored test bases for TPT/TPC inheritance tests pending issue #35025 |
| Multiple Query test files | Moved inheritance-related query tests to Microsoft.EntityFrameworkCore.Query.Inheritance namespace |
| ComplexTypeQuerySqlServerTest.cs | Updated SQL assertions to include nested complex type properties in equality comparisons |
| TPT/TPC InheritanceQueryTests | Updated SQL assertions to include complex type columns in projections |
| New Inheritance test files | Added test classes for TPH/TPT/TPC inheritance with complex types (table splitting and JSON) |
| Flower.cs | Added empty class body to abstract Flower class |
Files not reviewed (1)
- src/EFCore.Relational/Properties/RelationalStrings.Designer.cs: Language not supported
|
@AndriySvyryd see also test failures in RelationalModelTest.Can_use_relational_model_with_tables and similar. |
Also corrects comparison and assignment of complex types. Most completes dotnet#35025 Fixes dotnet#35392 Fixes dotnet#37391 Fixes dotnet#37395
roji
force-pushed
the
ComplexTypeTpcRedo
branch
from
084f81f to
127472e
Compare
AndriySvyryd
force-pushed
the
ComplexTypeTpcRedo
branch
from
1b43b96 to
9fa5fa0
Compare
AndriySvyryd
force-pushed
the
ComplexTypeTpcRedo
branch
from
a0866bf to
1919e98
Compare
AndriySvyryd
force-pushed
the
ComplexTypeTpcRedo
branch
from
4bb3410 to
0968a67
Compare
...FCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.CreateSelect.cs
Show resolved
Hide resolved
...FCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.CreateSelect.cs
Outdated
Show resolved
Hide resolved
...FCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.CreateSelect.cs
Outdated
Show resolved
Hide resolved
...FCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.CreateSelect.cs
Outdated
Show resolved
Hide resolved
| ?? structuralType.GetDefaultMappings().Single().Table) | ||
| .FindColumn(jsonColumnName)!.StoreTypeMapping; | ||
| var jsonColumn = (structuralType.ContainingEntityType.GetViewOrTableMappings().Select(m => m.Table.FindColumn(jsonColumnName)).FirstOrDefault(c => c is not null) | ||
| ?? structuralType.GetDefaultMappings().Single().Table.FindColumn(jsonColumnName)) |
There was a problem hiding this comment.
Here I'm less sure (again the code was this way before, for quite a while I think). I'm still not quite clear on what GetDefaultMappings() is exactly and how it related to everything...
If a query is executed with FromSql() for an entity type which has a JSON column, should we still simply expected to get the table from GetViewOrTableMappings(), i.e. the table the entity type is "normally" mapped to? With the expectation that whatever comes out of the user's SQL query should exactly match would would come out of the that table?
BTW given that we call GetViewOrTableMappings(), does that mean we don't support JSON columns on types mapped to TVFs?
There was a problem hiding this comment.
(FYI I removed the GetDefaultMappings() for now)
There was a problem hiding this comment.
If a query is executed with FromSql() for an entity type which has a JSON column, should we still simply expected to get the table from GetViewOrTableMappings(), i.e. the table the entity type is "normally" mapped to?
No, GetViewOrTableMappings() should never be called while processing a FromSql() query, only GetDefaultMappings() should be used. I know there are many incorrect usages currently, so we should try to clean them up when possible. But I am also aware that some of them are workarounds for some scenarios that wouldn't be supported until #21627 is implemented.
There was a problem hiding this comment.
OK. Then there are definitely incorrect usages - though generally I don't think we've seen many failed scenarios around FromSql. Let's keep an eye on it...
src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.StructuralEquality.cs
Outdated
Show resolved
Hide resolved
AndriySvyryd
left a comment
AndriySvyryd
left a comment
There was a problem hiding this comment.
@roji I finished reviewing your changes, PTAL at my commits
Fix JSON container column in hierarchy
AndriySvyryd
force-pushed
the
ComplexTypeTpcRedo
branch
from
0968a67 to
771c560
Compare
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
@AndriySvyryd thanks, your changes LGTM too. Take a look at the remaining unresolved conversations, but I think we're mostly done here.
2 participants
Also corrects comparison and assignment of complex types.
Mostly completes #35025
Fixes #35392
Fixes #37391
Fixes #37395
Contains changes from #35314
When I originally wrote the query support for complex types, I implemented them via a lazy approach. Regular properties all get created when the entity type's SelectExpression is originally created, and StructuralTypeProjectionExpression receives a fully baked property->column map which is used when binding a property, applying its projection, etc. In contrast, with complex types instead passed in a table map, mapping relational models to their table alias in the current query; then, binding complex properties was implemented in lazy fashion, finding the relational table, looking it up in the table map, and constructing a complex type shaper based on that.
This approach unfortunately breaks down with TPC: because multiple tables in the hierarchy might have columns with the same name, we uniquify projected column names, so that the TPC UNION table projects out
fooandfoo0from two subqueries which project outfoo. This means that when lazily binding a complex property, the table map is no longer enough to accurately do things: we also need a property->column to tell us exactly which column corresponds to which property in the TPC UNION projection. The lazy approach was also generally brittle in other ways.This PR changes query complex type support to work like regular properties - when a SelectExpression is first constructed, all property maps are built recursively eagerly, across complex properties.