Query: Full support for entity splitting by smitpatel · Pull Request #29062 · dotnet/efcore

smitpatel · 2022-09-12T21:02:06Z

Resolves #620

smitpatel · 2022-09-15T00:17:51Z

src/EFCore.Relational/Metadata/Internal/RelationalEntityTypeExtensions.cs

            }

-            var propertyMappings = table.FindColumn(property)!.PropertyMappings;
+            var column = table.FindColumn(property);


wrt #29079
This currently considers only properties stored in main fragment.

roji

LGTM, see minor nits/suggestions - nothing that can't be handled later.

roji · 2022-09-15T09:36:10Z

test/EFCore.SqlServer.FunctionalTests/Query/EntitySplittingQuerySqlServerTest.cs

+        AssertSql(
+            @"SELECT [e].[Id], [s0].[IntValue3], [s0].[StringValue3]
+FROM [EntityOne] AS [e]
+INNER JOIN [SplitEntityOnePart2] AS [s0] ON [e].[Id] = [s0].[Id]");


Could we improve this in the future by only querying SplitEntityOnePart2, given that all the information is there? If so, should we file an issue?

We can potentially improve. Though need to keep in mind optional dependents and optional fragments if we do that in future. #29035 could also alleviate this in a different way (like how we don't have any extra table for TPC selected)

Filed #29113
Added notes there.

roji · 2022-09-15T09:41:17Z

test/EFCore.Relational.Specification.Tests/TestModels/EntitySplitting/SplitEntityOne.cs

+    [NotMapped]
+    public OwnedReference OwnedReference { get; set; }
+    [NotMapped]
+    public List<OwnedCollection> OwnedCollection { get; set; } = new List<OwnedCollection>();


nit: target-typed new (here and below)

roji · 2022-09-15T09:59:25Z

test/EFCore.Relational.Specification.Tests/Query/EntitySplittingQueryTestBase.cs

+                mb.Entity<EntityOne>(
+                    b =>
+                    {
+                        b.ToTable("EntityOnes");


Possibly not needed?

roji · 2022-09-15T10:14:38Z

test/EFCore.SqlServer.FunctionalTests/Query/EntitySplittingQuerySqlServerTest.cs

+LEFT JOIN [OwnedReferencePart3] AS [o0] ON [b].[Id] = [o0].[LeafEntityId]");
+    }
+
+    public override async Task Tpt_entity_owning_a_split_reference_on_leaf_with_table_sharing(bool async)


Possibly add test coverage querying an unrelated sibling, to make sure the owned fields/tables aren't queries (for the multiple scenarios here, TPH/TPT/TPC)

roji · 2022-09-15T10:16:25Z

test/EFCore.SqlServer.FunctionalTests/Query/EntitySplittingQuerySqlServerTest.cs

+    SELECT [l0].[Id], [l0].[BaseValue], [l0].[MiddleValue], [l0].[LeafValue], N'LeafEntity' AS [Discriminator]
+    FROM [LeafEntity] AS [l0]
+) AS [t]
+LEFT JOIN [LeafEntity] AS [l] ON [t].[Id] = [l].[Id]


Is this 2nd join on LeafEntity needed?

Yes, we are selecting owned entity columns from this table which is also inside the TPC, so either we generate join or we inject inside TPC null columns for every other type and non-null for the leaf type owning the entity.

roji · 2022-09-15T10:18:32Z

test/EFCore.SqlServer.FunctionalTests/Query/EntitySplittingQuerySqlServerTest.cs

+LEFT JOIN [OwnedReferencePart3] AS [o0] ON [l].[Id] = [o0].[LeafEntityId]");
+    }
+
+    public override async Task Tpc_entity_owning_a_split_reference_on_leaf_with_table_sharing(bool async)


Is it intentional that we don't have the corresponding tests on middle/base like TPH/TPT?

In TPC, an entity type can only have owned entity with table sharing for leaf entities.

Ah OK, was not aware. Can still add those tests and assert failure for the day when we do implement TPC+owned on non-leaf (do we have any issue?). But obviously not very important.

There are model validation tests for it. I think if we remove that exception @AndriySvyryd would remind us to add query support.

I doubt we'll ever support it. Table splitting on non-leaves in TPC would require the columns to exist on all derived tables as well, which doesn't sound that useful considering the complexity of implementation.

roji · 2022-09-15T10:40:44Z

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

-                EntityProjectionExpression entityProjectionExpression,
-                ITableBase targetTable,
-                INavigation navigation)
+            if (navigation.IsCollection)


Just to make sure - this is relevant even when targetEntityType.IsMappedToJson()? (because it's not in an else clause)

JSON shouldn't reach here. I think the innerShaper being not null condition above is not needed since for JSON we always add shaper in advance. I will update code there to make it clearer.

roji · 2022-09-15T11:06:43Z

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

            }
        }
-
+        else


nit: can remove an unindent the following (I'd add a comment either way since this function is huge and it's easy to lose track of which scenario we're in)

variable name clash is I think the reason it remained this way in code cleanup.

roji · 2022-09-15T11:35:09Z

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

+            // Either we encountered a custom table source or dependent is not sharing table
+            // In either case we need to generate join to owner
+            var ownerJoinColumns = new List<ColumnExpression>();
+            var owenrTableReferenceExpression = selectExpression._tableReferences[baseTableIndex];


Suggested change

var owenrTableReferenceExpression = selectExpression._tableReferences[baseTableIndex];

var ownerTableReferenceExpression = selectExpression._tableReferences[baseTableIndex];

roji · 2022-09-15T11:35:40Z

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

            }

-            if (unwrappedTable is SelectExpression subquery)
+            // Either we encountered a custom table source or dependent is not sharing table


Not sure, but seems like there may be some DRY potential between the code below and above, can improve later.

I considered it but it was causing a lot of complexity in readability. I think at this point there are 2 major things which are kind of duplicated - adding join to other split tables and populating properties.
Join has difference in terms of finding existing table and populating properties eventually would be different from column sharing perspective in what nullability they get.
Plus adding join also populate some variables to be used later so if we move that to a method for DRY, we need to pass states around.
Feel free to change code if you can refactor it.

Filed #29115

Yeah, I saw it wasn't a straightforward DRY. If we can make it better somehow, great, but definitely not a good idea to make it significantly more complex with state just to DRY.

Resolves #620

smitpatel force-pushed the smit/entitysplitting branch from bb72359 to bf10425 Compare September 15, 2022 00:16

smitpatel marked this pull request as ready for review September 15, 2022 00:17

smitpatel requested a review from a team September 15, 2022 00:17

smitpatel commented Sep 15, 2022

View reviewed changes

roji approved these changes Sep 15, 2022

View reviewed changes

smitpatel force-pushed the smit/entitysplitting branch from bf10425 to b6e5a30 Compare September 15, 2022 19:36

Query: Full support for entity splitting

8f20257

Resolves #620

smitpatel force-pushed the smit/entitysplitting branch from b6e5a30 to 8f20257 Compare September 15, 2022 19:37

smitpatel merged commit d9518a5 into release/7.0 Sep 15, 2022

smitpatel deleted the smit/entitysplitting branch September 15, 2022 22:20

ajcvickers mentioned this pull request Sep 29, 2022

.NET Data Biweekly Updates (2022) #27185

Closed

                           }
                       }
+                      else

	var owenrTableReferenceExpression = selectExpression._tableReferences[baseTableIndex];
	var ownerTableReferenceExpression = selectExpression._tableReferences[baseTableIndex];

Conversation

smitpatel commented Sep 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roji Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

roji Sep 15, 2022 •

edited

Loading