diff --git a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs index 513e97990e3..64c5fcd3b60 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs +++ b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs @@ -1551,6 +1551,12 @@ public static string SelectExpressionNonTphWithCustomTable(object? entityType) GetString("SelectExpressionNonTphWithCustomTable", nameof(entityType)), entityType); + /// + /// SelectExpression.Update() is not supported while the expression is in mutable state. + /// + public static string SelectExpressionUpdateNotSupportedWhileMutable + => GetString("SelectExpressionUpdateNotSupportedWhileMutable"); + /// /// Set operations over different entity or complex types are not supported ('{type1}' and '{type2}'). /// diff --git a/src/EFCore.Relational/Properties/RelationalStrings.resx b/src/EFCore.Relational/Properties/RelationalStrings.resx index 4f0e31ca873..e791520a692 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.resx +++ b/src/EFCore.Relational/Properties/RelationalStrings.resx @@ -1017,6 +1017,9 @@ Set operations over different entity or complex types are not supported ('{type1}' and '{type2}'). + + SelectExpression.Update() is not supported while the expression is in mutable state. + Unable to translate set operation after client projection has been applied. Consider moving the set operation before the last 'Select' call. diff --git a/src/EFCore.Relational/Query/Internal/RelationalValueConverterCompensatingExpressionVisitor.cs b/src/EFCore.Relational/Query/Internal/RelationalValueConverterCompensatingExpressionVisitor.cs index 41ead38049d..d4d0ff8ee33 100644 --- a/src/EFCore.Relational/Query/Internal/RelationalValueConverterCompensatingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Internal/RelationalValueConverterCompensatingExpressionVisitor.cs @@ -77,55 +77,15 @@ private Expression VisitCase(CaseExpression caseExpression) private Expression VisitSelect(SelectExpression selectExpression) { - var changed = false; - var projections = new List(); - foreach (var item in selectExpression.Projection) - { - var updatedProjection = (ProjectionExpression)Visit(item); - projections.Add(updatedProjection); - changed |= updatedProjection != item; - } - - var tables = new List(); - foreach (var table in selectExpression.Tables) - { - var newTable = (TableExpressionBase)Visit(table); - changed |= newTable != table; - tables.Add(newTable); - } - + var projections = this.VisitAndConvert(selectExpression.Projection); + var tables = this.VisitAndConvert(selectExpression.Tables); var predicate = TryCompensateForBoolWithValueConverter((SqlExpression?)Visit(selectExpression.Predicate)); - changed |= predicate != selectExpression.Predicate; - - var groupBy = new List(); - foreach (var groupingKey in selectExpression.GroupBy) - { - var newGroupingKey = (SqlExpression)Visit(groupingKey); - changed |= newGroupingKey != groupingKey; - groupBy.Add(newGroupingKey); - } - + var groupBy = this.VisitAndConvert(selectExpression.GroupBy); var having = TryCompensateForBoolWithValueConverter((SqlExpression?)Visit(selectExpression.Having)); - changed |= having != selectExpression.Having; - - var orderings = new List(); - foreach (var ordering in selectExpression.Orderings) - { - var orderingExpression = (SqlExpression)Visit(ordering.Expression); - changed |= orderingExpression != ordering.Expression; - orderings.Add(ordering.Update(orderingExpression)); - } - + var orderings = this.VisitAndConvert(selectExpression.Orderings); var offset = (SqlExpression?)Visit(selectExpression.Offset); - changed |= offset != selectExpression.Offset; - var limit = (SqlExpression?)Visit(selectExpression.Limit); - changed |= limit != selectExpression.Limit; - - return changed - ? selectExpression.Update( - projections, tables, predicate, groupBy, having, orderings, limit, offset) - : selectExpression; + return selectExpression.Update(projections, tables, predicate, groupBy, having, orderings, limit, offset); } private Expression VisitInnerJoin(InnerJoinExpression innerJoinExpression) diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs index 3349ac8dba9..3ebcb7d2ac6 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs @@ -527,7 +527,7 @@ public TpcTableExpressionRemovingExpressionVisitor(SqlAliasManager sqlAliasManag CreateColumnExpression(projection, setOperationAlias), projection.Alias)); } - generatedSelectExpression._mutable = false; + generatedSelectExpression.IsMutable = false; result = generatedSelectExpression; } diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index 520b7059687..431da91d877 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -40,7 +40,7 @@ public sealed partial class SelectExpression : TableExpressionBase private readonly SqlAliasManager _sqlAliasManager; - private bool _mutable = true; + internal bool IsMutable { get; private set; } = true; private Dictionary _projectionMapping = new(); private List _clientProjections = []; private readonly List _aliasForClientProjections = []; @@ -332,7 +332,7 @@ _projectionMapping[new ProjectionMember()] = discriminatorColumnName)); discriminatorValues.Add(concreteEntityType.ShortName()); subSelectExpressions.Add(selectExpression); - selectExpression._mutable = false; + selectExpression.IsMutable = false; } var tableAlias = _sqlAliasManager.GenerateTableAlias("table"); @@ -909,12 +909,12 @@ void ProcessComplexType(StructuralTypeProjectionExpression complexTypeProjection /// public void ApplyProjection() { - if (!_mutable) + if (!IsMutable) { throw new InvalidOperationException("Applying projection on already finalized select expression"); } - _mutable = false; + IsMutable = false; if (_clientProjections.Count > 0) { for (var i = 0; i < _clientProjections.Count; i++) @@ -997,12 +997,12 @@ public Expression ApplyProjection( ResultCardinality resultCardinality, QuerySplittingBehavior querySplittingBehavior) { - if (!_mutable) + if (!IsMutable) { throw new InvalidOperationException("Applying projection on already finalized select expression"); } - _mutable = false; + IsMutable = false; if (shaperExpression is RelationalGroupByShaperExpression relationalGroupByShaperExpression) { // This is final GroupBy operation @@ -1241,7 +1241,7 @@ Expression AddGroupByKeySelectorToProjection( // Needs to happen after converting final GroupBy so we clone correct form. baseSelectExpression = (SelectExpression)cloningExpressionVisitor!.Visit(this); // We mark this as mutable because the split query will combine into this and take it over. - baseSelectExpression._mutable = true; + baseSelectExpression.IsMutable = true; if (resultCardinality is ResultCardinality.Single or ResultCardinality.SingleOrDefault) { // Update limit since split queries don't need limit 2 @@ -1274,7 +1274,7 @@ static void UpdateLimit(SelectExpression selectExpression) if (cloningExpressionVisitor != null) { baseSelectExpression = (SelectExpression)cloningExpressionVisitor.Visit(this); - baseSelectExpression._mutable = true; + baseSelectExpression.IsMutable = true; baseSelectExpression._projection.Clear(); } } @@ -2612,8 +2612,8 @@ private void ApplySetOperation( select2._projectionMapping.Clear(); // Mark both inner subqueries as immutable - select1._mutable = false; - select2._mutable = false; + select1.IsMutable = false; + select2.IsMutable = false; // We should apply _identifiers only when it is distinct and actual select expression had identifiers. if (distinct @@ -2799,7 +2799,7 @@ public void ApplyDefaultIfEmpty(ISqlExpressionFactory sqlExpressionFactory) var alias = _sqlAliasManager.GenerateTableAlias("empty"); var dummySelectExpression = new SelectExpression(alias, _sqlAliasManager); dummySelectExpression._projection.Add(new ProjectionExpression(nullSqlExpression, "empty")); - dummySelectExpression._mutable = false; + dummySelectExpression.IsMutable = false; if (Orderings.Any() || Limit != null @@ -3931,7 +3931,7 @@ private SqlRemappingVisitor PushdownIntoSubqueryInternal(bool liftOrderings = tr Having = Having, Offset = Offset, Limit = Limit, - _mutable = false + IsMutable = false }; _tables.Clear(); _groupBy.Clear(); @@ -4315,7 +4315,7 @@ public override TableExpressionBase Clone(string? alias, ExpressionVisitor cloni IsDistinct = IsDistinct, Tags = Tags, _projectionMapping = newProjectionMappings, - _mutable = _mutable + IsMutable = IsMutable }; // The below contain ColumnExpressions which need to be recreated if a table's alias is modified during cloning @@ -4344,127 +4344,10 @@ public override TableExpressionBase Clone(string? alias, ExpressionVisitor cloni /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// + // TODO: Look into TPC handling and possibly clean this up, #32873 [EntityFrameworkInternal] - public SelectExpression PruneToplevel(SqlTreePruner pruningVisitor) - { - // TODO: This doesn't belong in pruning, take a deeper look at how we manage TPC etc. - var select = (SelectExpression)new TpcTableExpressionRemovingExpressionVisitor(_sqlAliasManager).Visit(this); - select.Prune(pruningVisitor, pruneProjection: false); - return select; - } - - /// - /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to - /// the same compatibility standards as public APIs. It may be changed or removed without notice in - /// any release. You should only use it directly in your code with extreme caution and knowing that - /// doing so can result in application failures when updating to a new Entity Framework Core release. - /// - [EntityFrameworkInternal] - public void Prune(SqlTreePruner pruningVisitor, bool pruneProjection) - { - Check.DebugAssert(!IsMutable, "Mutable SelectExpression found when pruning"); - - var referencedColumnMap = pruningVisitor.ReferencedColumnMap; - // Prune the projection; any projected alias that isn't referenced on us from the outside can be removed. We avoid doing that when: - // 1. The caller requests we don't (top-level select, scalar subquery, select within a set operation where the other is distinct - - // projection must be preserved as-is) - // 2. The select has distinct (removing a projection changes which rows get projected out) - var prunedProjection = _projection; - if (pruneProjection && !IsDistinct && ((Alias ?? pruningVisitor.CurrentTableAlias) is string currentTableAlias)) - { - if (referencedColumnMap.TryGetValue(currentTableAlias, out var referencedProjectionAliases)) - { - for (var i = _projection.Count - 1; i >= 0; i--) - { - if (!referencedProjectionAliases.Contains(_projection[i].Alias)) - { - _projection.RemoveAt(i); - } - } - } - else - { - _projection.Clear(); - } - } - - // When visiting the select's tables, we track the alias so that we know it when processing nested table expressions (e.g. within - // set operations); make sure that when visiting other clauses (e.g. predicate), the tracked table alias is null. - var parentTableAlias = pruningVisitor.CurrentTableAlias; - pruningVisitor.CurrentTableAlias = null; - - // First visit all the non-table clauses of the SelectExpression - this will populate referencedColumnMap with all columns - // referenced on all tables. - if (pruningVisitor.VisitAndConvert(prunedProjection) is var newProjection && newProjection != _projection) - { - _projection.Clear(); - _projection.AddRange(newProjection); - } - - Predicate = (SqlExpression?)pruningVisitor.Visit(Predicate); - - if (pruningVisitor.VisitAndConvert(_groupBy) is var newGroupBy && newGroupBy != _groupBy) - { - _groupBy.Clear(); - _groupBy.AddRange(newGroupBy); - } - - Having = (SqlExpression?)pruningVisitor.Visit(Having); - - if (pruningVisitor.VisitAndConvert(_orderings) is var newOrderings && newOrderings != _orderings) - { - _orderings.Clear(); - _orderings.AddRange(newOrderings); - } - - Offset = (SqlExpression?)pruningVisitor.Visit(Offset); - Limit = (SqlExpression?)pruningVisitor.Visit(Limit); - - // TODO: This should happen earlier, not as part of pruning. - _identifier.Clear(); - _childIdentifiers.Clear(); - - foreach (var kvp in _tpcDiscriminatorValues) - { - var newColumn = pruningVisitor.Visit(kvp.Value.Item1); - Check.DebugAssert(newColumn == kvp.Value.Item1, "TPC discriminator column replaced during pruning"); - } - - // We've visited the entire select expression except for the table, and now have referencedColumnMap fully populated with column - // references to all its tables. - // Go over the tables, removing any which aren't referenced anywhere (and are prunable). - // We do this in backwards order, so that later joins referencing earlier tables in the predicate don't cause the earlier tables - // to be preserved. - for (var i = _tables.Count - 1; i >= 0; i--) - { - var table = _tables[i]; - var alias = table.GetRequiredAlias(); - - if (!referencedColumnMap.ContainsKey(alias) - // Note that we only prune joins; pruning the main is more complex because other tables need to unwrap joins to be main. - // We also only prune joins explicitly marked as prunable; otherwise e.g. an inner join may be needed to filter out rows - // even if no column references it. - && table is JoinExpressionBase { IsPrunable: true }) - { - _tables.RemoveAt(i); - continue; - } - - // The table wasn't pruned - visit it. This may add references to a previous table, causing it to be preserved (e.g. if it's - // referenced from the join predicate), or just prune something inside (e.g. a subquery table). - // Note that we track the table's alias in CurrentTableAlias, in case it contains nested selects (i.e. within set operations), - // which don't have their own alias. - pruningVisitor.CurrentTableAlias = alias; - var newTable = (TableExpressionBase)pruningVisitor.Visit(table); - - if (newTable != table) - { - _tables[i] = newTable; - } - } - - pruningVisitor.CurrentTableAlias = parentTableAlias; - } + public SelectExpression RemoveTpcTableExpression() + => (SelectExpression)new TpcTableExpressionRemovingExpressionVisitor(_sqlAliasManager).Visit(this); private Dictionary ConvertProjectionMappingToClientProjections( Dictionary projectionMapping, @@ -4606,7 +4489,7 @@ private ColumnExpression GenerateOuterColumn( /// protected override Expression VisitChildren(ExpressionVisitor visitor) { - if (_mutable) + if (IsMutable) { VisitList(_tables, inPlace: true, out _); @@ -4774,7 +4657,7 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) Limit = limit, IsDistinct = IsDistinct, Tags = Tags, - _mutable = false + IsMutable = false }; foreach (var kvp in newTpcDiscriminatorValues) { @@ -4857,7 +4740,22 @@ public SelectExpression Update( SqlExpression? limit, SqlExpression? offset) { - Check.DebugAssert(!_mutable, "SelectExpression shouldn't be mutable when calling this method."); + if (IsMutable) + { + throw new InvalidOperationException(RelationalStrings.SelectExpressionUpdateNotSupportedWhileMutable); + } + + if (projections == Projection + && tables == Tables + && predicate == Predicate + && groupBy == GroupBy + && having == Having + && orderings == Orderings + && limit == Limit + && offset == Offset) + { + return this; + } var projectionMapping = new Dictionary(); foreach (var (projectionMember, expression) in _projectionMapping) @@ -4865,9 +4763,6 @@ public SelectExpression Update( projectionMapping[projectionMember] = expression; } - // TODO: This assumes that no tables were added or removed (e.g. pruning). Update should be usable for that case. - // TODO: This always creates a new expression. It should check if anything changed instead (#31276), allowing us to remove "changed" - // tracking from calling code. var newSelectExpression = new SelectExpression( Alias, projections.ToList(), tables.ToList(), groupBy.ToList(), orderings.ToList(), Annotations, _sqlAliasManager) { @@ -4879,7 +4774,7 @@ public SelectExpression Update( Limit = limit, IsDistinct = IsDistinct, Tags = Tags, - _mutable = false + IsMutable = false }; // We don't copy identifiers because when we are doing reconstruction so projection is already applied. @@ -4895,7 +4790,7 @@ protected override SelectExpression WithAnnotations(IReadOnlyDictionary public override SelectExpression WithAlias(string newAlias) { - Check.DebugAssert(!_mutable, "Can't change alias on mutable SelectExpression"); + Check.DebugAssert(!IsMutable, "Can't change alias on mutable SelectExpression"); return new SelectExpression(newAlias, _projection, _tables, _groupBy, _orderings, Annotations, _sqlAliasManager) { @@ -4907,7 +4802,7 @@ public override SelectExpression WithAlias(string newAlias) Limit = Limit, IsDistinct = IsDistinct, Tags = Tags, - _mutable = false + IsMutable = false }; } @@ -5072,25 +4967,81 @@ public override bool Equals(object? obj) || obj is SelectExpression selectExpression && Equals(selectExpression)); + // Note that we vary our Equals/GetHashCode logic based on whether the SelectExpression is mutable or not; in the former case we use + // reference logic, whereas once the expression becomes immutable (after translation), we switch to value logic. + // This isn't a good state of affairs (e.g. it's impossible to keep a SelectExpression - or any expression containing one - as a + // dictionary key across the state change from mutable to immutable (we fortunately don't do that). private bool Equals(SelectExpression selectExpression) - /* - * This is intentionally reference equals. - * SelectExpression can appear at 2 levels, - * 1. Top most level which is always same reference when translation phase, post-translation it can change in - * ShapedQueryExpression where it would cause reconstruction. Reconstruction is cheaper than computing whole Equals - * 2. Nested level component inside top level SelectExpression where it could change the reference and reconstruct SQL tree. - * Since we assign unique aliases to components, 2 different SelectExpression would never match. And only positive case could - * happen when it is reference equal. - * If inner changed with in-place mutation then reference would be same, if inner changed with no mutation then it will cause - * reconstruction causing different reference. - */ - => ReferenceEquals(this, selectExpression); - + => IsMutable + ? ReferenceEquals(this, selectExpression) + : base.Equals(selectExpression) + && Tables.SequenceEqual(selectExpression.Tables) + && (Predicate is null && selectExpression.Predicate is null + || Predicate is not null && Predicate.Equals(selectExpression.Predicate)) + && GroupBy.SequenceEqual(selectExpression.GroupBy) + && (Having is null && selectExpression.Having is null + || Having is not null && Having.Equals(selectExpression.Having)) + && Projection.SequenceEqual(selectExpression.Projection) + && Orderings.SequenceEqual(selectExpression.Orderings) + && (Limit is null && selectExpression.Limit is null + || Limit is not null && Limit.Equals(selectExpression.Limit)) + && (Offset is null && selectExpression.Offset is null + || Offset is not null && Offset.Equals(selectExpression.Offset)); + + // ReSharper disable NonReadonlyMemberInGetHashCode /// public override int GetHashCode() - // Since equality above is reference equality, hash code can also be based on reference. - => RuntimeHelpers.GetHashCode(this); + { + if (IsMutable) + { + return RuntimeHelpers.GetHashCode(this); + } + + var hash = new HashCode(); + hash.Add(base.GetHashCode()); + + foreach (var table in Tables) + { + hash.Add(table); + } - internal bool IsMutable - => _mutable; + if (Predicate is not null) + { + hash.Add(Predicate); + } + + foreach (var groupingKey in GroupBy) + { + hash.Add(groupingKey); + } + + if (Having is not null) + { + hash.Add(Having); + } + + foreach (var projection in Projection) + { + hash.Add(projection); + } + + foreach (var ordering in Orderings) + { + hash.Add(ordering); + } + + if (Limit is not null) + { + hash.Add(Limit); + } + + if (Offset is not null) + { + hash.Add(Offset); + } + + return hash.ToHashCode(); + + } + // ReSharper restore NonReadonlyMemberInGetHashCode } diff --git a/src/EFCore.Relational/Query/SqlExpressions/UpdateExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/UpdateExpression.cs index edcb109e098..3a59decd327 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/UpdateExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/UpdateExpression.cs @@ -104,7 +104,7 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) return selectExpression == SelectExpression && table == Table && columnValueSetters is null ? this - : new UpdateExpression(Table, selectExpression, columnValueSetters ?? ColumnValueSetters); + : new UpdateExpression(table, selectExpression, columnValueSetters ?? ColumnValueSetters); } /// diff --git a/src/EFCore.Relational/Query/SqlNullabilityProcessor.cs b/src/EFCore.Relational/Query/SqlNullabilityProcessor.cs index 838a24884ad..0cf1e315fbb 100644 --- a/src/EFCore.Relational/Query/SqlNullabilityProcessor.cs +++ b/src/EFCore.Relational/Query/SqlNullabilityProcessor.cs @@ -251,7 +251,6 @@ protected virtual TableExpressionBase Visit(TableExpressionBase tableExpressionB /// An optimized select expression. protected virtual SelectExpression Visit(SelectExpression selectExpression) { - var changed = false; var projections = (List)selectExpression.Projection; for (var i = 0; i < selectExpression.Projection.Count; i++) { @@ -265,8 +264,6 @@ protected virtual SelectExpression Visit(SelectExpression selectExpression) { projections.Add(selectExpression.Projection[j]); } - - changed = true; } if (projections != selectExpression.Projection) @@ -288,8 +285,6 @@ protected virtual SelectExpression Visit(SelectExpression selectExpression) { tables.Add(selectExpression.Tables[j]); } - - changed = true; } if (tables != selectExpression.Tables) @@ -299,12 +294,10 @@ protected virtual SelectExpression Visit(SelectExpression selectExpression) } var predicate = Visit(selectExpression.Predicate, allowOptimizedExpansion: true, out _); - changed |= predicate != selectExpression.Predicate; if (IsTrue(predicate)) { predicate = null; - changed = true; } var groupBy = (List)selectExpression.GroupBy; @@ -320,8 +313,6 @@ protected virtual SelectExpression Visit(SelectExpression selectExpression) { groupBy.Add(selectExpression.GroupBy[j]); } - - changed = true; } if (groupBy != selectExpression.GroupBy) @@ -331,12 +322,10 @@ protected virtual SelectExpression Visit(SelectExpression selectExpression) } var having = Visit(selectExpression.Having, allowOptimizedExpansion: true, out _); - changed |= having != selectExpression.Having; if (IsTrue(having)) { having = null; - changed = true; } var orderings = (List)selectExpression.Orderings; @@ -352,8 +341,6 @@ protected virtual SelectExpression Visit(SelectExpression selectExpression) { orderings.Add(selectExpression.Orderings[j]); } - - changed = true; } if (orderings != selectExpression.Orderings) @@ -363,15 +350,10 @@ protected virtual SelectExpression Visit(SelectExpression selectExpression) } var offset = Visit(selectExpression.Offset, out _); - changed |= offset != selectExpression.Offset; var limit = Visit(selectExpression.Limit, out _); - changed |= limit != selectExpression.Limit; - return changed - ? selectExpression.Update( - projections, tables, predicate, groupBy, having, orderings, limit, offset) - : selectExpression; + return selectExpression.Update(projections, tables, predicate, groupBy, having, orderings, limit, offset); } /// diff --git a/src/EFCore.Relational/Query/SqlTreePruner.cs b/src/EFCore.Relational/Query/SqlTreePruner.cs index 1b6ffbbc54a..21bb82bf9e5 100644 --- a/src/EFCore.Relational/Query/SqlTreePruner.cs +++ b/src/EFCore.Relational/Query/SqlTreePruner.cs @@ -21,9 +21,7 @@ public class SqlTreePruner : ExpressionVisitor /// /// Maps table aliases to the list of column aliases found referenced on them. /// - // TODO: Make this protected after SelectExpression.Prune is moved into this visitor - [EntityFrameworkInternal] - public virtual IReadOnlyDictionary> ReferencedColumnMap => _referencedColumnMap; + protected virtual IReadOnlyDictionary> ReferencedColumnMap => _referencedColumnMap; /// /// When visiting a nested (e.g. a select within a set operation), this holds the table alias @@ -36,9 +34,7 @@ public class SqlTreePruner : ExpressionVisitor /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - // TODO: Make this protected after SelectExpression.Prune is moved into this visitor - [EntityFrameworkInternal] - public virtual string? CurrentTableAlias { get; set; } + protected virtual string? CurrentTableAlias { get; set; } /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -62,7 +58,7 @@ protected override Expression VisitExtension(Expression node) case ShapedQueryExpression shapedQueryExpression: _referencedColumnMap.Clear(); return shapedQueryExpression.Update( - ((SelectExpression)shapedQueryExpression.QueryExpression).PruneToplevel(this), + PruneToplevelSelect((SelectExpression)shapedQueryExpression.QueryExpression), Visit(shapedQueryExpression.ShaperExpression)); case RelationalSplitCollectionShaperExpression relationalSplitCollectionShaperExpression: @@ -70,20 +66,18 @@ protected override Expression VisitExtension(Expression node) return relationalSplitCollectionShaperExpression.Update( relationalSplitCollectionShaperExpression.ParentIdentifier, relationalSplitCollectionShaperExpression.ChildIdentifier, - relationalSplitCollectionShaperExpression.SelectExpression.PruneToplevel(this), + PruneToplevelSelect(relationalSplitCollectionShaperExpression.SelectExpression), Visit(relationalSplitCollectionShaperExpression.InnerShaper)); case DeleteExpression deleteExpression: - return deleteExpression.Update(deleteExpression.Table, deleteExpression.SelectExpression.PruneToplevel(this)); + return deleteExpression.Update(deleteExpression.Table, PruneToplevelSelect(deleteExpression.SelectExpression)); case UpdateExpression updateExpression: // Note that we must visit the setters before we visit the select, since the setters can reference tables inside it. var visitedSetters = updateExpression.ColumnValueSetters .Select(e => e with { Value = (SqlExpression)Visit(e.Value) }) .ToList(); - return updateExpression.Update( - updateExpression.SelectExpression.PruneToplevel(this), - visitedSetters); + return updateExpression.Update(PruneToplevelSelect(updateExpression.SelectExpression), visitedSetters); // The following remaining cases deal with recursive visitation (i.e. non-top-level things) @@ -96,8 +90,7 @@ protected override Expression VisitExtension(Expression node) // Note that this only handles nested selects, and *not* the top-level select - that was already handled above in the first // cases. case SelectExpression select: - select.Prune(this, pruneProjection: true); - return select; + return PruneSelect(select, preserveProjection: false); // PredicateJoinExpressionBase.VisitChildren visits the table before the predicate, but we must visit the predicate first // since it can contain columns referencing the table's projection (which we shouldn't prune). @@ -109,14 +102,13 @@ protected override Expression VisitExtension(Expression node) // Never prune the projection of a scalar subquery. Note that there are never columns referencing scalar subqueries, since // they're not tables. case ScalarSubqueryExpression scalarSubquery: - scalarSubquery.Subquery.Prune(this, pruneProjection: false); - return scalarSubquery; + return scalarSubquery.Update(PruneSelect(scalarSubquery.Subquery, preserveProjection: true)); // Same for subqueries inside InExpression case InExpression { Subquery: SelectExpression subquery } inExpression: - var item = (SqlExpression)Visit(inExpression.Item); - subquery.Prune(this, pruneProjection: false); - return inExpression.Update(item, subquery); + var visitedItem = (SqlExpression)Visit(inExpression.Item); + var visitedSubquery = PruneSelect(subquery, preserveProjection: true); + return inExpression.Update(visitedItem, visitedSubquery); // If the set operation is distinct (union/intersect/except, but not concat), we cannot prune the projection since that would // affect which rows come out. @@ -126,9 +118,9 @@ protected override Expression VisitExtension(Expression node) case SetOperationBase { Source1: var source1, Source2: var source2 } setOperation when setOperation.IsDistinct || source1.IsDistinct || source2.IsDistinct: { - source1.Prune(this, pruneProjection: false); - source2.Prune(this, pruneProjection: false); - return setOperation; + return setOperation.Update( + PruneSelect(source1, preserveProjection: true), + PruneSelect(source2, preserveProjection: true)); } default: @@ -138,4 +130,135 @@ protected override Expression VisitExtension(Expression node) void RegisterTable(string tableAlias, ColumnExpression column) => _referencedColumnMap.GetOrAddNew(tableAlias).Add(column.Name); } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + [EntityFrameworkInternal] + protected virtual SelectExpression PruneToplevelSelect(SelectExpression select) + { + // TODO: This doesn't belong in pruning, take a deeper look at how we manage TPC, #32873 + select = select.RemoveTpcTableExpression(); + return PruneSelect(select, preserveProjection: true); + } + + /// + /// Prunes a , removes tables inside it which aren't referenced, and optionally also projections + /// which aren't referenced from outside it. + /// + /// The to prune. + /// Whether to prune projections if they aren't referenced from the outside. + /// A pruned copy of , or the same instance of nothing was pruned. + protected virtual SelectExpression PruneSelect(SelectExpression select, bool preserveProjection) + { + Check.DebugAssert(!select.IsMutable, "Mutable SelectExpression found when pruning"); + + var referencedColumnMap = ReferencedColumnMap; + + // When visiting the select's tables, we track the alias so that we know it when processing nested table expressions (e.g. within + // set operations); make sure that when visiting other clauses (e.g. predicate), the tracked table alias is null. + var currentSelectAlias = select.Alias ?? CurrentTableAlias; + var parentTableAlias = CurrentTableAlias; + CurrentTableAlias = null; + + // First visit all the non-table clauses of the SelectExpression - this will populate referencedColumnMap with all columns + // referenced on all tables. + + // Go over the projections, prune any that isn't referenced from the outside and visiting those that are. + // We avoid pruning projections when: + // 1. The caller requests we don't (top-level select, scalar subquery, select within a set operation where the other is distinct - + // projection must be preserved as-is) + // 2. The select has distinct (removing a projection changes which rows get projected out) + preserveProjection |= select.IsDistinct || currentSelectAlias is null; + List? projections = null; + + var referencedProjectionAliases = currentSelectAlias is not null ? referencedColumnMap.GetValueOrDefault(currentSelectAlias) : null; + + for (var i = 0; i < select.Projection.Count; i++) + { + var projection = select.Projection[i]; + + var visitedProjection = preserveProjection || referencedProjectionAliases?.Contains(projection.Alias) == true + ? (ProjectionExpression)Visit(projection) + : null; // "visited" in the sense of pruned + + if (visitedProjection != projection && projections is null) + { + projections = new(select.Projection.Count); + for (var j = 0; j < i; j++) + { + projections.Add(select.Projection[j]); + } + } + + if (projections is not null && visitedProjection is not null) + { + projections.Add(visitedProjection); + } + } + + var predicate = (SqlExpression?)Visit(select.Predicate); + var groupBy = this.VisitAndConvert(select.GroupBy); + var having = (SqlExpression?)Visit(select.Having); + var orderings = this.VisitAndConvert(select.Orderings); + var offset = (SqlExpression?)Visit(select.Offset); + var limit = (SqlExpression?)Visit(select.Limit); + + // Note that we don't visit/copy _identifier, _childIdentifier and _tpcDiscriminatorValues; these have already been applied + // and are no longer needed. + + // We've visited the entire select expression except for the table, and now have referencedColumnMap fully populated with column + // references to all its tables. + // Go over the tables, removing any which aren't referenced anywhere (and are prunable). + // We do this in backwards order, so that later joins referencing earlier tables in the predicate don't cause the earlier tables + // to be preserved. + List? tables = null; + for (var i = select.Tables.Count - 1; i >= 0; i--) + { + var table = select.Tables[i]; + var alias = table.GetRequiredAlias(); + TableExpressionBase? visitedTable; + + if (!referencedColumnMap.ContainsKey(alias) + && table is JoinExpressionBase { IsPrunable: true }) + { + // If no column references the table, prune it. + // Note that we only prune joins; pruning the main is more complex because other tables need to unwrap joins to be main. + // We also only prune joins explicitly marked as prunable; otherwise e.g. an inner join may be needed to filter out rows + // even if no column references it. + visitedTable = null; + } + else + { + // The table wasn't pruned - visit it. This may add references to a previous table, causing it to be preserved (e.g. if it's + // referenced from the join predicate), or just prune something inside (e.g. a subquery table). + // Note that we track the table's alias in CurrentTableAlias, in case it contains nested selects (i.e. within set + // operations), which don't have their own alias. + CurrentTableAlias = alias; + visitedTable = (TableExpressionBase)Visit(table); + } + + if (visitedTable != table && tables is null) + { + tables = new List(select.Tables.Count); + for (var j = i + 1; j < select.Tables.Count; j++) + { + tables.Add(select.Tables[j]); + } + } + + if (tables is not null && visitedTable is not null) + { + tables.Insert(0, visitedTable); + } + } + + CurrentTableAlias = parentTableAlias; + + return select.Update( + projections ?? select.Projection, tables ?? select.Tables, predicate, groupBy, having, orderings, limit, offset); + } } diff --git a/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs b/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs index 217600dfbe5..55e64112f64 100644 --- a/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs +++ b/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs @@ -281,64 +281,25 @@ protected override Expression VisitLike(LikeExpression likeExpression) /// protected override Expression VisitSelect(SelectExpression selectExpression) { - var changed = false; var parentSearchCondition = _isSearchCondition; - var projections = new List(); _isSearchCondition = false; - foreach (var item in selectExpression.Projection) - { - var updatedProjection = (ProjectionExpression)Visit(item); - projections.Add(updatedProjection); - changed |= updatedProjection != item; - } - var tables = new List(); - foreach (var table in selectExpression.Tables) - { - var newTable = (TableExpressionBase)Visit(table); - changed |= newTable != table; - tables.Add(newTable); - } + var projections = this.VisitAndConvert(selectExpression.Projection); + var tables = this.VisitAndConvert(selectExpression.Tables); + var groupBy = this.VisitAndConvert(selectExpression.GroupBy); + var orderings = this.VisitAndConvert(selectExpression.Orderings); + var offset = (SqlExpression?)Visit(selectExpression.Offset); + var limit = (SqlExpression?)Visit(selectExpression.Limit); _isSearchCondition = true; - var predicate = (SqlExpression?)Visit(selectExpression.Predicate); - changed |= predicate != selectExpression.Predicate; - - var groupBy = new List(); - _isSearchCondition = false; - foreach (var groupingKey in selectExpression.GroupBy) - { - var newGroupingKey = (SqlExpression)Visit(groupingKey); - changed |= newGroupingKey != groupingKey; - groupBy.Add(newGroupingKey); - } - _isSearchCondition = true; + var predicate = (SqlExpression?)Visit(selectExpression.Predicate); var havingExpression = (SqlExpression?)Visit(selectExpression.Having); - changed |= havingExpression != selectExpression.Having; - - var orderings = new List(); - _isSearchCondition = false; - foreach (var ordering in selectExpression.Orderings) - { - var orderingExpression = (SqlExpression)Visit(ordering.Expression); - changed |= orderingExpression != ordering.Expression; - orderings.Add(ordering.Update(orderingExpression)); - } - - var offset = (SqlExpression?)Visit(selectExpression.Offset); - changed |= offset != selectExpression.Offset; - - var limit = (SqlExpression?)Visit(selectExpression.Limit); - changed |= limit != selectExpression.Limit; _isSearchCondition = parentSearchCondition; - return changed - ? selectExpression.Update( - projections, tables, predicate, groupBy, havingExpression, orderings, limit, offset) - : selectExpression; + return selectExpression.Update(projections, tables, predicate, groupBy, havingExpression, orderings, limit, offset); } /// diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerSqlTreePruner.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerSqlTreePruner.cs index 8834ae1c2a7..48af433f3b9 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerSqlTreePruner.cs +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerSqlTreePruner.cs @@ -27,9 +27,7 @@ protected override Expression VisitExtension(Expression node) case SqlServerOpenJsonExpression { ColumnInfos: IReadOnlyList columnInfos } openJson: var visitedJson = (SqlExpression)Visit(openJson.JsonExpression); -#pragma warning disable EF1001 // ReferencedColumnMap is pubternal; should be made protected if (ReferencedColumnMap.TryGetValue(openJson.Alias, out var referencedAliases)) -#pragma warning restore EF1001 { List? newColumnInfos = null;