From 41b92914f89be855cc6af768f13fd5fc53c967fa Mon Sep 17 00:00:00 2001 From: Vladimir Kochnev Date: Tue, 25 Sep 2018 14:54:03 +0400 Subject: Abandon TOP support. Initially, `TOP` was introduced to support `limit` for MSSQL database. Unlike PostgreSQL/MySQL/SQLite, MSSQL does not have native `LIMIT`/`OFFSET` support. The commit adding `TOP` is 1a246f71616cf246a75ef6cbdb56032e43d4e643. However, it figured out that `TOP` implementation was weak and it's not sufficient to also support `OFFSET`, then `TOP` was substituted with `ROW_NUMBER()` subquery in be48ed3071fd6524d0145c4ad3faeb4aafe3eda3. This is a well known trick in MSSQL - https://stackoverflow.com/questions/2135418/equivalent-of-limit-and-offset-for-sql-server. So now we don't need this `visit_Arel_Nodes_Top` at all. It does nothing useful but also adds an extra space after `SELECT` when `LIMIT` is being used for **any** database. --- activerecord/lib/arel/nodes/select_core.rb | 6 ++---- activerecord/lib/arel/nodes/unary.rb | 1 - activerecord/lib/arel/select_manager.rb | 2 -- activerecord/lib/arel/visitors/depth_first.rb | 1 - activerecord/lib/arel/visitors/dot.rb | 1 - activerecord/lib/arel/visitors/mssql.rb | 7 ------- activerecord/lib/arel/visitors/to_sql.rb | 7 ------- activerecord/test/cases/arel/select_manager_test.rb | 4 ++-- activerecord/test/cases/arel/visitors/depth_first_test.rb | 1 - activerecord/test/cases/arel/visitors/dot_test.rb | 1 - 10 files changed, 4 insertions(+), 27 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/arel/nodes/select_core.rb b/activerecord/lib/arel/nodes/select_core.rb index 2defe61974..73461ff683 100644 --- a/activerecord/lib/arel/nodes/select_core.rb +++ b/activerecord/lib/arel/nodes/select_core.rb @@ -3,13 +3,12 @@ module Arel # :nodoc: all module Nodes class SelectCore < Arel::Nodes::Node - attr_accessor :top, :projections, :wheres, :groups, :windows + attr_accessor :projections, :wheres, :groups, :windows attr_accessor :havings, :source, :set_quantifier def initialize super() @source = JoinSource.new nil - @top = nil # https://ronsavage.github.io/SQL/sql-92.bnf.html#set%20quantifier @set_quantifier = nil @@ -43,7 +42,7 @@ module Arel # :nodoc: all def hash [ - @source, @top, @set_quantifier, @projections, + @source, @set_quantifier, @projections, @wheres, @groups, @havings, @windows ].hash end @@ -51,7 +50,6 @@ module Arel # :nodoc: all def eql?(other) self.class == other.class && self.source == other.source && - self.top == other.top && self.set_quantifier == other.set_quantifier && self.projections == other.projections && self.wheres == other.wheres && diff --git a/activerecord/lib/arel/nodes/unary.rb b/activerecord/lib/arel/nodes/unary.rb index a3c0045897..00639304e4 100644 --- a/activerecord/lib/arel/nodes/unary.rb +++ b/activerecord/lib/arel/nodes/unary.rb @@ -37,7 +37,6 @@ module Arel # :nodoc: all On Ordering RollUp - Top }.each do |name| const_set(name, Class.new(Unary)) end diff --git a/activerecord/lib/arel/select_manager.rb b/activerecord/lib/arel/select_manager.rb index 733176ad9e..a2b2838a3d 100644 --- a/activerecord/lib/arel/select_manager.rb +++ b/activerecord/lib/arel/select_manager.rb @@ -222,10 +222,8 @@ module Arel # :nodoc: all def take(limit) if limit @ast.limit = Nodes::Limit.new(limit) - @ctx.top = Nodes::Top.new(limit) else @ast.limit = nil - @ctx.top = nil end self end diff --git a/activerecord/lib/arel/visitors/depth_first.rb b/activerecord/lib/arel/visitors/depth_first.rb index bcf8f8f980..5948622aea 100644 --- a/activerecord/lib/arel/visitors/depth_first.rb +++ b/activerecord/lib/arel/visitors/depth_first.rb @@ -34,7 +34,6 @@ module Arel # :nodoc: all alias :visit_Arel_Nodes_Ordering :unary alias :visit_Arel_Nodes_Ascending :unary alias :visit_Arel_Nodes_Descending :unary - alias :visit_Arel_Nodes_Top :unary alias :visit_Arel_Nodes_UnqualifiedColumn :unary def function(o) diff --git a/activerecord/lib/arel/visitors/dot.rb b/activerecord/lib/arel/visitors/dot.rb index d352b81914..76830412d4 100644 --- a/activerecord/lib/arel/visitors/dot.rb +++ b/activerecord/lib/arel/visitors/dot.rb @@ -81,7 +81,6 @@ module Arel # :nodoc: all alias :visit_Arel_Nodes_Not :unary alias :visit_Arel_Nodes_Offset :unary alias :visit_Arel_Nodes_On :unary - alias :visit_Arel_Nodes_Top :unary alias :visit_Arel_Nodes_UnqualifiedColumn :unary alias :visit_Arel_Nodes_Preceding :unary alias :visit_Arel_Nodes_Following :unary diff --git a/activerecord/lib/arel/visitors/mssql.rb b/activerecord/lib/arel/visitors/mssql.rb index 9aedc51d15..d564e19089 100644 --- a/activerecord/lib/arel/visitors/mssql.rb +++ b/activerecord/lib/arel/visitors/mssql.rb @@ -12,13 +12,6 @@ module Arel # :nodoc: all private - # `top` wouldn't really work here. I.e. User.select("distinct first_name").limit(10) would generate - # "select top 10 distinct first_name from users", which is invalid query! it should be - # "select distinct top 10 first_name from users" - def visit_Arel_Nodes_Top(o) - "" - end - def visit_Arel_Visitors_MSSQL_RowNumber(o, collector) collector << "ROW_NUMBER() OVER (ORDER BY " inject_join(o.children, collector, ", ") << ") as _row_num" diff --git a/activerecord/lib/arel/visitors/to_sql.rb b/activerecord/lib/arel/visitors/to_sql.rb index 0682c066fb..81ca63f261 100644 --- a/activerecord/lib/arel/visitors/to_sql.rb +++ b/activerecord/lib/arel/visitors/to_sql.rb @@ -237,8 +237,6 @@ module Arel # :nodoc: all def visit_Arel_Nodes_SelectCore(o, collector) collector << "SELECT" - collector = maybe_visit o.top, collector - collector = maybe_visit o.set_quantifier, collector collect_nodes_for o.projections, collector, SPACE @@ -405,11 +403,6 @@ module Arel # :nodoc: all visit o.expr, collector end - # FIXME: this does nothing on most databases, but does on MSSQL - def visit_Arel_Nodes_Top(o, collector) - collector - end - def visit_Arel_Nodes_Lock(o, collector) visit o.expr, collector end diff --git a/activerecord/test/cases/arel/select_manager_test.rb b/activerecord/test/cases/arel/select_manager_test.rb index c17487ae88..5220950905 100644 --- a/activerecord/test/cases/arel/select_manager_test.rb +++ b/activerecord/test/cases/arel/select_manager_test.rb @@ -131,7 +131,7 @@ module Arel right = table.alias mgr = table.from mgr.join(right).on("omg") - mgr.to_sql.must_be_like %{ SELECT FROM "users" INNER JOIN "users" "users_2" ON omg } + mgr.to_sql.must_be_like %{ SELECT FROM "users" INNER JOIN "users" "users_2" ON omg } end it "converts to sqlliterals with multiple items" do @@ -139,7 +139,7 @@ module Arel right = table.alias mgr = table.from mgr.join(right).on("omg", "123") - mgr.to_sql.must_be_like %{ SELECT FROM "users" INNER JOIN "users" "users_2" ON omg AND 123 } + mgr.to_sql.must_be_like %{ SELECT FROM "users" INNER JOIN "users" "users_2" ON omg AND 123 } end end end diff --git a/activerecord/test/cases/arel/visitors/depth_first_test.rb b/activerecord/test/cases/arel/visitors/depth_first_test.rb index 3baccc7316..f94ad521d7 100644 --- a/activerecord/test/cases/arel/visitors/depth_first_test.rb +++ b/activerecord/test/cases/arel/visitors/depth_first_test.rb @@ -33,7 +33,6 @@ module Arel Arel::Nodes::Ordering, Arel::Nodes::StringJoin, Arel::Nodes::UnqualifiedColumn, - Arel::Nodes::Top, Arel::Nodes::Limit, Arel::Nodes::Else, ].each do |klass| diff --git a/activerecord/test/cases/arel/visitors/dot_test.rb b/activerecord/test/cases/arel/visitors/dot_test.rb index 98f3bab620..6b3c132f83 100644 --- a/activerecord/test/cases/arel/visitors/dot_test.rb +++ b/activerecord/test/cases/arel/visitors/dot_test.rb @@ -37,7 +37,6 @@ module Arel Arel::Nodes::Offset, Arel::Nodes::Ordering, Arel::Nodes::UnqualifiedColumn, - Arel::Nodes::Top, Arel::Nodes::Limit, ].each do |klass| define_method("test_#{klass.name.gsub('::', '_')}") do -- cgit v1.2.3