From 38a943ed1b7cecfcb4e93cb6b02cc2e325814af8 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Mon, 31 Mar 2025 12:03:56 +0100 Subject: [PATCH 01/13] When using `LIMIT`/`OFFSET` without ordering try to order using projection rather than with the primary key --- CHANGELOG.md | 1 + lib/arel/visitors/sqlserver.rb | 44 ++++++++++++++---- test/cases/order_test_sqlserver.rb | 71 ++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63173aedb..8bca8611d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ #### Fixed - [#1318](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1318) Reverse order of values when upserting +- []() When using `LIMIT`/`OFFSET` without ordering try to order using projection rather than with the primary key. ## v8.0.5 diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index 5890b7fc3..d67189a9f 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -252,7 +252,11 @@ def visit_Arel_Nodes_SelectStatement_SQLServer_Lock(collector, options = {}) collector end + # AIDO def visit_Orders_And_Let_Fetch_Happen(o, collector) + + # binding.pry if $DEBUG + make_Fetch_Possible_And_Deterministic o if o.orders.any? collector << " ORDER BY " @@ -300,24 +304,48 @@ def select_statement_lock? @select_statement && @select_statement.lock end + # If LIMIT/OFFSET is used without ORDER BY, SQLServer will return an error. + # This method will add a deterministic ORDER BY clause to the query using following rules: + # 1. If the query has projections, use the first projection as the ORDER BY clause. + # 2. If the query has SQL literal projection, use the first part of the SQL literal as the ORDER BY clause. + # 3. If the query has a table with a primary key, use the primary key as the ORDER BY clause. def make_Fetch_Possible_And_Deterministic(o) return if o.limit.nil? && o.offset.nil? return if o.orders.any? - t = table_From_Statement o - pk = primary_Key_From_Table t - return unless pk + # TODO: Refactor to list all projections and then find the first one that looks good. + + projection = o.cores.first.projections.first + + + binding.pry if $DEBUG + + + if projection.is_a?(Arel::Attributes::Attribute) && !projection.name.include?("*") + o.orders = [projection.asc] - # Prefer deterministic vs a simple `(SELECT NULL)` expr. - o.orders = [pk.asc] + # TODO: Use better logic to find first projection that is usable for ordering. + elsif projection.is_a?(Arel::Nodes::SqlLiteral) && !projection.match?(/^\s*(1 as ONE|\*)(\s|,)*/i) + + first_projection = Arel::Nodes::SqlLiteral.new(projection.split(",").first.split(/\sAS\s/i).first) + o.orders = [first_projection.asc] + else + + pk = primary_Key_From_Table(table_From_Statement(o)) + o.orders = [pk.asc] if pk + end + + # rescue => e + # binding.pry end def distinct_One_As_One_Is_So_Not_Fetch(o) core = o.cores.first distinct = Nodes::Distinct === core.set_quantifier - oneasone = core.projections.all? { |x| x == ActiveRecord::FinderMethods::ONE_AS_ONE } - limitone = [nil, 0, 1].include? node_value(o.limit) - if distinct && oneasone && limitone && !o.offset + one_as_one = core.projections.all? { |x| x == ActiveRecord::FinderMethods::ONE_AS_ONE } + limit_one = [nil, 0, 1].include? node_value(o.limit) + + if distinct && one_as_one && limit_one && !o.offset core.projections = [Arel.sql("TOP(1) 1 AS [one]")] o.limit = nil end diff --git a/test/cases/order_test_sqlserver.rb b/test/cases/order_test_sqlserver.rb index 82f43d539..795cc925d 100644 --- a/test/cases/order_test_sqlserver.rb +++ b/test/cases/order_test_sqlserver.rb @@ -150,4 +150,75 @@ class OrderTestSQLServer < ActiveRecord::TestCase sql = Post.order(:id).order("posts.id ASC").to_sql assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC, posts.id ASC", sql end + + describe "simple query containing limit" do + it "order by primary key if no projections" do + $DEBUG = false + + sql = Post.limit(5).to_sql + + assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + + $DEBUG = false + end + + it "use order provided" do + # $DEBUG = true + + sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql + + assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + + # binding.pry + + end + + it "order by first projection if no order provided" do + # $DEBUG = true + + sql = Post.select(:legacy_comments_count).limit(5).to_sql + + assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + + # binding.pry + + end + + it "order by first projection (when multiple projections) if no order provided" do + sql = Post.select(:legacy_comments_count, :tags_count).limit(5).to_sql + + assert_equal "SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + end + end + + describe "query containing FROM and limit" do + it "uses the provided orderings" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [11, 5, 1] + end + end + # + it "in the subquery the first projection is used for ordering if none provided" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + # binding.pry + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [0, 5, 0] + end + end + + it "in the subquery the primary key is used for ordering if none provided" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [10, 5, 0] + end + end + end end From f739a7b5ec2b1248744c195a75fd5f1cdf1fcb7f Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Tue, 1 Apr 2025 15:29:50 +0100 Subject: [PATCH 02/13] Fix --- lib/arel/visitors/sqlserver.rb | 59 +++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index d67189a9f..2724cefa2 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -9,6 +9,8 @@ class SQLServer < Arel::Visitors::ToSql FETCH0 = " FETCH FIRST (SELECT 0) " ROWS_ONLY = " ROWS ONLY" + ONE_AS_ONE = ActiveRecord::FinderMethods::ONE_AS_ONE + private # SQLServer ToSql/Visitor (Overrides) @@ -251,12 +253,8 @@ def visit_Arel_Nodes_SelectStatement_SQLServer_Lock(collector, options = {}) end collector end - - # AIDO + def visit_Orders_And_Let_Fetch_Happen(o, collector) - - # binding.pry if $DEBUG - make_Fetch_Possible_And_Deterministic o if o.orders.any? collector << " ORDER BY " @@ -304,8 +302,7 @@ def select_statement_lock? @select_statement && @select_statement.lock end - # If LIMIT/OFFSET is used without ORDER BY, SQLServer will return an error. - # This method will add a deterministic ORDER BY clause to the query using following rules: + # LIMIT/OFFSET cannot be used without an ORDER. This method adds a deterministic ORDER using following rules: # 1. If the query has projections, use the first projection as the ORDER BY clause. # 2. If the query has SQL literal projection, use the first part of the SQL literal as the ORDER BY clause. # 3. If the query has a table with a primary key, use the primary key as the ORDER BY clause. @@ -313,36 +310,46 @@ def make_Fetch_Possible_And_Deterministic(o) return if o.limit.nil? && o.offset.nil? return if o.orders.any? - # TODO: Refactor to list all projections and then find the first one that looks good. - - projection = o.cores.first.projections.first - - - binding.pry if $DEBUG - - - if projection.is_a?(Arel::Attributes::Attribute) && !projection.name.include?("*") + if (projection = projection_to_order_by_for_fetch(o)) o.orders = [projection.asc] - - # TODO: Use better logic to find first projection that is usable for ordering. - elsif projection.is_a?(Arel::Nodes::SqlLiteral) && !projection.match?(/^\s*(1 as ONE|\*)(\s|,)*/i) - - first_projection = Arel::Nodes::SqlLiteral.new(projection.split(",").first.split(/\sAS\s/i).first) - o.orders = [first_projection.asc] else - pk = primary_Key_From_Table(table_From_Statement(o)) o.orders = [pk.asc] if pk end + end + + def projection_to_order_by_for_fetch(o) + o.cores.first.projections.each do |projection| + case projection + when Arel::Attributes::Attribute + return projection unless projection.name.include?("*") + when Arel::Nodes::SqlLiteral + projection.split(",").each do |p| + next if p.match?(/#{Regexp.escape(ONE_AS_ONE)}/i) || p.include?("*") + + return Arel::Nodes::SqlLiteral.new(remove_last_AS_from_projection(p)) + end + end + end + + nil + end - # rescue => e - # binding.pry + # Remove last AS from projection that could contain multiple AS clauses. + # Examples: + # - 'name' + # - 'name AS first_name' + # - 'AVG(accounts.credit_limit AS DECIMAL) AS avg_credit_limit)' + def remove_last_AS_from_projection(projection) + parts = projection.split(/\sAS\s/i) + parts.pop if parts.length > 1 + projection.join(" AS ") end def distinct_One_As_One_Is_So_Not_Fetch(o) core = o.cores.first distinct = Nodes::Distinct === core.set_quantifier - one_as_one = core.projections.all? { |x| x == ActiveRecord::FinderMethods::ONE_AS_ONE } + one_as_one = core.projections.all? { |x| x == ONE_AS_ONE } limit_one = [nil, 0, 1].include? node_value(o.limit) if distinct && one_as_one && limit_one && !o.offset From f09d2192c7a8fae8e42c96950a6a615706b783cf Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Tue, 1 Apr 2025 15:31:46 +0100 Subject: [PATCH 03/13] Cleanup --- lib/arel/visitors/sqlserver.rb | 2 +- test/cases/order_test_sqlserver.rb | 18 +----------------- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index 2724cefa2..f47afb405 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -253,7 +253,7 @@ def visit_Arel_Nodes_SelectStatement_SQLServer_Lock(collector, options = {}) end collector end - + def visit_Orders_And_Let_Fetch_Happen(o, collector) make_Fetch_Possible_And_Deterministic o if o.orders.any? diff --git a/test/cases/order_test_sqlserver.rb b/test/cases/order_test_sqlserver.rb index 795cc925d..bd59e586d 100644 --- a/test/cases/order_test_sqlserver.rb +++ b/test/cases/order_test_sqlserver.rb @@ -153,35 +153,21 @@ class OrderTestSQLServer < ActiveRecord::TestCase describe "simple query containing limit" do it "order by primary key if no projections" do - $DEBUG = false - sql = Post.limit(5).to_sql assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - - $DEBUG = false end it "use order provided" do - # $DEBUG = true - sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - - # binding.pry - end it "order by first projection if no order provided" do - # $DEBUG = true - sql = Post.select(:legacy_comments_count).limit(5).to_sql assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - - # binding.pry - end it "order by first projection (when multiple projections) if no order provided" do @@ -200,12 +186,10 @@ class OrderTestSQLServer < ActiveRecord::TestCase assert_equal result, [11, 5, 1] end end - # + it "in the subquery the first projection is used for ordering if none provided" do sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - # binding.pry - assert_queries_match(/#{Regexp.escape(sql)}/) do result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) assert_equal result, [0, 5, 0] From ba39216e93e862230c63a8c209d7cabe084fb9bd Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Tue, 1 Apr 2025 17:04:28 +0100 Subject: [PATCH 04/13] Typo --- lib/arel/visitors/sqlserver.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index f47afb405..868befbbc 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -343,7 +343,7 @@ def projection_to_order_by_for_fetch(o) def remove_last_AS_from_projection(projection) parts = projection.split(/\sAS\s/i) parts.pop if parts.length > 1 - projection.join(" AS ") + parts.join(" AS ") end def distinct_One_As_One_Is_So_Not_Fetch(o) From 3cf43eadd26f4747627f170c3d65c5ea7135d92d Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Tue, 1 Apr 2025 19:18:56 +0100 Subject: [PATCH 05/13] Cleanup --- CHANGELOG.md | 2 +- lib/arel/visitors/sqlserver.rb | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b7a315aa..a774d2d6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ - [#1318](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1318) Reverse order of values when upserting - [#1321](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1321) Fix SQL statement to calculate `updated_at` when upserting -- []() When using `LIMIT`/`OFFSET` without ordering try to order using projection rather than with the primary key. +- [1322](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1322) When using `FETCH` without ordering try to order using projection rather than the primary key. ## v8.0.5 diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index 868befbbc..93e25875c 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -302,10 +302,8 @@ def select_statement_lock? @select_statement && @select_statement.lock end - # LIMIT/OFFSET cannot be used without an ORDER. This method adds a deterministic ORDER using following rules: - # 1. If the query has projections, use the first projection as the ORDER BY clause. - # 2. If the query has SQL literal projection, use the first part of the SQL literal as the ORDER BY clause. - # 3. If the query has a table with a primary key, use the primary key as the ORDER BY clause. + # FETCH cannot be used without an order. If an order is not given then try to use the projections for the ordering. + # If no suitable projection are present then fallback to using the primary key of the table. def make_Fetch_Possible_And_Deterministic(o) return if o.limit.nil? && o.offset.nil? return if o.orders.any? @@ -318,6 +316,8 @@ def make_Fetch_Possible_And_Deterministic(o) end end + # Find the first projection or part of projection that can be used for ordering. Cannot use + # projections with '*' or '1 AS one' in them. def projection_to_order_by_for_fetch(o) o.cores.first.projections.each do |projection| case projection @@ -335,8 +335,7 @@ def projection_to_order_by_for_fetch(o) nil end - # Remove last AS from projection that could contain multiple AS clauses. - # Examples: + # Remove last AS from projection. Example projections: # - 'name' # - 'name AS first_name' # - 'AVG(accounts.credit_limit AS DECIMAL) AS avg_credit_limit)' From a364e3cfa942fbfa42e20e602ef33863fbeb3e4d Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Thu, 3 Apr 2025 16:29:04 +0100 Subject: [PATCH 06/13] WIP --- lib/arel/visitors/sqlserver.rb | 10 +- test/cases/order_test_sqlserver.rb | 427 +++++++++++++++++------------ 2 files changed, 256 insertions(+), 181 deletions(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index 93e25875c..c9a7b8783 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -305,10 +305,14 @@ def select_statement_lock? # FETCH cannot be used without an order. If an order is not given then try to use the projections for the ordering. # If no suitable projection are present then fallback to using the primary key of the table. def make_Fetch_Possible_And_Deterministic(o) + binding.pry if $DEBUG + return if o.limit.nil? && o.offset.nil? return if o.orders.any? - if (projection = projection_to_order_by_for_fetch(o)) + + + if any_groupings?(o) && (projection = projection_to_order_by_for_fetch(o)) o.orders = [projection.asc] else pk = primary_Key_From_Table(table_From_Statement(o)) @@ -316,6 +320,10 @@ def make_Fetch_Possible_And_Deterministic(o) end end + def any_groupings?(o) + o.cores.any? { |core| core.groups.present? } + end + # Find the first projection or part of projection that can be used for ordering. Cannot use # projections with '*' or '1 AS one' in them. def projection_to_order_by_for_fetch(o) diff --git a/test/cases/order_test_sqlserver.rb b/test/cases/order_test_sqlserver.rb index bd59e586d..d29ec7147 100644 --- a/test/cases/order_test_sqlserver.rb +++ b/test/cases/order_test_sqlserver.rb @@ -6,203 +6,270 @@ class OrderTestSQLServer < ActiveRecord::TestCase fixtures :posts - it "not mangel complex order clauses" do - xyz_order = "CASE WHEN [title] LIKE N'XYZ%' THEN 0 ELSE 1 END" - xyz_post = Post.create title: "XYZ Post", body: "Test cased orders." - assert_equal xyz_post, Post.order(Arel.sql(xyz_order)).first - end - - it "support column" do - order = "title" - post1 = Post.create title: "AAA Post", body: "Test cased orders." - assert_equal post1, Post.order(order).first - end - - it "support column ASC" do - order = "title ASC" - post1 = Post.create title: "AAA Post", body: "Test cased orders." - assert_equal post1, Post.order(order).first - end - - it "support column DESC" do - order = "title DESC" - post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - assert_equal post1, Post.order(order).first - end - it "support column as symbol" do - order = :title - post1 = Post.create title: "AAA Post", body: "Test cased orders." - assert_equal post1, Post.order(order).first - end - - it "support table and column" do - order = "posts.title" - post1 = Post.create title: "AAA Post", body: "Test cased orders." - assert_equal post1, Post.order(order).first - end - - it "support quoted column" do - order = "[title]" - post1 = Post.create title: "AAA Post", body: "Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - end - - it "support quoted table and column" do - order = "[posts].[title]" - post1 = Post.create title: "AAA Post", body: "Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - end - it "support primary: column, secondary: column" do - order = "title DESC, body" - post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." - assert_equal post1, Post.order(order).first - assert_equal post2, Post.order(order).second - end - - it "support primary: table and column, secondary: column" do - order = "posts.title DESC, body" - post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." - assert_equal post1, Post.order(order).first - assert_equal post2, Post.order(order).second - end - - it "support primary: case expression, secondary: column" do - order = "(CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END) DESC, body" - post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - assert_equal post2, Post.order(Arel.sql(order)).second - end - - it "support primary: quoted table and column, secondary: case expresion" do - order = "[posts].[body] DESC, (CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END) DESC" - post1 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." - post2 = Post.create title: "ZZY Post", body: "ZZZ Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - assert_equal post2, Post.order(Arel.sql(order)).second - end - - it "support inline function" do - order = "LEN(title)" - post1 = Post.create title: "A", body: "AAA Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - end - - it "support inline function with parameters" do - order = "SUBSTRING(title, 1, 3)" - post1 = Post.create title: "AAA Post", body: "Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - end - - it "support inline function with parameters DESC" do - order = "SUBSTRING(title, 1, 3) DESC" - post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - end + # it "not mangel complex order clauses" do + # xyz_order = "CASE WHEN [title] LIKE N'XYZ%' THEN 0 ELSE 1 END" + # xyz_post = Post.create title: "XYZ Post", body: "Test cased orders." + # assert_equal xyz_post, Post.order(Arel.sql(xyz_order)).first + # end + # + # it "support column" do + # order = "title" + # post1 = Post.create title: "AAA Post", body: "Test cased orders." + # assert_equal post1, Post.order(order).first + # end + # + # it "support column ASC" do + # order = "title ASC" + # post1 = Post.create title: "AAA Post", body: "Test cased orders." + # assert_equal post1, Post.order(order).first + # end + # + # it "support column DESC" do + # order = "title DESC" + # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + # assert_equal post1, Post.order(order).first + # end + # + # it "support column as symbol" do + # order = :title + # post1 = Post.create title: "AAA Post", body: "Test cased orders." + # assert_equal post1, Post.order(order).first + # end + # + # it "support table and column" do + # order = "posts.title" + # post1 = Post.create title: "AAA Post", body: "Test cased orders." + # assert_equal post1, Post.order(order).first + # end + # + # it "support quoted column" do + # order = "[title]" + # post1 = Post.create title: "AAA Post", body: "Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # end + # + # it "support quoted table and column" do + # order = "[posts].[title]" + # post1 = Post.create title: "AAA Post", body: "Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # end + # + # it "support primary: column, secondary: column" do + # order = "title DESC, body" + # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + # post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." + # assert_equal post1, Post.order(order).first + # assert_equal post2, Post.order(order).second + # end + # + # it "support primary: table and column, secondary: column" do + # order = "posts.title DESC, body" + # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + # post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." + # assert_equal post1, Post.order(order).first + # assert_equal post2, Post.order(order).second + # end + # + # it "support primary: case expression, secondary: column" do + # order = "(CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END) DESC, body" + # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + # post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # assert_equal post2, Post.order(Arel.sql(order)).second + # end + # + # it "support primary: quoted table and column, secondary: case expresion" do + # order = "[posts].[body] DESC, (CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END) DESC" + # post1 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." + # post2 = Post.create title: "ZZY Post", body: "ZZZ Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # assert_equal post2, Post.order(Arel.sql(order)).second + # end + # + # it "support inline function" do + # order = "LEN(title)" + # post1 = Post.create title: "A", body: "AAA Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # end + # + # it "support inline function with parameters" do + # order = "SUBSTRING(title, 1, 3)" + # post1 = Post.create title: "AAA Post", body: "Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # end + # + # it "support inline function with parameters DESC" do + # order = "SUBSTRING(title, 1, 3) DESC" + # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # end + # + # it "support primary: inline function, secondary: column" do + # order = "LEN(title), body" + # post1 = Post.create title: "A", body: "AAA Test cased orders." + # post2 = Post.create title: "A", body: "Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # assert_equal post2, Post.order(Arel.sql(order)).second + # end + # + # it "support primary: inline function, secondary: column with direction" do + # order = "LEN(title) ASC, body DESC" + # post1 = Post.create title: "A", body: "ZZZ Test cased orders." + # post2 = Post.create title: "A", body: "Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # assert_equal post2, Post.order(Arel.sql(order)).second + # end + # + # it "support primary: column, secondary: inline function" do + # order = "body DESC, LEN(title)" + # post1 = Post.create title: "Post", body: "ZZZ Test cased orders." + # post2 = Post.create title: "Longer Post", body: "ZZZ Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # assert_equal post2, Post.order(Arel.sql(order)).second + # end + # + # it "support primary: case expression, secondary: inline function" do + # order = "CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END DESC, LEN(body) ASC" + # post1 = Post.create title: "ZZZ Post", body: "Z" + # post2 = Post.create title: "ZZZ Post", body: "Test cased orders." + # assert_equal post1, Post.order(Arel.sql(order)).first + # assert_equal post2, Post.order(Arel.sql(order)).second + # end + # + # it "support primary: inline function, secondary: case expression" do + # order = "LEN(body), CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END DESC" + # post1 = Post.create title: "ZZZ Post", body: "Z" + # post2 = Post.create title: "Post", body: "Z" + # assert_equal post1, Post.order(Arel.sql(order)).first + # assert_equal post2, Post.order(Arel.sql(order)).second + # end + # + # # Executing this kind of queries will raise "A column has been specified more than once in the order by list" + # # This test shows that we don't do anything to prevent this + # it "doesn't deduplicate semantically equal orders" do + # sql = Post.order(:id).order("posts.id ASC").to_sql + # assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC, posts.id ASC", sql + # end + + + describe "grouping queries" do + + # it "order by primary key by default if not a grouping query" do + # + # $DEBUG = true + # + # assert_queries_match(/ORDER BY \[posts\]\.\[id\] ASC/i) do + # Post.pick(:title) + # end + # end + + + it "ordering not required if not using FETCH" do + skip + + assert_queries_match(/^#{Regexp.escape("SELECT count(*) FROM [posts] GROUP BY [posts].[title]")}$/i) do + Post.group(:title).select("count(*)").load + end + end - it "support primary: inline function, secondary: column" do - order = "LEN(title), body" - post1 = Post.create title: "A", body: "AAA Test cased orders." - post2 = Post.create title: "A", body: "Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - assert_equal post2, Post.order(Arel.sql(order)).second - end + it "error thrown if using FETCH without ordering and column not in select" do + skip - it "support primary: inline function, secondary: column with direction" do - order = "LEN(title) ASC, body DESC" - post1 = Post.create title: "A", body: "ZZZ Test cased orders." - post2 = Post.create title: "A", body: "Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - assert_equal post2, Post.order(Arel.sql(order)).second - end - - it "support primary: column, secondary: inline function" do - order = "body DESC, LEN(title)" - post1 = Post.create title: "Post", body: "ZZZ Test cased orders." - post2 = Post.create title: "Longer Post", body: "ZZZ Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - assert_equal post2, Post.order(Arel.sql(order)).second - end + error = assert_raises(ActiveRecord::StatementInvalid) do + Post.select(:title, "count(*)").group(:title).first(2) + end + assert_match(/Column "posts\.id" is invalid in the ORDER BY clause because it is not contained in either an aggregate function or the GROUP BY clause/, error.message) + end - it "support primary: case expression, secondary: inline function" do - order = "CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END DESC, LEN(body) ASC" - post1 = Post.create title: "ZZZ Post", body: "Z" - post2 = Post.create title: "ZZZ Post", body: "Test cased orders." - assert_equal post1, Post.order(Arel.sql(order)).first - assert_equal post2, Post.order(Arel.sql(order)).second - end + it "ordering required if using FETCH" do - it "support primary: inline function, secondary: case expression" do - order = "LEN(body), CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END DESC" - post1 = Post.create title: "ZZZ Post", body: "Z" - post2 = Post.create title: "Post", body: "Z" - assert_equal post1, Post.order(Arel.sql(order)).first - assert_equal post2, Post.order(Arel.sql(order)).second - end + ActiveSupport::Notifications.subscribe('sql.active_record') do |_name, _start, _finish, _id, payload| + puts payload[:sql] + end - # Executing this kind of queries will raise "A column has been specified more than once in the order by list" - # This test shows that we don't do anything to prevent this - it "doesn't deduplicate semantically equal orders" do - sql = Post.order(:id).order("posts.id ASC").to_sql - assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC, posts.id ASC", sql - end + # $DEBUG = true - describe "simple query containing limit" do - it "order by primary key if no projections" do - sql = Post.limit(5).to_sql + # TODO: failing when it should not fail. The ordering should be taken from the projection. - assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - end + assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY")}/i) do + Post.select(:title, "count(*)").group(:title).first(2)#.load + end - it "use order provided" do - sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql + # Post.group(:title).select("count(*)").first(2).load - assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + # assert_match(/Column "posts\.id" is invalid in the ORDER BY clause because it is not contained in either an aggregate function or the GROUP BY clause/, error.message) end - it "order by first projection if no order provided" do - sql = Post.select(:legacy_comments_count).limit(5).to_sql - assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - end - - it "order by first projection (when multiple projections) if no order provided" do - sql = Post.select(:legacy_comments_count, :tags_count).limit(5).to_sql + # + # it "order by primary key by default if not a grouping query" do + # $DEBUG = true + # + # ActiveSupport::Notifications.subscribe('sql.active_record') do |_name, _start, _finish, _id, payload| + # puts payload[:sql] + # end + # + # Post.group(:title).select("count(*)").first(2).load + # end - assert_equal "SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - end end - describe "query containing FROM and limit" do - it "uses the provided orderings" do - sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - assert_queries_match(/#{Regexp.escape(sql)}/) do - result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - assert_equal result, [11, 5, 1] - end - end - - it "in the subquery the first projection is used for ordering if none provided" do - sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - assert_queries_match(/#{Regexp.escape(sql)}/) do - result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - assert_equal result, [0, 5, 0] - end - end - - it "in the subquery the primary key is used for ordering if none provided" do - sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - - assert_queries_match(/#{Regexp.escape(sql)}/) do - result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - assert_equal result, [10, 5, 0] - end - end - end + # describe "simple query containing limit" do + # it "order by primary key if no projections" do + # sql = Post.limit(5).to_sql + # + # assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + # end + # + # it "use order provided" do + # sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql + # + # assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + # end + # + # it "order by first projection if no order provided" do + # sql = Post.select(:legacy_comments_count).limit(5).to_sql + # + # assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + # end + # + # it "order by first projection (when multiple projections) if no order provided" do + # sql = Post.select(:legacy_comments_count, :tags_count).limit(5).to_sql + # + # assert_equal "SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + # end + # end + # + # describe "query containing FROM and limit" do + # it "uses the provided orderings" do + # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + # + # assert_queries_match(/#{Regexp.escape(sql)}/) do + # result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + # assert_equal result, [11, 5, 1] + # end + # end + # + # it "in the subquery the first projection is used for ordering if none provided" do + # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + # + # assert_queries_match(/#{Regexp.escape(sql)}/) do + # result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + # assert_equal result, [0, 5, 0] + # end + # end + # + # it "in the subquery the primary key is used for ordering if none provided" do + # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + # + # assert_queries_match(/#{Regexp.escape(sql)}/) do + # result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + # assert_equal result, [10, 5, 0] + # end + # end + # end end From 3e3678525aceb1dc85100eb6534d8d2e7d67d3d8 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Thu, 1 May 2025 11:49:35 +0100 Subject: [PATCH 07/13] WIP --- lib/arel/visitors/sqlserver.rb | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index c9a7b8783..51269045c 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -305,14 +305,14 @@ def select_statement_lock? # FETCH cannot be used without an order. If an order is not given then try to use the projections for the ordering. # If no suitable projection are present then fallback to using the primary key of the table. def make_Fetch_Possible_And_Deterministic(o) - binding.pry if $DEBUG + # binding.pry if $DEBUG return if o.limit.nil? && o.offset.nil? return if o.orders.any? - if any_groupings?(o) && (projection = projection_to_order_by_for_fetch(o)) + if (any_groupings?(o) || has_join_sources?(o)) && (projection = projection_to_order_by_for_fetch(o)) o.orders = [projection.asc] else pk = primary_Key_From_Table(table_From_Statement(o)) @@ -324,6 +324,19 @@ def any_groupings?(o) o.cores.any? { |core| core.groups.present? } end + # TODO: Need this for "in the subquery the first projection is used for ordering if none provided" test. + def has_join_sources?(o) + # binding.pry if $DEBUG + + + return false unless o.is_a?(Arel::Nodes::SelectStatement) + + # false + o.cores.any? { |core| core.source.is_a?(Arel::Nodes::JoinSource) } + rescue => e + binding.pry + end + # Find the first projection or part of projection that can be used for ordering. Cannot use # projections with '*' or '1 AS one' in them. def projection_to_order_by_for_fetch(o) From 3d9cf2493038ab9c4fa80aebf5d3ee744c45c536 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Thu, 1 May 2025 13:52:17 +0100 Subject: [PATCH 08/13] Fix method name clash --- lib/arel/visitors/sqlserver.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index 51269045c..ac4960657 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -312,7 +312,7 @@ def make_Fetch_Possible_And_Deterministic(o) - if (any_groupings?(o) || has_join_sources?(o)) && (projection = projection_to_order_by_for_fetch(o)) + if (any_groupings?(o) || xxx_has_join_sources?(o)) && (projection = projection_to_order_by_for_fetch(o)) o.orders = [projection.asc] else pk = primary_Key_From_Table(table_From_Statement(o)) @@ -325,7 +325,8 @@ def any_groupings?(o) end # TODO: Need this for "in the subquery the first projection is used for ordering if none provided" test. - def has_join_sources?(o) + # TODO: rename + def xxx_has_join_sources?(o) # binding.pry if $DEBUG @@ -333,8 +334,8 @@ def has_join_sources?(o) # false o.cores.any? { |core| core.source.is_a?(Arel::Nodes::JoinSource) } - rescue => e - binding.pry + # rescue => e + # binding.pry end # Find the first projection or part of projection that can be used for ordering. Cannot use From 911ea0ce98042076f0e926227c6aeead0a2f1a36 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Thu, 1 May 2025 13:53:49 +0100 Subject: [PATCH 09/13] Tests all passing on adapter and solid_cache --- test/cases/order_test_sqlserver.rb | 409 ++++++++++++++--------------- 1 file changed, 204 insertions(+), 205 deletions(-) diff --git a/test/cases/order_test_sqlserver.rb b/test/cases/order_test_sqlserver.rb index d29ec7147..3eab28d4a 100644 --- a/test/cases/order_test_sqlserver.rb +++ b/test/cases/order_test_sqlserver.rb @@ -8,150 +8,150 @@ class OrderTestSQLServer < ActiveRecord::TestCase - # it "not mangel complex order clauses" do - # xyz_order = "CASE WHEN [title] LIKE N'XYZ%' THEN 0 ELSE 1 END" - # xyz_post = Post.create title: "XYZ Post", body: "Test cased orders." - # assert_equal xyz_post, Post.order(Arel.sql(xyz_order)).first - # end - # - # it "support column" do - # order = "title" - # post1 = Post.create title: "AAA Post", body: "Test cased orders." - # assert_equal post1, Post.order(order).first - # end - # - # it "support column ASC" do - # order = "title ASC" - # post1 = Post.create title: "AAA Post", body: "Test cased orders." - # assert_equal post1, Post.order(order).first - # end - # - # it "support column DESC" do - # order = "title DESC" - # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - # assert_equal post1, Post.order(order).first - # end - # - # it "support column as symbol" do - # order = :title - # post1 = Post.create title: "AAA Post", body: "Test cased orders." - # assert_equal post1, Post.order(order).first - # end - # - # it "support table and column" do - # order = "posts.title" - # post1 = Post.create title: "AAA Post", body: "Test cased orders." - # assert_equal post1, Post.order(order).first - # end - # - # it "support quoted column" do - # order = "[title]" - # post1 = Post.create title: "AAA Post", body: "Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # end - # - # it "support quoted table and column" do - # order = "[posts].[title]" - # post1 = Post.create title: "AAA Post", body: "Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # end - # - # it "support primary: column, secondary: column" do - # order = "title DESC, body" - # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - # post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." - # assert_equal post1, Post.order(order).first - # assert_equal post2, Post.order(order).second - # end - # - # it "support primary: table and column, secondary: column" do - # order = "posts.title DESC, body" - # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - # post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." - # assert_equal post1, Post.order(order).first - # assert_equal post2, Post.order(order).second - # end - # - # it "support primary: case expression, secondary: column" do - # order = "(CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END) DESC, body" - # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - # post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # assert_equal post2, Post.order(Arel.sql(order)).second - # end - # - # it "support primary: quoted table and column, secondary: case expresion" do - # order = "[posts].[body] DESC, (CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END) DESC" - # post1 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." - # post2 = Post.create title: "ZZY Post", body: "ZZZ Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # assert_equal post2, Post.order(Arel.sql(order)).second - # end - # - # it "support inline function" do - # order = "LEN(title)" - # post1 = Post.create title: "A", body: "AAA Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # end - # - # it "support inline function with parameters" do - # order = "SUBSTRING(title, 1, 3)" - # post1 = Post.create title: "AAA Post", body: "Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # end - # - # it "support inline function with parameters DESC" do - # order = "SUBSTRING(title, 1, 3) DESC" - # post1 = Post.create title: "ZZZ Post", body: "Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # end - # - # it "support primary: inline function, secondary: column" do - # order = "LEN(title), body" - # post1 = Post.create title: "A", body: "AAA Test cased orders." - # post2 = Post.create title: "A", body: "Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # assert_equal post2, Post.order(Arel.sql(order)).second - # end - # - # it "support primary: inline function, secondary: column with direction" do - # order = "LEN(title) ASC, body DESC" - # post1 = Post.create title: "A", body: "ZZZ Test cased orders." - # post2 = Post.create title: "A", body: "Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # assert_equal post2, Post.order(Arel.sql(order)).second - # end - # - # it "support primary: column, secondary: inline function" do - # order = "body DESC, LEN(title)" - # post1 = Post.create title: "Post", body: "ZZZ Test cased orders." - # post2 = Post.create title: "Longer Post", body: "ZZZ Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # assert_equal post2, Post.order(Arel.sql(order)).second - # end - # - # it "support primary: case expression, secondary: inline function" do - # order = "CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END DESC, LEN(body) ASC" - # post1 = Post.create title: "ZZZ Post", body: "Z" - # post2 = Post.create title: "ZZZ Post", body: "Test cased orders." - # assert_equal post1, Post.order(Arel.sql(order)).first - # assert_equal post2, Post.order(Arel.sql(order)).second - # end - # - # it "support primary: inline function, secondary: case expression" do - # order = "LEN(body), CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END DESC" - # post1 = Post.create title: "ZZZ Post", body: "Z" - # post2 = Post.create title: "Post", body: "Z" - # assert_equal post1, Post.order(Arel.sql(order)).first - # assert_equal post2, Post.order(Arel.sql(order)).second - # end - # - # # Executing this kind of queries will raise "A column has been specified more than once in the order by list" - # # This test shows that we don't do anything to prevent this - # it "doesn't deduplicate semantically equal orders" do - # sql = Post.order(:id).order("posts.id ASC").to_sql - # assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC, posts.id ASC", sql - # end + it "not mangel complex order clauses" do + xyz_order = "CASE WHEN [title] LIKE N'XYZ%' THEN 0 ELSE 1 END" + xyz_post = Post.create title: "XYZ Post", body: "Test cased orders." + assert_equal xyz_post, Post.order(Arel.sql(xyz_order)).first + end + + it "support column" do + order = "title" + post1 = Post.create title: "AAA Post", body: "Test cased orders." + assert_equal post1, Post.order(order).first + end + + it "support column ASC" do + order = "title ASC" + post1 = Post.create title: "AAA Post", body: "Test cased orders." + assert_equal post1, Post.order(order).first + end + + it "support column DESC" do + order = "title DESC" + post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + assert_equal post1, Post.order(order).first + end + + it "support column as symbol" do + order = :title + post1 = Post.create title: "AAA Post", body: "Test cased orders." + assert_equal post1, Post.order(order).first + end + + it "support table and column" do + order = "posts.title" + post1 = Post.create title: "AAA Post", body: "Test cased orders." + assert_equal post1, Post.order(order).first + end + + it "support quoted column" do + order = "[title]" + post1 = Post.create title: "AAA Post", body: "Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + end + + it "support quoted table and column" do + order = "[posts].[title]" + post1 = Post.create title: "AAA Post", body: "Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + end + + it "support primary: column, secondary: column" do + order = "title DESC, body" + post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." + assert_equal post1, Post.order(order).first + assert_equal post2, Post.order(order).second + end + + it "support primary: table and column, secondary: column" do + order = "posts.title DESC, body" + post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." + assert_equal post1, Post.order(order).first + assert_equal post2, Post.order(order).second + end + + it "support primary: case expression, secondary: column" do + order = "(CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END) DESC, body" + post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + post2 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + assert_equal post2, Post.order(Arel.sql(order)).second + end + + it "support primary: quoted table and column, secondary: case expresion" do + order = "[posts].[body] DESC, (CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END) DESC" + post1 = Post.create title: "ZZZ Post", body: "ZZZ Test cased orders." + post2 = Post.create title: "ZZY Post", body: "ZZZ Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + assert_equal post2, Post.order(Arel.sql(order)).second + end + + it "support inline function" do + order = "LEN(title)" + post1 = Post.create title: "A", body: "AAA Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + end + + it "support inline function with parameters" do + order = "SUBSTRING(title, 1, 3)" + post1 = Post.create title: "AAA Post", body: "Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + end + + it "support inline function with parameters DESC" do + order = "SUBSTRING(title, 1, 3) DESC" + post1 = Post.create title: "ZZZ Post", body: "Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + end + + it "support primary: inline function, secondary: column" do + order = "LEN(title), body" + post1 = Post.create title: "A", body: "AAA Test cased orders." + post2 = Post.create title: "A", body: "Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + assert_equal post2, Post.order(Arel.sql(order)).second + end + + it "support primary: inline function, secondary: column with direction" do + order = "LEN(title) ASC, body DESC" + post1 = Post.create title: "A", body: "ZZZ Test cased orders." + post2 = Post.create title: "A", body: "Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + assert_equal post2, Post.order(Arel.sql(order)).second + end + + it "support primary: column, secondary: inline function" do + order = "body DESC, LEN(title)" + post1 = Post.create title: "Post", body: "ZZZ Test cased orders." + post2 = Post.create title: "Longer Post", body: "ZZZ Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + assert_equal post2, Post.order(Arel.sql(order)).second + end + + it "support primary: case expression, secondary: inline function" do + order = "CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END DESC, LEN(body) ASC" + post1 = Post.create title: "ZZZ Post", body: "Z" + post2 = Post.create title: "ZZZ Post", body: "Test cased orders." + assert_equal post1, Post.order(Arel.sql(order)).first + assert_equal post2, Post.order(Arel.sql(order)).second + end + + it "support primary: inline function, secondary: case expression" do + order = "LEN(body), CASE WHEN [title] LIKE N'ZZZ%' THEN title ELSE '' END DESC" + post1 = Post.create title: "ZZZ Post", body: "Z" + post2 = Post.create title: "Post", body: "Z" + assert_equal post1, Post.order(Arel.sql(order)).first + assert_equal post2, Post.order(Arel.sql(order)).second + end + + # Executing this kind of queries will raise "A column has been specified more than once in the order by list" + # This test shows that we don't do anything to prevent this + it "doesn't deduplicate semantically equal orders" do + sql = Post.order(:id).order("posts.id ASC").to_sql + assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC, posts.id ASC", sql + end describe "grouping queries" do @@ -167,7 +167,7 @@ class OrderTestSQLServer < ActiveRecord::TestCase it "ordering not required if not using FETCH" do - skip + # skip assert_queries_match(/^#{Regexp.escape("SELECT count(*) FROM [posts] GROUP BY [posts].[title]")}$/i) do Post.group(:title).select("count(*)").load @@ -175,28 +175,33 @@ class OrderTestSQLServer < ActiveRecord::TestCase end it "error thrown if using FETCH without ordering and column not in select" do - skip + # skip error = assert_raises(ActiveRecord::StatementInvalid) do Post.select(:title, "count(*)").group(:title).first(2) end - assert_match(/Column "posts\.id" is invalid in the ORDER BY clause because it is not contained in either an aggregate function or the GROUP BY clause/, error.message) + assert_match(/Column "posts\.id" is invalid in the ORDER BY clause/, error.message) end it "ordering required if using FETCH" do - ActiveSupport::Notifications.subscribe('sql.active_record') do |_name, _start, _finish, _id, payload| - puts payload[:sql] - end + # ActiveSupport::Notifications.subscribe('sql.active_record') do |_name, _start, _finish, _id, payload| + # puts payload[:sql] + # end # $DEBUG = true # TODO: failing when it should not fail. The ordering should be taken from the projection. + # `first` calls `find_nth_with_limit` which adds ordering. + assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY")}/i) do - Post.select(:title, "count(*)").group(:title).first(2)#.load + # error = assert_raises(ActiveRecord::StatementInvalid) do + Post.select(:title, "count(*)").group(:title).order(:title).first(2) end + # assert_match(/Column "posts\.id" is invalid in the ORDER BY clause because/, error.message) + # Post.group(:title).select("count(*)").first(2).load # assert_match(/Column "posts\.id" is invalid in the ORDER BY clause because it is not contained in either an aggregate function or the GROUP BY clause/, error.message) @@ -218,58 +223,52 @@ class OrderTestSQLServer < ActiveRecord::TestCase - # describe "simple query containing limit" do - # it "order by primary key if no projections" do - # sql = Post.limit(5).to_sql - # - # assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - # end - # - # it "use order provided" do - # sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql - # - # assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - # end - # - # it "order by first projection if no order provided" do - # sql = Post.select(:legacy_comments_count).limit(5).to_sql - # - # assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - # end - # - # it "order by first projection (when multiple projections) if no order provided" do - # sql = Post.select(:legacy_comments_count, :tags_count).limit(5).to_sql - # - # assert_equal "SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - # end - # end - # - # describe "query containing FROM and limit" do - # it "uses the provided orderings" do - # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - # - # assert_queries_match(/#{Regexp.escape(sql)}/) do - # result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - # assert_equal result, [11, 5, 1] - # end - # end - # - # it "in the subquery the first projection is used for ordering if none provided" do - # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - # - # assert_queries_match(/#{Regexp.escape(sql)}/) do - # result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - # assert_equal result, [0, 5, 0] - # end - # end - # - # it "in the subquery the primary key is used for ordering if none provided" do - # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - # - # assert_queries_match(/#{Regexp.escape(sql)}/) do - # result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - # assert_equal result, [10, 5, 0] - # end - # end - # end + describe "simple query containing limit" do + it "order by primary key if no projections" do + sql = Post.limit(5).to_sql + + assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + end + + it "use order provided" do + sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql + + assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + end + + + + + end + + describe "query containing FROM and limit" do + it "uses the provided orderings" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [11, 5, 1] + end + end + + it "in the subquery the first projection is used for ordering if none provided" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + # $DEBUG = true + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [0, 5, 0] + end + end + + it "in the subquery the primary key is used for ordering if none provided" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [10, 5, 0] + end + end + end end From 72de6cf031204e48bdc9258199b28a46eadc01fb Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Thu, 1 May 2025 20:37:49 +0100 Subject: [PATCH 10/13] Update order_test_sqlserver.rb --- test/cases/order_test_sqlserver.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cases/order_test_sqlserver.rb b/test/cases/order_test_sqlserver.rb index 3eab28d4a..3dabfa3fb 100644 --- a/test/cases/order_test_sqlserver.rb +++ b/test/cases/order_test_sqlserver.rb @@ -252,13 +252,13 @@ class OrderTestSQLServer < ActiveRecord::TestCase end it "in the subquery the first projection is used for ordering if none provided" do - sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" # $DEBUG = true assert_queries_match(/#{Regexp.escape(sql)}/) do result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - assert_equal result, [0, 5, 0] + assert_equal result, [10, 5, 0] end end From 10f1d3be84375dc7bb12a645eb5dbf5e8cd1446f Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Fri, 2 May 2025 10:46:13 +0100 Subject: [PATCH 11/13] WIP --- lib/arel/visitors/sqlserver.rb | 39 ++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index ac4960657..faf061afc 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -310,33 +310,36 @@ def make_Fetch_Possible_And_Deterministic(o) return if o.limit.nil? && o.offset.nil? return if o.orders.any? - - - if (any_groupings?(o) || xxx_has_join_sources?(o)) && (projection = projection_to_order_by_for_fetch(o)) - o.orders = [projection.asc] - else - pk = primary_Key_From_Table(table_From_Statement(o)) - o.orders = [pk.asc] if pk + if any_groupings?(o) || has_non_table_join_sources?(o) + if projection = projection_to_order_by_for_fetch(o) + o.orders = [projection.asc] + return + end end + + pk = primary_Key_From_Table(table_From_Statement(o)) + o.orders = [pk.asc] if pk end def any_groupings?(o) o.cores.any? { |core| core.groups.present? } end + def has_non_table_join_sources?(o) + o.cores.none? { |core| core.source.left.is_a?(Arel::Table) } + end + # TODO: Need this for "in the subquery the first projection is used for ordering if none provided" test. # TODO: rename - def xxx_has_join_sources?(o) - # binding.pry if $DEBUG - - - return false unless o.is_a?(Arel::Nodes::SelectStatement) - - # false - o.cores.any? { |core| core.source.is_a?(Arel::Nodes::JoinSource) } - # rescue => e - # binding.pry - end + # def xxx_has_join_sources?(o) + # # binding.pry if $DEBUG + # + # return true + # + # # return false unless o.is_a?(Arel::Nodes::SelectStatement) + # # + # # o.cores.any? { |core| core.source.is_a?(Arel::Nodes::JoinSource) } + # end # Find the first projection or part of projection that can be used for ordering. Cannot use # projections with '*' or '1 AS one' in them. From 72f0b3e305062e57fe3a89477abe9148858b8f06 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Fri, 2 May 2025 11:34:27 +0100 Subject: [PATCH 12/13] Cleaning up --- lib/arel/visitors/sqlserver.rb | 25 +--- test/cases/implicit_order_test_sqlserver.rb | 102 +++++++++++++++++ test/cases/order_test_sqlserver.rb | 121 -------------------- 3 files changed, 108 insertions(+), 140 deletions(-) create mode 100644 test/cases/implicit_order_test_sqlserver.rb diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index faf061afc..8fcf88aa1 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -302,23 +302,22 @@ def select_statement_lock? @select_statement && @select_statement.lock end - # FETCH cannot be used without an order. If an order is not given then try to use the projections for the ordering. - # If no suitable projection are present then fallback to using the primary key of the table. + # FETCH cannot be used without an order. If no order is given, then try to use the projections for the ordering. + # If no suitable projection is present, then fallback to using the primary key of the table. def make_Fetch_Possible_And_Deterministic(o) - # binding.pry if $DEBUG - return if o.limit.nil? && o.offset.nil? return if o.orders.any? if any_groupings?(o) || has_non_table_join_sources?(o) - if projection = projection_to_order_by_for_fetch(o) + if (projection = projection_to_order_by_for_fetch(o)) o.orders = [projection.asc] return end end - pk = primary_Key_From_Table(table_From_Statement(o)) - o.orders = [pk.asc] if pk + if (pk = primary_Key_From_Table(table_From_Statement(o))) + o.orders = [pk.asc] + end end def any_groupings?(o) @@ -329,18 +328,6 @@ def has_non_table_join_sources?(o) o.cores.none? { |core| core.source.left.is_a?(Arel::Table) } end - # TODO: Need this for "in the subquery the first projection is used for ordering if none provided" test. - # TODO: rename - # def xxx_has_join_sources?(o) - # # binding.pry if $DEBUG - # - # return true - # - # # return false unless o.is_a?(Arel::Nodes::SelectStatement) - # # - # # o.cores.any? { |core| core.source.is_a?(Arel::Nodes::JoinSource) } - # end - # Find the first projection or part of projection that can be used for ordering. Cannot use # projections with '*' or '1 AS one' in them. def projection_to_order_by_for_fetch(o) diff --git a/test/cases/implicit_order_test_sqlserver.rb b/test/cases/implicit_order_test_sqlserver.rb new file mode 100644 index 000000000..a5e7c4c38 --- /dev/null +++ b/test/cases/implicit_order_test_sqlserver.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require "cases/helper_sqlserver" +require "models/post" +require "models/company" + +class ImplicitOrderTestSQLServer < ActiveRecord::TestCase + + + describe "GROUP queries" do + + it "order by primary key if not a GROUP query" do + assert_queries_match(/#{Regexp.escape("ORDER BY [posts].[id] ASC")}/i) do + Post.pick(:title) + end + end + + it "ordering not required if not using FETCH" do + assert_queries_match(/^#{Regexp.escape("SELECT count(*) FROM [posts] GROUP BY [posts].[title]")}$/i) do + Post.group(:title).select("count(*)").load + end + end + + it "error if using `first` without primary key projection (as `find_nth_with_limit` adds primary key ordering)" do + error = assert_raises(ActiveRecord::StatementInvalid) do + Post.select(:title, "count(*)").group(:title).first(2) + end + assert_match(/Column "posts\.id" is invalid in the ORDER BY clause/, error.message) + end + + + it "using `first` with primary key projection (as `find_nth_with_limit` adds primary key ordering)" do + assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title]")}/i) do + Post.select(:title, "count(*)").group(:title).order(:title).first(2) + end + end + end + + + + # describe "simple query containing limit" do + # it "order by primary key if no projections" do + # sql = Post.limit(5).to_sql + # + # assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + # end + # + # it "use order provided" do + # sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql + # + # assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + # end + # + # end + # + # describe "query containing FROM and limit" do + # it "uses the provided orderings" do + # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + # + # assert_queries_match(/#{Regexp.escape(sql)}/) do + # result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + # assert_equal result, [11, 5, 1] + # end + # end + # + # it "in the subquery the first projection is used for ordering if none provided" do + # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + # + # # $DEBUG = true + # + # assert_queries_match(/#{Regexp.escape(sql)}/) do + # result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + # assert_equal result, [10, 5, 0] + # end + # end + # + # it "in the subquery the primary key is used for ordering if none provided" do + # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + # + # assert_queries_match(/#{Regexp.escape(sql)}/) do + # result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + # assert_equal result, [10, 5, 0] + # end + # end + # end + # + # + # it "generates correct SQL" do + # + # # $DEBUG = true + # + # sql = "SELECT [posts].[title], [posts].[id] FROM [posts] ORDER BY [posts].[id] ASC" + # + # assert_queries_match(/#{Regexp.escape(sql)}/) do + # Post.select(posts: [:title, :id]).take + # end + # + # # assert_match /#{Regexp.escape(sql)}/, Post.select(posts: [:bar, :id]).to_sql + # + # end + +end diff --git a/test/cases/order_test_sqlserver.rb b/test/cases/order_test_sqlserver.rb index 3dabfa3fb..82f43d539 100644 --- a/test/cases/order_test_sqlserver.rb +++ b/test/cases/order_test_sqlserver.rb @@ -6,8 +6,6 @@ class OrderTestSQLServer < ActiveRecord::TestCase fixtures :posts - - it "not mangel complex order clauses" do xyz_order = "CASE WHEN [title] LIKE N'XYZ%' THEN 0 ELSE 1 END" xyz_post = Post.create title: "XYZ Post", body: "Test cased orders." @@ -152,123 +150,4 @@ class OrderTestSQLServer < ActiveRecord::TestCase sql = Post.order(:id).order("posts.id ASC").to_sql assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC, posts.id ASC", sql end - - - describe "grouping queries" do - - # it "order by primary key by default if not a grouping query" do - # - # $DEBUG = true - # - # assert_queries_match(/ORDER BY \[posts\]\.\[id\] ASC/i) do - # Post.pick(:title) - # end - # end - - - it "ordering not required if not using FETCH" do - # skip - - assert_queries_match(/^#{Regexp.escape("SELECT count(*) FROM [posts] GROUP BY [posts].[title]")}$/i) do - Post.group(:title).select("count(*)").load - end - end - - it "error thrown if using FETCH without ordering and column not in select" do - # skip - - error = assert_raises(ActiveRecord::StatementInvalid) do - Post.select(:title, "count(*)").group(:title).first(2) - end - assert_match(/Column "posts\.id" is invalid in the ORDER BY clause/, error.message) - end - - it "ordering required if using FETCH" do - - # ActiveSupport::Notifications.subscribe('sql.active_record') do |_name, _start, _finish, _id, payload| - # puts payload[:sql] - # end - - # $DEBUG = true - - # TODO: failing when it should not fail. The ordering should be taken from the projection. - - # `first` calls `find_nth_with_limit` which adds ordering. - - assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY")}/i) do - # error = assert_raises(ActiveRecord::StatementInvalid) do - Post.select(:title, "count(*)").group(:title).order(:title).first(2) - end - - # assert_match(/Column "posts\.id" is invalid in the ORDER BY clause because/, error.message) - - # Post.group(:title).select("count(*)").first(2).load - - # assert_match(/Column "posts\.id" is invalid in the ORDER BY clause because it is not contained in either an aggregate function or the GROUP BY clause/, error.message) - end - - - # - # it "order by primary key by default if not a grouping query" do - # $DEBUG = true - # - # ActiveSupport::Notifications.subscribe('sql.active_record') do |_name, _start, _finish, _id, payload| - # puts payload[:sql] - # end - # - # Post.group(:title).select("count(*)").first(2).load - # end - - end - - - - describe "simple query containing limit" do - it "order by primary key if no projections" do - sql = Post.limit(5).to_sql - - assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - end - - it "use order provided" do - sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql - - assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - end - - - - - end - - describe "query containing FROM and limit" do - it "uses the provided orderings" do - sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - - assert_queries_match(/#{Regexp.escape(sql)}/) do - result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - assert_equal result, [11, 5, 1] - end - end - - it "in the subquery the first projection is used for ordering if none provided" do - sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - - # $DEBUG = true - - assert_queries_match(/#{Regexp.escape(sql)}/) do - result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - assert_equal result, [10, 5, 0] - end - end - - it "in the subquery the primary key is used for ordering if none provided" do - sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - - assert_queries_match(/#{Regexp.escape(sql)}/) do - result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - assert_equal result, [10, 5, 0] - end - end - end end From db61f02e14e2436a5313718a065fb7e38332a846 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Fri, 2 May 2025 13:57:15 +0100 Subject: [PATCH 13/13] Moved tests --- test/cases/fetch_test_sqlserver.rb | 71 ++++++++++++++ test/cases/implicit_order_test_sqlserver.rb | 102 -------------------- 2 files changed, 71 insertions(+), 102 deletions(-) delete mode 100644 test/cases/implicit_order_test_sqlserver.rb diff --git a/test/cases/fetch_test_sqlserver.rb b/test/cases/fetch_test_sqlserver.rb index 2b55bd25a..51214ffe2 100755 --- a/test/cases/fetch_test_sqlserver.rb +++ b/test/cases/fetch_test_sqlserver.rb @@ -2,8 +2,11 @@ require "cases/helper_sqlserver" require "models/book" +require "models/post" class FetchTestSqlserver < ActiveRecord::TestCase + fixtures :posts + let(:books) { @books } before { create_10_books } @@ -59,6 +62,74 @@ class FetchTestSqlserver < ActiveRecord::TestCase query.to_sql end end + + it "order by the provided orderings" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [11, 5, 1] + end + end + + it "in subquery order by first projection if no order provided" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [10, 5, 0] + end + end + + it "in subquery order by primary key if no projections and order provided" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [10, 5, 0] + end + end + end + + describe "GROUP query" do + it "order by primary key if not a GROUP query" do + assert_queries_match(/#{Regexp.escape("ORDER BY [posts].[id] ASC")}/i) do + Post.pick(:title) + end + end + + it "invalid SQL generated by `find_nth_with_limit` adding primary key ordering" do + error = assert_raises(ActiveRecord::StatementInvalid) do + Post.select(:title, "count(*)").group(:title).first(2) + end + assert_match(/Column "posts\.id" is invalid in the ORDER BY clause/, error.message) + end + + it "valid SQL generated if order specified" do + assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title]")}/i) do + Post.select(:title, "count(*)").group(:title).order(:title).first(2) + end + end + end + + describe "LIMIT query" do + it "order by primary key if no projections" do + sql = Post.limit(5).to_sql + + assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + end + + it "order by primary key if projections and no order provided" do + sql = Post.select(:legacy_comments_count).limit(5).to_sql + + assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + end + + it "use order provided" do + sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql + + assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + end end protected diff --git a/test/cases/implicit_order_test_sqlserver.rb b/test/cases/implicit_order_test_sqlserver.rb deleted file mode 100644 index a5e7c4c38..000000000 --- a/test/cases/implicit_order_test_sqlserver.rb +++ /dev/null @@ -1,102 +0,0 @@ -# frozen_string_literal: true - -require "cases/helper_sqlserver" -require "models/post" -require "models/company" - -class ImplicitOrderTestSQLServer < ActiveRecord::TestCase - - - describe "GROUP queries" do - - it "order by primary key if not a GROUP query" do - assert_queries_match(/#{Regexp.escape("ORDER BY [posts].[id] ASC")}/i) do - Post.pick(:title) - end - end - - it "ordering not required if not using FETCH" do - assert_queries_match(/^#{Regexp.escape("SELECT count(*) FROM [posts] GROUP BY [posts].[title]")}$/i) do - Post.group(:title).select("count(*)").load - end - end - - it "error if using `first` without primary key projection (as `find_nth_with_limit` adds primary key ordering)" do - error = assert_raises(ActiveRecord::StatementInvalid) do - Post.select(:title, "count(*)").group(:title).first(2) - end - assert_match(/Column "posts\.id" is invalid in the ORDER BY clause/, error.message) - end - - - it "using `first` with primary key projection (as `find_nth_with_limit` adds primary key ordering)" do - assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title]")}/i) do - Post.select(:title, "count(*)").group(:title).order(:title).first(2) - end - end - end - - - - # describe "simple query containing limit" do - # it "order by primary key if no projections" do - # sql = Post.limit(5).to_sql - # - # assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - # end - # - # it "use order provided" do - # sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql - # - # assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql - # end - # - # end - # - # describe "query containing FROM and limit" do - # it "uses the provided orderings" do - # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - # - # assert_queries_match(/#{Regexp.escape(sql)}/) do - # result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - # assert_equal result, [11, 5, 1] - # end - # end - # - # it "in the subquery the first projection is used for ordering if none provided" do - # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - # - # # $DEBUG = true - # - # assert_queries_match(/#{Regexp.escape(sql)}/) do - # result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - # assert_equal result, [10, 5, 0] - # end - # end - # - # it "in the subquery the primary key is used for ordering if none provided" do - # sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" - # - # assert_queries_match(/#{Regexp.escape(sql)}/) do - # result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) - # assert_equal result, [10, 5, 0] - # end - # end - # end - # - # - # it "generates correct SQL" do - # - # # $DEBUG = true - # - # sql = "SELECT [posts].[title], [posts].[id] FROM [posts] ORDER BY [posts].[id] ASC" - # - # assert_queries_match(/#{Regexp.escape(sql)}/) do - # Post.select(posts: [:title, :id]).take - # end - # - # # assert_match /#{Regexp.escape(sql)}/, Post.select(posts: [:bar, :id]).to_sql - # - # end - -end