Addressed review comments

Also found some more skipped set operation tests, unskipped and
rearranged them.

Fixes #12568

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

@@ -502,18 +502,8 @@ void HandleEntityMapping(
  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 =
- shaperExpression as EntityShaperExpression ?? (
- shaperExpression is UnaryExpression unary
- && unary.NodeType == ExpressionType.Convert
- && unary.Type == commonParentEntityType.ClrType
- ? unary.Operand as EntityShaperExpression : null);
-
- if (entityShaperExpression != null)
- {
- shaperExpression = new EntityShaperExpression(
- commonParentEntityType, entityShaperExpression.ValueBufferExpression, entityShaperExpression.Nullable);
- }
+ // The EntityShaperExpression may be buried under Convert nodes produced by Cast operators, preserve those.
+ shaperExpression = UpdateEntityShaperEntityType(shaperExpression, commonParentEntityType);
  }
  }
 
@@ -567,6 +557,21 @@ void HandleColumnMapping(
  var outerColumn = new ColumnExpression(projectionExpression1, select1, IsNullableProjection(projectionExpression1));
  _projectionMapping[projectionMember] = outerColumn;
  }
+
+ static Expression UpdateEntityShaperEntityType(Expression shaperExpression, IEntityType newEntityType)
+ {
+ switch (shaperExpression)
+ {
+ case EntityShaperExpression entityShaperExpression:
+ return new EntityShaperExpression(newEntityType, entityShaperExpression.ValueBufferExpression, entityShaperExpression.Nullable);
+ case UnaryExpression unary when unary.NodeType == ExpressionType.Convert:
+ return Convert(UpdateEntityShaperEntityType(unary.Operand, newEntityType), unary.Type);
+ case UnaryExpression unary when unary.NodeType == ExpressionType.ConvertChecked:
+ return ConvertChecked(UpdateEntityShaperEntityType(unary.Operand, newEntityType), unary.Type);
+ default:
+ throw new Exception($"Unexpected expression type {shaperExpression.GetType().Name} encountered in {nameof(UpdateEntityShaperEntityType)}");
+ }
+ }
  }
 
  public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
@@ -716,8 +721,7 @@ public RelationalCollectionShaperExpression ApplyCollectionJoin(int collectionId
  {
  if (IsDistinct || Limit != null || Offset != null || IsSetOperation)
  {
- var pushdown = PushdownIntoSubquery();
- outer = new SqlRemappingVisitor(pushdown).Remap(outer);
+ outer = new SqlRemappingVisitor(PushdownIntoSubquery()).Remap(outer);
  }
 
  if (innerSelectExpression.Offset != null
@@ -1332,12 +1336,7 @@ public enum SetOperationType
  /// <summary>
  /// Represents an SQL EXCEPT set operation.
  /// </summary>
- Except = 4,
-
- /// <summary>
- /// Represents a custom, provider-specific set operation.
- /// </summary>
- Other = 9999
+ Except = 4
  }
 }
 

src/EFCore/Properties/CoreStrings.resx

@@ -1186,4 +1186,7 @@
  <data name="UnableToDiscriminate" xml:space="preserve">
  <value>Unable to materialize entity of type '{entityType}'. No discriminators matched '{discriminator}'.</value>
  </data>
-</root>
+ <data name="SetOperationWithDifferentIncludesInOperands" xml:space="preserve">
+ <value>When performing a set operation, both operands must have the same Include operations.</value>
+ </data>
+</root>

src/EFCore/Query/NavigationExpansion/NavigationTreeNode.cs

@@ -132,17 +132,14 @@ public static NavigationTreeNode Create(
  return result;
  }
 
- public List<NavigationTreeNode> Flatten()
+ public IEnumerable<NavigationTreeNode> Flatten()
  {
- var result = new List<NavigationTreeNode>();
- result.Add(this);
+ yield return this;
 
- foreach (var child in Children)
+ foreach (var child in Children.SelectMany(c => c.Flatten()))
  {
- result.AddRange(child.Flatten());
+ yield return child;
  }
-
- return result;
  }
 
  // TODO: just make property settable?

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

@@ -7,6 +7,7 @@
 using System.Linq.Expressions;
 using System.Reflection;
 using System.Xml;
+using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.Internal;
 using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Metadata.Internal;
@@ -873,6 +874,35 @@ private Expression ProcessSetOperation(MethodCallExpression methodCallExpression
  var source2 = VisitSourceExpression(methodCallExpression.Arguments[1]);
  var preProcessResult2 = PreProcessTerminatingOperation(source2);
 
+ // Compare the include chains from each side to make sure they're identical. We don't allow set operations over
+ // operands with different include chains.
+ var current1 = preProcessResult1.state.PendingIncludeChain?.NavigationTreeNode;
+ var current2 = preProcessResult2.state.PendingIncludeChain?.NavigationTreeNode;
+ while (true)
+ {
+ if (current1 == null)
+ {
+ if (current2 == null)
+ {
+ break;
+ }
+ throw new NotSupportedException(CoreStrings.SetOperationWithDifferentIncludesInOperands);
+ }
+
+ if (current2 == null)
+ {
+ throw new NotSupportedException(CoreStrings.SetOperationWithDifferentIncludesInOperands);
+ }
+
+ if (current1.FromMappings.Zip(current2.FromMappings, (m1, m2) => (m1, m2))
+ .Any(t => !t.m1.SequenceEqual(t.m2)))
+ {
+ throw new NotSupportedException(CoreStrings.SetOperationWithDifferentIncludesInOperands);
+ }
+
+ (current1, current2) = (current1.Parent, current2.Parent);
+ }
+
  // If the siblings are different types, one is derived from the other the set operation returns the less derived type.
  // Find that.
  var clrType1 = preProcessResult1.state.CurrentParameter.Type;