From d5994ee48af14d67f0eec7d23863d4b19211b078 Mon Sep 17 00:00:00 2001 From: John Mileham Date: Thu, 3 Mar 2011 23:26:45 -0500 Subject: [PATCH] Change behavior of count(:limit => x, :offset => y) to limit/offset before counting. --- .../active_record/relation/calculations.rb | 40 +++++++++++------ activerecord/test/cases/calculations_test.rb | 45 ++++++++++++------- activerecord/test/cases/relations_test.rb | 28 ++++++++++++ 3 files changed, 84 insertions(+), 29 deletions(-) diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index c1842b1a96..7bfeb20a2b 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -183,10 +183,13 @@ module ActiveRecord end end - def aggregate_column(column_name) + def aggregate_column(column_name, subquery_alias = nil) if @klass.column_names.include?(column_name.to_s) - Arel::Attribute.new(@klass.unscoped.table, column_name) + Arel::Attribute.new(subquery_alias || @klass.unscoped.table, column_name) else + if subquery_alias && (split_name = column_name.to_s.split(".")).length > 1 + column_name = split_name.last + end Arel.sql(column_name == :all ? "*" : column_name.to_s) end end @@ -196,24 +199,22 @@ module ActiveRecord end def execute_simple_calculation(operation, column_name, distinct) #:nodoc: - column = aggregate_column(column_name) - # Postgresql doesn't like ORDER BY when there are no GROUP BY relation = except(:order) - select_value = operation_over_aggregate_column(column, operation, distinct) - relation.select_values = [select_value] + if operation == "count" && (relation.limit_value || relation.offset_value) + # Shortcut when limit is zero. + return 0 if relation.limit_value == 0 - query_builder = relation.arel + query_builder = build_count_subquery(relation, column_name, distinct) + else + column = aggregate_column(column_name) - if operation == "count" - limit = relation.limit_value - offset = relation.offset_value + select_value = operation_over_aggregate_column(column, operation, distinct) - unless limit && offset - query_builder.limit = nil - query_builder.offset = nil - end + relation.select_values = [select_value] + + query_builder = relation.arel end type_cast_calculated_value(@klass.connection.select_value(query_builder.to_sql), column_for(column_name), operation) @@ -312,5 +313,16 @@ module ActiveRecord select if select !~ /(,|\*)/ end end + + def build_count_subquery(relation, column_name, distinct) + # Arel doesn't do subqueries + subquery_alias = arel_table.alias("subquery_for_count") + aliased_column = aggregate_column(column_name, subquery_alias) + select_value = operation_over_aggregate_column(aliased_column, 'count', distinct) + + relation.select_values = [(column_name == :all ? 1 : aggregate_column(column_name))] + subquery_sql = "(#{relation.arel.to_sql}) #{subquery_alias.name}" + subquery_alias.relation.select_manager.project(select_value).from(subquery_sql) + end end end diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index caf07a7357..1e8ce4fda7 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -65,7 +65,7 @@ class CalculationsTest < ActiveRecord::TestCase c = Account.sum(:credit_limit, :group => :firm_id) [1,6,2].each { |firm_id| assert c.keys.include?(firm_id) } end - + def test_should_group_by_multiple_fields c = Account.count(:all, :group => ['firm_id', :credit_limit]) [ [nil, 50], [1, 50], [6, 50], [6, 55], [9, 53], [2, 60] ].each { |firm_and_limit| assert c.keys.include?(firm_and_limit) } @@ -109,6 +109,35 @@ class CalculationsTest < ActiveRecord::TestCase assert_equal [2, 6], c.keys.compact end + def test_limit_should_apply_before_count + accounts = Account.limit(3).where('firm_id IS NOT NULL') + + assert_equal 3, accounts.count(:firm_id) + assert_equal 3, accounts.select(:firm_id).count + end + + def test_count_should_shortcut_with_limit_zero + accounts = Account.limit(0) + + assert_no_queries { assert_equal 0, accounts.count } + end + + def test_limit_is_kept + return if current_adapter?(:OracleAdapter) + + queries = assert_sql { Account.limit(1).count } + assert_equal 1, queries.length + assert_match(/LIMIT/, queries.first) + end + + def test_offset_is_kept + return if current_adapter?(:OracleAdapter) + + queries = assert_sql { Account.offset(1).count } + assert_equal 1, queries.length + assert_match(/OFFSET/, queries.first) + end + def test_limit_with_offset_is_kept return if current_adapter?(:OracleAdapter) @@ -118,20 +147,6 @@ class CalculationsTest < ActiveRecord::TestCase assert_match(/OFFSET/, queries.first) end - def test_offset_without_limit_removes_offset - queries = assert_sql { Account.offset(1).count } - assert_equal 1, queries.length - assert_no_match(/LIMIT/, queries.first) - assert_no_match(/OFFSET/, queries.first) - end - - def test_limit_without_offset_removes_limit - queries = assert_sql { Account.limit(1).count } - assert_equal 1, queries.length - assert_no_match(/LIMIT/, queries.first) - assert_no_match(/OFFSET/, queries.first) - end - def test_no_limit_no_offset queries = assert_sql { Account.count } assert_equal 1, queries.length diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 37bbb17e74..e9c006d623 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -666,6 +666,34 @@ class RelationTest < ActiveRecord::TestCase assert_no_queries { assert_equal 5, best_posts.size } end + def test_size_with_limit + posts = Post.limit(6) + + assert_queries(1) { assert_equal 6, posts.size } + assert ! posts.loaded? + + best_posts = posts.where(:comments_count => 0) + best_posts.to_a # force load + assert_no_queries { assert_equal 5, best_posts.size } + end + + def test_size_with_zero_limit + posts = Post.limit(0) + + assert_no_queries { assert_equal 0, posts.size } + assert ! posts.loaded? + + posts.to_a # force load + assert_no_queries { assert_equal 0, posts.size } + end + + def test_empty_with_zero_limit + posts = Post.limit(0) + + assert_no_queries { assert_equal true, posts.empty? } + assert ! posts.loaded? + end + def test_count_complex_chained_relations posts = Post.select('comments_count').where('id is not null').group("author_id").where("comments_count > 0")