From f52355f7a613f54f0048a32e0597f7fc963c9bb2 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Tue, 18 Jun 2019 16:53:37 +0200 Subject: [PATCH] Refactor query SQL generation for standardization * GenerateTop() implementation moved to SqlServer * GenerateLimitOffset() generates ISO SQL:2008 by default, overridden by SqlServer (and Sqlite) --- eng/common/dotnet-install.sh | 0 .../Query/Pipeline/QuerySqlGenerator.cs | 20 ++++--- .../SqlServerServiceCollectionExtensions.cs | 1 + .../Pipeline/SqlServerQuerySqlGenerator.cs | 55 +++++++++++++++++++ .../SqlServerQuerySqlGeneratorFactory.cs | 26 +++++++++ .../Query/Pipeline/SqliteQuerySqlGenerator.cs | 5 -- ...2.cs => SqliteQuerySqlGeneratorFactory.cs} | 4 +- 7 files changed, 94 insertions(+), 17 deletions(-) mode change 100644 => 100755 eng/common/dotnet-install.sh create mode 100644 src/EFCore.SqlServer/Query/Pipeline/SqlServerQuerySqlGenerator.cs create mode 100644 src/EFCore.SqlServer/Query/Pipeline/SqlServerQuerySqlGeneratorFactory.cs rename src/EFCore.Sqlite.Core/Query/Pipeline/{SqliteQuerySqlGeneratorFactory2.cs => SqliteQuerySqlGeneratorFactory.cs} (89%) diff --git a/eng/common/dotnet-install.sh b/eng/common/dotnet-install.sh old mode 100644 new mode 100755 diff --git a/src/EFCore.Relational/Query/Pipeline/QuerySqlGenerator.cs b/src/EFCore.Relational/Query/Pipeline/QuerySqlGenerator.cs index 3567d0c5e5b..853d484a892 100644 --- a/src/EFCore.Relational/Query/Pipeline/QuerySqlGenerator.cs +++ b/src/EFCore.Relational/Query/Pipeline/QuerySqlGenerator.cs @@ -531,19 +531,12 @@ protected virtual string GenerateOperator(SqlBinaryExpression binaryExpression) protected virtual void GenerateTop(SelectExpression selectExpression) { - if (selectExpression.Limit != null - && selectExpression.Offset == null) - { - _relationalCommandBuilder.Append("TOP("); - - Visit(selectExpression.Limit); - - _relationalCommandBuilder.Append(") "); - } } protected virtual void GenerateLimitOffset(SelectExpression selectExpression) { + // The below implements ISO SQL:2008 + if (selectExpression.Offset != null) { _relationalCommandBuilder.AppendLine() @@ -562,6 +555,15 @@ protected virtual void GenerateLimitOffset(SelectExpression selectExpression) _relationalCommandBuilder.Append(" ROWS ONLY"); } } + else if (selectExpression.Limit != null) + { + _relationalCommandBuilder.AppendLine() + .Append("FETCH FIRST "); + + Visit(selectExpression.Limit); + + _relationalCommandBuilder.Append(" ROWS ONLY"); + } } private void GenerateList( diff --git a/src/EFCore.SqlServer/Extensions/SqlServerServiceCollectionExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerServiceCollectionExtensions.cs index 8a588a42a23..1ac600f34b8 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerServiceCollectionExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerServiceCollectionExtensions.cs @@ -90,6 +90,7 @@ public static IServiceCollection AddEntityFrameworkSqlServer([NotNull] this ISer // New Query Pipeline .TryAdd() .TryAdd() + .TryAdd() .TryAdd() .TryAdd() diff --git a/src/EFCore.SqlServer/Query/Pipeline/SqlServerQuerySqlGenerator.cs b/src/EFCore.SqlServer/Query/Pipeline/SqlServerQuerySqlGenerator.cs new file mode 100644 index 00000000000..48317a054ea --- /dev/null +++ b/src/EFCore.SqlServer/Query/Pipeline/SqlServerQuerySqlGenerator.cs @@ -0,0 +1,55 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.EntityFrameworkCore.Relational.Query.Pipeline; +using Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.SqlExpressions; +using Microsoft.EntityFrameworkCore.Storage; + +namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Pipeline +{ + public class SqlServerQuerySqlGenerator : QuerySqlGenerator + { + public SqlServerQuerySqlGenerator( + IRelationalCommandBuilderFactory relationalCommandBuilderFactory, + ISqlGenerationHelper sqlGenerationHelper) + : base(relationalCommandBuilderFactory, sqlGenerationHelper) + { + } + + protected override void GenerateTop(SelectExpression selectExpression) + { + if (selectExpression.Limit != null + && selectExpression.Offset == null) + { + Sql.Append("TOP("); + + Visit(selectExpression.Limit); + + Sql.Append(") "); + } + } + + protected override void GenerateLimitOffset(SelectExpression selectExpression) + { + // Note: For Limit without Offset, SqlServer generates TOP() + if (selectExpression.Offset != null) + { + Sql.AppendLine() + .Append("OFFSET "); + + Visit(selectExpression.Offset); + + Sql.Append(" ROWS"); + + if (selectExpression.Limit != null) + { + Sql.Append(" FETCH NEXT "); + + Visit(selectExpression.Limit); + + Sql.Append(" ROWS ONLY"); + } + } + } + } +} diff --git a/src/EFCore.SqlServer/Query/Pipeline/SqlServerQuerySqlGeneratorFactory.cs b/src/EFCore.SqlServer/Query/Pipeline/SqlServerQuerySqlGeneratorFactory.cs new file mode 100644 index 00000000000..44efbb8cbd6 --- /dev/null +++ b/src/EFCore.SqlServer/Query/Pipeline/SqlServerQuerySqlGeneratorFactory.cs @@ -0,0 +1,26 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.EntityFrameworkCore.Relational.Query.Pipeline; +using Microsoft.EntityFrameworkCore.Storage; + +namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Pipeline +{ + public class SqlServerQuerySqlGeneratorFactory : QuerySqlGeneratorFactory + { + private readonly IRelationalCommandBuilderFactory _commandBuilderFactory; + private readonly ISqlGenerationHelper _sqlGenerationHelper; + + public SqlServerQuerySqlGeneratorFactory( + IRelationalCommandBuilderFactory commandBuilderFactory, + ISqlGenerationHelper sqlGenerationHelper) + : base(commandBuilderFactory, sqlGenerationHelper) + { + _commandBuilderFactory = commandBuilderFactory; + _sqlGenerationHelper = sqlGenerationHelper; + } + + public override QuerySqlGenerator Create() + => new SqlServerQuerySqlGenerator(_commandBuilderFactory, _sqlGenerationHelper); + } +} diff --git a/src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGenerator.cs b/src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGenerator.cs index 48254efe95e..f92b25c3c8a 100644 --- a/src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGenerator.cs +++ b/src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGenerator.cs @@ -24,11 +24,6 @@ protected override string GenerateOperator(SqlBinaryExpression binaryExpression) ? " || " : base.GenerateOperator(binaryExpression); - protected override void GenerateTop(SelectExpression selectExpression) - { - // Handled by GenerateLimitOffset - } - protected override void GenerateLimitOffset(SelectExpression selectExpression) { Check.NotNull(selectExpression, nameof(selectExpression)); diff --git a/src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGeneratorFactory2.cs b/src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGeneratorFactory.cs similarity index 89% rename from src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGeneratorFactory2.cs rename to src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGeneratorFactory.cs index 27af19bc142..f407f33d0c6 100644 --- a/src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGeneratorFactory2.cs +++ b/src/EFCore.Sqlite.Core/Query/Pipeline/SqliteQuerySqlGeneratorFactory.cs @@ -21,8 +21,6 @@ public SqliteQuerySqlGeneratorFactory( } public override QuerySqlGenerator Create() - { - return new SqliteQuerySqlGenerator(_commandBuilderFactory, _sqlGenerationHelper); - } + => new SqliteQuerySqlGenerator(_commandBuilderFactory, _sqlGenerationHelper); } }