Set operations: cleanup #16249

roji · 2019-06-25T20:01:15Z

NOTE: This PR was changed to only cleanup some aspects, other parts were split out

Include/nav rewriting is now supported.
We now push down to subquery when OrderBy, Take or Skip are applied to set operations, to avoid using the hack where ColumnExpression with no table alias was used. Set operations: translate OrderBy/Take/Skip directly on set operation instead of pushing down #16244 was opened to track for post-3.0.
Added missing support for union over subselect projection mappings.
Added Other type to SetOperationType so that providers can define extra set operations (e.g. PostgreSQL INTERSECT ALL, EXCEPT ALL).

Completes #6812
Fixes #13196
Fixes #16065
Fixes #16165

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

roji · 2019-06-25T20:03:49Z

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

+                 if (commonParentEntityType != projection1.EntityType)
+                 {
+                     // The first source has been up-cast by the set operation, so we also need to change the shaper expression.
+                     var entityShaperExpression =


@smitpatel I know remove the cast only if it was casting to our common parent type, for which we're setting the new shaper anyway. This code doesn't handle all cases - it's possible there will be two cast nodes, in which case this will not work (but I've seen this pattern in other places in the code). I'm still not exactly sure what we expect to happen, but this may be enough for now. Let me know if you want us to do something else here.

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

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs

maumar · 2019-06-25T22:12:14Z

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs

+            // Find that.
+            var clrType1 = preProcessResult1.state.CurrentParameter.Type;
+            var clrType2 = preProcessResult2.state.CurrentParameter.Type;
+            var parentState = clrType1.IsAssignableFrom(clrType2) ? preProcessResult1.state : preProcessResult2.state;


this way we only propagate include information applied on less derived type. e.g.
customers.Include(c => c.Nav1).Concat(customers.OfType().Include(c => c.Nav2)), we would end up including only Nav1. We either need to merge include information (which is embedded in NavigationTreeNode on the NavigationExpansionState) or detect when the include paths on both sides are different and throw - can probably be done in a future PR as the scenario is somewhat contrived.

you can use IncludeHelpers.CopyIncludeInformation

Yeah, I thought about this a bit. I think that trying to do a set operation with differing includes should throw, since merging includes doesn't correspond to the presumed user expectation, which would be to only include each nav the set operand it's specified. This can't be done in SQL (i.e. unless we fetch the information and don't materialize it).

If users want merged includes, they can simply write the following themselves:

customers.Union(customers.OfType<...>()) .Include(c => c.Nav1) .Include(c => c.Nav2)

We may want to discuss this in design (@divega @ajcvickers). For now I've implemented a NotSupportedException for this (@maumar hope you can take a look to make sure I did it right).

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

smitpatel · 2019-06-26T06:16:56Z

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

            var select2 = otherSelectExpression;

            if (_projection.Any())
            {
-                throw new NotImplementedException("Set operation on SelectExpression with populated _projection");
+                throw new NotSupportedException("Can't process set operations after client evaluation, consider moving the operation before the last Select() call (see issue #16243)");


Which exception type to use? @ajcvickers
We had some recent discussion around this but I don't remember the outcome.

I don't remember that discussion 😄 But I am leaning towards ~~NotImplementedException~~ InvalidOperationException and simplifying the message, but that is because I think #16243 is by design (of course I could be wrong and I may need someone to explain to me why it isn't).

FYI, I created https://go.microsoft.com/fwlink/?linkid=2097910 for exceptions we throw when something would require client evaluation. We tweak what it points to in the future.

@divega I don't have the full picture, but @smitpatel seemed to indicate this should be possible by us moving the union before the client evaluation.

I'm leaning towards NotSupportedException (in the sense of not supported in this version), or to a less extent NotImplementedException (although that's usually used as a very temporary marker for a dev to remember they still have to do something). InvalidOperationException seems used more because an object is in a wrong state, which doesn't seem to be the case here.

However, I'm changing to InvalidOperationException for now we we really want this PR to get merged in time for preview7.

Ah, I see. That is a good idea. I guess it would only about lifting the client eval part and not the actual projection, because lifting the projection over Union() would not be correct in all cases. I personally would love to do something when we can, but I suspect it is going to take some time for this to get to the top of the list.

Re the exception type, I actually don't feel strongly about which one would be the most correct to throw, but what I heard from @ajcvickers and @smitpatel is that keeping the set of exception types customers should catch beats being absolutely correct.

smitpatel · 2019-06-26T06:19:34Z

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

+                     {
+                         shaperExpression = new EntityShaperExpression(
+                             commonParentEntityType, entityShaperExpression.ValueBufferExpression, entityShaperExpression.Nullable);
+                     }


What is else result? Just ignore?

OK, I went with a different approach - I now replace the EntityShaperExpression, and if it's wrapped by Convert nodes I recreate those exactly as they previously were. It's effectively as if the entity was originally queried as a less derived type as what it is.

Sorry this part is taking so many iterations, please let me know if it's still not the way you want - I still don't feel like I understand all the possibilities here 100%.

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

roji · 2019-06-26T14:09:07Z

Pushed a new commit with various fixes.

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs

smitpatel · 2019-06-26T18:08:02Z

If navigation part of set operation is going to take time then can we split out, push down for order by in separate PR and get it merged sooner?

smitpatel · 2019-06-26T18:21:23Z

Also set operations with fromsql.

roji · 2019-06-26T18:48:42Z

If navigation part of set operation is going to take time then can we split out, push down for order by in separate PR and get it merged sooner?

I'm going to take a quick stab at it - I have to leave very soon anyway.

In any case I will look at it later tonight. @smitpatel, maybe comment on what you consider OK (e.g. there's still the convert nodes when replacing the EntityShaperExpression). If necessary I can split out the problematic parts.

Finally, even if the PR isn't perfect as-is, we can also decide to merge and I'll fix anything right afterwards (i.e. tomorrow during my day). Up to you.

smitpatel · 2019-06-26T18:55:29Z

private ColumnExpression(string name, TableExpressionBase table, Type type, RelationalTypeMapping typeMapping, bool nullable)
            : base(type, typeMapping)
        {
// Add this line
            Check.NotEmpty(Table.Alias);
            Name = name;
            Table = table;
            Nullable = nullable;
        }

Fix whatever fails with that, (disable/pushdown). (Apparently this PR does not update code about null check). Split out above in a new PR.

roji · 2019-06-27T16:57:18Z

@smitpatel, removed support entirely for set operations over operands of different entity types - scoped this out to #16298 (this is what the ColumnExpression was for). We can look at this later, hopefully (but not 100% necessarily for 3.0).

@maumar implemented the better way to compare includes on both sides, you can take a look.

Let's try to get it into preview7.

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

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs

maumar · 2019-06-27T17:41:41Z

@roji nav rewrite changes look good!

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs

roji · 2019-06-28T11:27:35Z

Addressed @maumar's comments and rebased on latest master.

smitpatel · 2019-06-28T15:39:55Z

Can you split out PR in 2 parts?

Fixing issue with Table without alias
Nav expansion part of set operations

Since @maumar is gone for vacation, it may take some time to understand nav expansion changes.

roji · 2019-06-28T16:04:15Z

Done, this PR now only fixes the ColumnExpression alias issue. Once it's merged I'll submit a separate one for nav/include support.

* We no longer allow ColumnExpression without table alias * Set operations over operands with different types have been disabled for now. * We now push down set operations into a subquery on Orderby, Skip or Take, which shouldn't be necessary (#16244). * Some test consolidation and cleanup. Continues #6812

roji commented Jun 25, 2019

View reviewed changes

src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs Show resolved Hide resolved

roji commented Jun 25, 2019

View reviewed changes

src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs Outdated Show resolved Hide resolved

roji commented Jun 25, 2019

View reviewed changes

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs Outdated Show resolved Hide resolved

This was referenced Jun 25, 2019

Include() does not work if used after Union() #16065

Closed

Include before Concat throws on nav rewrite #13196

Closed

maumar reviewed Jun 25, 2019

View reviewed changes

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

smitpatel reviewed Jun 26, 2019

View reviewed changes

roji mentioned this pull request Jun 26, 2019

Query: compilation error for query with Except projecting a constant #12568

Closed

maumar reviewed Jun 26, 2019

View reviewed changes

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs Outdated Show resolved Hide resolved

roji mentioned this pull request Jun 27, 2019

Compare to all Queryable/Enumerable methods via MethodInfo, not by name #16300

Closed

1 task

roji force-pushed the StateOfTheUnion2 branch 2 times, most recently from 6de3436 to 8b0fce1 Compare June 27, 2019 16:57

AndriySvyryd reviewed Jun 27, 2019

View reviewed changes

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

maumar reviewed Jun 27, 2019

View reviewed changes

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs Outdated Show resolved Hide resolved

maumar reviewed Jun 27, 2019

View reviewed changes

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs Outdated Show resolved Hide resolved

maumar reviewed Jun 27, 2019

View reviewed changes

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs Outdated Show resolved Hide resolved

maumar reviewed Jun 27, 2019

View reviewed changes

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs Outdated Show resolved Hide resolved

roji force-pushed the StateOfTheUnion2 branch from 79c832a to 1abdfbf Compare June 28, 2019 11:27

roji mentioned this pull request Jun 28, 2019

Queryable methods translation progress #15711

Closed

67 tasks

roji force-pushed the StateOfTheUnion2 branch from 1abdfbf to 8003a14 Compare June 28, 2019 16:02

roji force-pushed the StateOfTheUnion2 branch from 8003a14 to 1285b4f Compare June 28, 2019 16:06

smitpatel approved these changes Jun 28, 2019

View reviewed changes

roji merged commit a2ce460 into master Jun 28, 2019

ghost deleted the StateOfTheUnion2 branch June 28, 2019 16:23

roji changed the title ~~Set operations: finalization~~ Set operations: cleanup Jun 28, 2019

roji mentioned this pull request Jun 28, 2019

Set operations: include/nav support #16339

Merged

ajcvickers mentioned this pull request Jul 5, 2019

Entity Framework Weekly Status Updates (2019) #15403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set operations: cleanup #16249

Set operations: cleanup #16249

roji commented Jun 25, 2019 •

edited

Loading

roji Jun 25, 2019

maumar Jun 25, 2019

maumar Jun 25, 2019

roji Jun 26, 2019

smitpatel Jun 26, 2019

ajcvickers Jun 26, 2019

divega Jun 26, 2019 •

edited

Loading

divega Jun 26, 2019

roji Jun 27, 2019

divega Jun 27, 2019

smitpatel Jun 26, 2019

roji Jun 26, 2019

roji commented Jun 26, 2019

smitpatel commented Jun 26, 2019

smitpatel commented Jun 26, 2019

roji commented Jun 26, 2019

smitpatel commented Jun 26, 2019 •

edited

Loading

roji commented Jun 27, 2019

maumar commented Jun 27, 2019

roji commented Jun 28, 2019

smitpatel commented Jun 28, 2019

roji commented Jun 28, 2019

Set operations: cleanup #16249

Set operations: cleanup #16249

Conversation

roji commented Jun 25, 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

divega Jun 26, 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

roji commented Jun 26, 2019

smitpatel commented Jun 26, 2019

smitpatel commented Jun 26, 2019

roji commented Jun 26, 2019

smitpatel commented Jun 26, 2019 • edited Loading

roji commented Jun 27, 2019

maumar commented Jun 27, 2019

roji commented Jun 28, 2019

smitpatel commented Jun 28, 2019

roji commented Jun 28, 2019

roji commented Jun 25, 2019 •

edited

Loading

divega Jun 26, 2019 •

edited

Loading

smitpatel commented Jun 26, 2019 •

edited

Loading