Work on set operations and inheritance #18833

roji · 2019-11-10T22:40:05Z

Weekend/Jetlag Project

Added support for set operations over different entity types.

When performing a set operation, if two properties in the entity inheritance hierarchy were mapped to the same database column, that column was projected twice.

Closes #16298
Fixes #18832

smitpatel · 2019-11-11T01:33:12Z

test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs

@@ -487,7 +514,7 @@ public virtual void Subquery_OfType()
                .ToList();
        }

-        [ConditionalFact(Skip = "#16298")]
+        [ConditionalFact]


Convert all the tests to ConditionalTheory.

Should we convert the entire test class to use QueryAsserter etc. before doing this (otherwise should I just duplicate the test code for sync/async)? Note that for now there's no ConditionalTheory in this class, so maybe we can do it in another pass afterwards?

/cc @maumar

We can defer is nothing is there. File an issue.

test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs

smitpatel · 2019-11-11T01:35:18Z

I will review it after planning as I want to think more about design overall.

roji · 2019-11-11T03:05:50Z

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

                }

-                return outerProjection;
+                _projectionMapping[projectionMember] = new EntityProjectionExpression(commonParentEntityType, propertyExpressions);


LiftEntityProjectionFromSubquery, which is called from PushdownIntoSubquery, also lifts navigations. We probably need to do the same here.

We need to do the same recursively else set operation over 2 different aggregates will fail.

Added this but you should definitely verify that it's correct (don't understand everything here).

Also, here's the test I added for this, it fails in NavigationExpandingExpressionVisitor (on this line, so seems like that visitor would also need to be fixed. Makes sense?

[ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Union_over_owned_types_with_different_owners(bool async) { return AssertQuery( async, ss => ss .Set<LeafA>().Select(l => l.LeafAAddress) .Union(ss.Set<LeafB>().Select(l => l.LeafBAddress)), entryCount: 5); }

navigation expansion also needs to be update to allow set operations over different entityTypes.

@smitpatel I've thought about this a bit, and I want to be sure I understand what we want to do here...

In the owned entity scenario above, LeafAAddress and LeafBAddress have the same CLR type (OwnedAddress) but are different entity types (as they're owned by different entity types), not within the same type hierarchy. Each could also define its own shadow properties, in which case things get especially complicated: we can still project nulls for missing properties on each side (just like with the inheritance scenario), but we don't have a common ancestor to be returned by the EntityProjectionExpression (and shaper). We could implement #16215 (and also find a solution for EntityProjectionExpression to project more than one type?), but that seems like a lot of complexity and work.

We could also support set operations over types only if they happen to have exactly the same properties - or even if one type's property list is a subset of the other's. That seems like it would be relatively easy - the the EntityProjectionExpression and shaper would return the entity type with the property superset.

Note that if do any of the above, this has nothing really to do with owned entities: it would amount to supporting set operations over arbitrary/disconnected types which happen to have the same properties. For example, one could do a union over A and B although they're not in the same hierarchy and not owned or anything (the CLR types just need to implement the same interface). This is not necessarily a bad thing, just more than the original inheritance work I originally thought we'd do here.

What do you think?

Actually, there's an additional difficulty... In the case of inheritance, the shaper knows which entity type to materialize because there's a discriminator; but in the non-inheritance scenario above it wouldn't have anything to go on.

So unless I'm mistaken, the only thing we can do, is to allow set operations over different types that are either in the same hierarchy, or which share the same CLR type and have the exact same list of properties. Does that make sense?

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

smitpatel · 2019-11-19T17:37:42Z

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

+        /// </summary>
+        private void ModifyShaperForSetOperation(ShapedQueryExpression source1, ShapedQueryExpression source2)
+        {
+            if (RemoveConvert(source1.ShaperExpression) is EntityShaperExpression shaper1


If you are using remove convert and not putting the type backs in then it will cause exception for async single element.
Regression test for this would be casting entities to base type but closest parent is a derived type of that base type.

Made the change to reintroduce the Convert node if the common ancestor is different from the original convert type (although if there are multiple Convert nodes overlaid on each other they're discarded - hope that's OK).

However, I couldn't get a test to fail before this change. Tried as you suggested but the following passed:

[ConditionalFact] public virtual async Task Union_casting_to_base_with_lower_common_ancestor() { using var context = CreateContext(); var birds = await context.Set<Kiwi>() .Cast<Animal>() .Union(context.Set<Eagle>() .Cast<Animal>()) .FirstAsync(); }

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

smitpatel · 2019-11-19T17:42:28Z

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

+                // Finally, the shaper will expect to read properties from unrelated siblings, since the set operations
+                // return type is the common ancestor. Add appropriate null constant projections for both sides.
+                // See #16215 for a possible optimization.
+                var unrelatedSiblingProperties = GetAllPropertiesInHierarchy(commonParentEntityType)


You already computed all these above.

You mean the properties of each side? Yeah, but those lists are later mutated to remove the handled properties...

I'll make a copy of these lists to avoid calling GetAllPropertiesInHierarchy as part of Except etc.

smitpatel · 2019-11-19T17:43:09Z

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

                }

-                return outerProjection;
+                _projectionMapping[projectionMember] = new EntityProjectionExpression(commonParentEntityType, propertyExpressions);


We need to do the same recursively else set operation over 2 different aggregates will fail.

smitpatel · 2019-11-19T17:44:55Z

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

+
+                ColumnExpression AddSetOperationColumnProjections(string columnName, SqlExpression innerExpression1, SqlExpression innerExpression2)
+                {
+                    if (expressionsByColumnName.TryGetValue(columnName, out var outerProjection))


You shouldn't need this. You are already combining projections mapped to same column. If something calls in this with same column name, which could be the case if unrelated sibling type is also using same column name, you still need null for it rather than returning existing projection.

The problem is with multiple properties on the same side which map to the same column (e.g. Coke.SugarGrams and Lilt.SugarGrams) - unless we do this deduplication we fetch the same column twice in the projection.

Note: this isn't just a perf issue - it can lead to wrong results in some cases. For example, in Except_parent_with_child, SugarGrams is loaded twice on the left side - once for Coke.SugarGrams as SugarGrams, once for Lilt.SugarGrams as SugarGrams0. This means that the row for the Coke has a value for SugarGrams0 on the left side, but NULL on the right side, and the EXCEPT operation doesn't identify them as identical.

You are already combining projections mapped to same column.

You already de-duped them so why again. Unless the first logic is incorrect. Essentially, you are trying to find IProperty which maps to same column twice.

smitpatel · 2019-11-19T17:51:15Z

test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs

+        [ConditionalFact]
+        public virtual void Union_of_supertype_with_itself_with_properties_mapped_to_same_column()
+        {
+            // Both Tea and Coffee define properties called CaffeineGrams mapped to the same database column.


Can you do union on Tea & Coffee rather than Drink?

Coffee? Which store do you think we're in?

Concat_siblings_with_two_properties_mapped_to_same_column does Tea & Coke, the idea in this test was to go through the supertype.

Added support for set operations over different entity types. When performing a set operation, if two properties in the entity inheritance hierarchy were mapped to the same database column, that column was projected twice. Closes #16298 Fixes #18832

roji · 2019-11-27T16:31:38Z

@smitpatel rebased and pushed changes.

roji · 2019-11-27T17:35:55Z

Getting some InMemory failures, will look at those soon (but we can still discuss the above etc.).

test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs

smitpatel · 2019-11-28T23:05:03Z

⏱ for nav expansion update before I review.

roji · 2020-06-09T11:28:57Z

Closing for now.

roji requested a review from smitpatel November 10, 2019 22:40

smitpatel self-assigned this Nov 11, 2019

smitpatel reviewed Nov 11, 2019

View reviewed changes

test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs Outdated Show resolved Hide resolved

roji commented Nov 11, 2019

View reviewed changes

smitpatel suggested changes Nov 19, 2019

View reviewed changes

roji added 2 commits November 26, 2019 20:17

Address review comments

c1513db

roji force-pushed the SetOperationsDifferentEntityTypes branch from 28812bb to c1513db Compare November 27, 2019 16:31

roji requested a review from smitpatel November 27, 2019 16:31

roji commented Nov 28, 2019

View reviewed changes

test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs Show resolved Hide resolved

smitpatel removed their assignment Dec 10, 2019

roji closed this Jun 9, 2020

smitpatel deleted the SetOperationsDifferentEntityTypes branch June 9, 2020 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Work on set operations and inheritance #18833

Work on set operations and inheritance #18833

roji commented Nov 10, 2019

smitpatel Nov 11, 2019

roji Nov 26, 2019

smitpatel Nov 28, 2019

smitpatel commented Nov 11, 2019

roji Nov 11, 2019

smitpatel Nov 19, 2019

roji Nov 27, 2019

smitpatel Nov 28, 2019

roji Dec 19, 2019

roji Dec 19, 2019

smitpatel Nov 19, 2019

roji Nov 27, 2019 •

edited

Loading

smitpatel Nov 19, 2019

roji Nov 26, 2019

smitpatel Nov 19, 2019

smitpatel Nov 19, 2019

roji Nov 27, 2019

smitpatel Nov 28, 2019

smitpatel Nov 19, 2019

roji Nov 27, 2019 •

edited

Loading

roji commented Nov 27, 2019

roji commented Nov 27, 2019

smitpatel commented Nov 28, 2019

roji commented Jun 9, 2020

Work on set operations and inheritance #18833

Work on set operations and inheritance #18833

Conversation

roji commented Nov 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

roji commented Nov 27, 2019

roji commented Nov 27, 2019

smitpatel commented Nov 28, 2019

roji commented Jun 9, 2020

roji Nov 27, 2019 •

edited

Loading

roji Nov 27, 2019 •

edited

Loading