Query: API Review changes (#25504)

Part of #24743

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

@@ -17,7 +17,7 @@ namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions
     ///         not used in application code.
     ///     </para>
     /// </summary>
-    public class FromSqlExpression : TableExpressionBase, ICloneable
+    public class FromSqlExpression : TableExpressionBase, IClonableTableExpressionBase
     {
         /// <summary>
         ///     Creates a new instance of the <see cref="FromSqlExpression" /> class.
@@ -96,7 +96,7 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
         }
 
         /// <inheritdoc />
-        public virtual object Clone() => new FromSqlExpression(Alias, Sql, Arguments);
+        public virtual TableExpressionBase Clone() => new FromSqlExpression(Alias, Sql, Arguments);
 
         /// <inheritdoc />
         protected override void Print(ExpressionPrinter expressionPrinter)

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

@@ -0,0 +1,23 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions
+{
+    /// <summary>
+    ///     <para>
+    ///         An interface that represents a table source in a SQL tree which can be cloned.
+    ///     </para>
+    ///     <para>
+    ///         This interface is typically used by database providers (and other extensions). It is generally
+    ///         not used in application code.
+    ///     </para>
+    /// </summary>
+    public interface IClonableTableExpressionBase
+    {
+        /// <summary>
+        ///     Creates a new object that is a copy of the current instance.
+        /// </summary>
+        /// <returns> A new object that is a copy of this instance. </returns>
+        TableExpressionBase Clone();
+    }
+}

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

@@ -739,8 +739,6 @@ private sealed class CloningExpressionVisitor : ExpressionVisitor
             {
                 if (expression is SelectExpression selectExpression)
                 {
-                    // We ignore projection binding related elements as we don't want to copy them over for top level
-                    // Nested level will have _projection populated and no binding elements
                     var newProjections = selectExpression._projection.Select(Visit).ToList<ProjectionExpression>();
 
                     var newTables = selectExpression._tables.Select(Visit).ToList<TableExpressionBase>();
@@ -792,7 +790,7 @@ private sealed class CloningExpressionVisitor : ExpressionVisitor
 
                 }
 
-                return expression is ICloneable cloneable ? (Expression)cloneable.Clone() : base.Visit(expression);
+                return expression is IClonableTableExpressionBase cloneable ? (Expression)cloneable.Clone() : base.Visit(expression);
             }
         }
 

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

@@ -18,7 +18,7 @@ namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions
     ///         an issue at https://github.com/dotnet/efcore.
     ///     </para>
     /// </summary>
-    public sealed class TableExpression : TableExpressionBase, ICloneable
+    public sealed class TableExpression : TableExpressionBase, IClonableTableExpressionBase
     {
         internal TableExpression(ITableBase table)
             : base(table.Name.Substring(0, 1).ToLowerInvariant())
@@ -67,7 +67,7 @@ protected override void Print(ExpressionPrinter expressionPrinter)
         public ITableBase Table { get; }
 
         /// <inheritdoc />
-        public object Clone()
+        public TableExpressionBase Clone()
             => new TableExpression(Table) { Alias = Alias };
 
         /// <inheritdoc />

src/EFCore.SqlServer/Extensions/SqlServerDbSetExtensions.cs

@@ -36,10 +36,13 @@ public static class SqlServerDbSetExtensions
             Check.NotNull(source, nameof(source));
 
             var queryableSource = (IQueryable)source;
+            var queryRootExpression = (QueryRootExpression)queryableSource.Expression;
+            var entityType = queryRootExpression.EntityType;
 
             return queryableSource.Provider.CreateQuery<TEntity>(
-                GenerateTemporalAsOfQueryRoot<TEntity>(
-                    queryableSource,
+                new TemporalAsOfQueryRootExpression(
+                    queryRootExpression.QueryProvider!,
+                    entityType,
                     utcPointInTime)).AsNoTracking();
         }
 
@@ -73,13 +76,15 @@ public static class SqlServerDbSetExtensions
             Check.NotNull(source, nameof(source));
 
             var queryableSource = (IQueryable)source;
+            var queryRootExpression = (QueryRootExpression)queryableSource.Expression;
+            var entityType = queryRootExpression.EntityType;
 
             return queryableSource.Provider.CreateQuery<TEntity>(
-                GenerateRangeTemporalQueryRoot<TEntity>(
-                    queryableSource,
+                new TemporalFromToQueryRootExpression(
+                    queryRootExpression.QueryProvider!,
+                    entityType,
                     utcFrom,
-                    utcTo,
-                    TemporalOperationType.FromTo)).AsNoTracking();
+                    utcTo)).AsNoTracking();
         }
 
         /// <summary>
@@ -112,13 +117,15 @@ public static class SqlServerDbSetExtensions
             Check.NotNull(source, nameof(source));
 
             var queryableSource = (IQueryable)source;
+            var queryRootExpression = (QueryRootExpression)queryableSource.Expression;
+            var entityType = queryRootExpression.EntityType;
 
             return queryableSource.Provider.CreateQuery<TEntity>(
-                GenerateRangeTemporalQueryRoot<TEntity>(
-                    queryableSource,
+                new TemporalBetweenQueryRootExpression(
+                    queryRootExpression.QueryProvider!,
+                    entityType,
                     utcFrom,
-                    utcTo,
-                    TemporalOperationType.Between)).AsNoTracking();
+                    utcTo)).AsNoTracking();
         }
 
         /// <summary>
@@ -151,13 +158,15 @@ public static class SqlServerDbSetExtensions
             Check.NotNull(source, nameof(source));
 
             var queryableSource = (IQueryable)source;
+            var queryRootExpression = (QueryRootExpression)queryableSource.Expression;
+            var entityType = queryRootExpression.EntityType;
 
             return queryableSource.Provider.CreateQuery<TEntity>(
-                GenerateRangeTemporalQueryRoot<TEntity>(
-                    queryableSource,
+                new TemporalContainedInQueryRootExpression(
+                    queryRootExpression.QueryProvider!,
+                    entityType,
                     utcFrom,
-                    utcTo,
-                    TemporalOperationType.ContainedIn)).AsNoTracking();
+                    utcTo)).AsNoTracking();
         }
 
         /// <summary>
@@ -180,42 +189,9 @@ public static class SqlServerDbSetExtensions
             var queryRootExpression = (QueryRootExpression)queryableSource.Expression;
             var entityType = queryRootExpression.EntityType;
 
-            var temporalQueryRootExpression = new TemporalAllQueryRootExpression(
-                queryRootExpression.QueryProvider!,
-                entityType);
-
-            return queryableSource.Provider.CreateQuery<TEntity>(temporalQueryRootExpression)
-                .AsNoTracking();
-        }
-
-        private static Expression GenerateTemporalAsOfQueryRoot<TEntity>(
-            IQueryable source,
-            DateTime pointInTime)
-        {
-            var queryRootExpression = (QueryRootExpression)source.Expression;
-            var entityType = queryRootExpression.EntityType;
-
-            return new TemporalAsOfQueryRootExpression(
-                queryRootExpression.QueryProvider!,
-                entityType,
-                pointInTime: pointInTime);
-        }
-
-        private static Expression GenerateRangeTemporalQueryRoot<TEntity>(
-            IQueryable source,
-            DateTime from,
-            DateTime to,
-            TemporalOperationType temporalOperationType)
-        {
-            var queryRootExpression = (QueryRootExpression)source.Expression;
-            var entityType = queryRootExpression.EntityType;
-
-            return new TemporalRangeQueryRootExpression(
-                queryRootExpression.QueryProvider!,
-                entityType,
-                from: from,
-                to: to,
-                temporalOperationType: temporalOperationType);
+            return queryableSource.Provider.CreateQuery<TEntity>(
+                new TemporalAllQueryRootExpression(
+                    queryRootExpression.QueryProvider!, entityType)).AsNoTracking();
         }
     }
 }

src/EFCore.SqlServer/Query/Internal/SqlServerNavigationExpansionExtensibilityHelper.cs

@@ -35,9 +35,7 @@ public SqlServerNavigationExpansionExtensibilityHelper(NavigationExpansionExtens
         /// </summary>
         public override QueryRootExpression CreateQueryRoot(IEntityType entityType, QueryRootExpression? source)
         {
-            if (source is TemporalAsOfQueryRootExpression
-                || source is TemporalRangeQueryRootExpression
-                || source is TemporalAllQueryRootExpression)
+            if (source is TemporalQueryRootExpression)
             {
                 if (!entityType.GetRootType().IsTemporal())
                 {
@@ -51,7 +49,7 @@ public override QueryRootExpression CreateQueryRoot(IEntityType entityType, Quer
                         : new TemporalAsOfQueryRootExpression(entityType, asOf.PointInTime);
                 }
 
-                throw new InvalidOperationException(SqlServerStrings.TemporalNavigationExpansionOnlySupportedForAsOf(nameof(TemporalOperationType.AsOf)));
+                throw new InvalidOperationException(SqlServerStrings.TemporalNavigationExpansionOnlySupportedForAsOf("AsOf"));
             }
 
             return base.CreateQueryRoot(entityType, source);
@@ -70,13 +68,8 @@ public override bool AreQueryRootsCompatible(QueryRootExpression? first, QueryRo
                 return false;
             }
 
-            var firstTemporal = first is TemporalAsOfQueryRootExpression
-                || first is TemporalRangeQueryRootExpression
-                || first is TemporalAllQueryRootExpression;
-
-            var secondTemporal = second is TemporalAsOfQueryRootExpression
-                || second is TemporalRangeQueryRootExpression
-                || second is TemporalAllQueryRootExpression;
+            var firstTemporal = first is TemporalQueryRootExpression;
+            var secondTemporal = second is TemporalQueryRootExpression;
 
             if (firstTemporal && secondTemporal)
             {

src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs

@@ -3,9 +3,9 @@
 
 using System;
 using System.Linq.Expressions;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Query;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
-using Microsoft.EntityFrameworkCore.SqlServer.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.Storage;
 using Microsoft.EntityFrameworkCore.Utilities;
 
@@ -112,52 +112,49 @@ protected override Expression VisitExtension(Expression extensionExpression)
         {
             if (extensionExpression is TemporalTableExpression temporalTableExpression)
             {
-                Sql
-                    .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(temporalTableExpression.Name, temporalTableExpression.Schema))
+                Sql.Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(temporalTableExpression.Name, temporalTableExpression.Schema))
                     .Append(" FOR SYSTEM_TIME ");
 
-                switch (temporalTableExpression.TemporalOperationType)
+                switch (temporalTableExpression)
                 {
-                    case TemporalOperationType.AsOf:
-                        Sql
-                            .Append("AS OF ")
-                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(temporalTableExpression.PointInTime));
+                    case TemporalAsOfTableExpression asOf:
+                        Sql.Append("AS OF ")
+                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(asOf.PointInTime));
                         break;
 
-                    case TemporalOperationType.FromTo:
-                        Sql
-                            .Append("FROM ")
-                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(temporalTableExpression.From))
+                    case TemporalFromToTableExpression fromTo:
+                        Sql.Append("FROM ")
+                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(fromTo.From))
                             .Append(" TO ")
-                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(temporalTableExpression.To));
+                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(fromTo.To));
                         break;
 
-                    case TemporalOperationType.Between:
-                        Sql
-                            .Append("BETWEEN ")
-                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(temporalTableExpression.From))
+                    case TemporalBetweenTableExpression between:
+                        Sql.Append("BETWEEN ")
+                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(between.From))
                             .Append(" AND ")
-                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(temporalTableExpression.To));
+                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(between.To));
                         break;
 
-                    case TemporalOperationType.ContainedIn:
-                        Sql
-                            .Append("CONTAINED IN (")
-                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(temporalTableExpression.From))
+                    case TemporalContainedInTableExpression containedIn:
+                        Sql.Append("CONTAINED IN (")
+                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(containedIn.From))
                             .Append(", ")
-                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(temporalTableExpression.To))
+                            .Append(Sql.TypeMappingSource.GetMapping(typeof(DateTime)).GenerateSqlLiteral(containedIn.To))
                             .Append(")");
                         break;
 
-                    default:
+                    case TemporalAllTableExpression _:
                         Sql.Append("ALL");
                         break;
+
+                    default:
+                        throw new InvalidOperationException(temporalTableExpression.Print());
                 }
 
                 if (temporalTableExpression.Alias != null)
                 {
-                    Sql
-                        .Append(AliasSeparator)
+                    Sql.Append(AliasSeparator)
                         .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(temporalTableExpression.Alias));
                 }
 

src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs

@@ -3,8 +3,8 @@
 
 using System.Linq;
 using System.Linq.Expressions;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Query;
-using Microsoft.EntityFrameworkCore.SqlServer.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.Storage;
 
 namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal
@@ -60,24 +60,18 @@ protected override QueryableMethodTranslatingExpressionVisitor CreateSubqueryVis
         /// </summary>
         protected override Expression VisitExtension(Expression extensionExpression)
         {
-            if (extensionExpression is TemporalAsOfQueryRootExpression
-                || extensionExpression is TemporalRangeQueryRootExpression
-                || extensionExpression is TemporalAllQueryRootExpression)
+            if (extensionExpression is TemporalQueryRootExpression queryRootExpression)
             {
-                var queryRootExpression = (QueryRootExpression)extensionExpression;
-
                 // sql server model validator will throw if entity is mapped to multiple tables
                 var table = queryRootExpression.EntityType.GetTableMappings().Single().Table;
                 var temporalTableExpression = queryRootExpression switch
                 {
-                    TemporalRangeQueryRootExpression range => new TemporalTableExpression(
-                        table,
-                        range.From,
-                        range.To,
-                        range.TemporalOperationType),
-                    TemporalAsOfQueryRootExpression asOf => new TemporalTableExpression(table, asOf.PointInTime),
-                    // all
-                    _ => new TemporalTableExpression(table),
+                    TemporalAllQueryRootExpression _ => (TemporalTableExpression)new TemporalAllTableExpression(table),
+                    TemporalAsOfQueryRootExpression asOf => new TemporalAsOfTableExpression(table, asOf.PointInTime),
+                    TemporalBetweenQueryRootExpression between => new TemporalBetweenTableExpression(table, between.From, between.To),
+                    TemporalContainedInQueryRootExpression containedIn => new TemporalContainedInTableExpression(table, containedIn.From, containedIn.To),
+                    TemporalFromToQueryRootExpression fromTo => new TemporalFromToTableExpression(table, fromTo.From, fromTo.To),
+                    _ => throw new InvalidOperationException(queryRootExpression.Print())
                 };
 
                 var selectExpression = RelationalDependencies.SqlExpressionFactory.Select(

src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs

@@ -3,7 +3,6 @@
 
 using Microsoft.EntityFrameworkCore.Query;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
-using Microsoft.EntityFrameworkCore.SqlServer.Query.SqlExpressions;
 
 namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal
 {
@@ -36,12 +35,9 @@ public class SqlServerSqlNullabilityProcessor : SqlNullabilityProcessor
         /// </summary>
         protected override TableExpressionBase Visit(TableExpressionBase tableExpressionBase)
         {
-            if (tableExpressionBase is TemporalTableExpression temporalTableExpression)
-            {
-                return temporalTableExpression;
-            }
-
-            return base.Visit(tableExpressionBase);
+            return tableExpressionBase is TemporalTableExpression temporalTableExpression
+                ? temporalTableExpression
+                : base.Visit(tableExpressionBase);
         }
     }
 }