Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Optimize none? and one? relation query methods to use LIMIT 1 and COUNT. #12670

Closed
wants to merge 1 commit into from

4 participants

@egilburg

I was surprised to find out that relation's none? method was loading the entire collection as an array,
even if no block or limit was given. This contrasts how any? and many? work, which is to use LIMIT 1 and COUNT respectively, if no block and limit is given.

IMO it's more natural to use .none? query rather than either empty? (because empty? is an Array method while none? is an Enumerable method and thus seems more suitable for a generic set such a a set of database rows). I therefore changed none? to follow the already existing behavior for any? and many?.

I also implemented an optimized one? for similar reasons and for completion sake - just as me, others could be surprised that one? behaves differently than any? and many?, and I don't see a drawback to being consistent in providing an efficient implementation if possible.

This change applies to relations (e.g. User.all) as well as associations (e.g. account.users).

Before:

users.none?
SELECT "users".* FROM "users"

users.one?
SELECT "users".* FROM "users"

After:

users.none?
SELECT 1 AS one FROM "users" LIMIT 1

users.one?
SELECT COUNT(*) FROM "users"

NullRelation has been updated to short-curciut none? and one?, as it already does with any? and many?.

Also, improved method documentation a bit.

I added the (appropriately modified) same set of tests as present for any? and many? methods in relations_test.rb and associations/has_many_association_test.rb unit tests.

@egilburg

Seems the build is also erroring out on master branch.

@egilburg egilburg Optimize none? and one? relation query methods to use LIMIT and COUNT.
Use SQL COUNT and LIMIT 1 queries for none? and one? methods if no block or limit is given,
instead of loading the entire collection to memory. The any? and many? methods already
follow this behavior.
d657eec
@egilburg

@core team: I'm not sure whether I should define one? and none? on CollectionAssociation in addition to defining it on Relation. Tests pass without this definition (because CollectionAssociation forwards to Relation), but any? and many? are defined on both CollectionAssociation and Relation, with slight implementation differences, although I'm not sure what the difference actually does and whether it's needed for my case.

@seuros
Collaborator

:+1:

@cefigueiredo cefigueiredo commented on the diff
...test/cases/associations/has_many_associations_test.rb
((62 lines not shown))
+ def test_calling_one_should_return_false_if_zero
+ firm = companies(:another_firm)
+ assert ! firm.clients_like_ms.one?
+ assert_equal 0, firm.clients_like_ms.size
+ end
+
+ def test_calling_one_should_return_true_if_one
+ firm = companies(:first_firm)
+ assert firm.limited_clients.one?
+ assert_equal 1, firm.limited_clients.size
+ end
+
+ def test_calling_one_should_return_false_if_more_than_one
+ firm = companies(:first_firm)
+ assert ! firm.clients.one?
+ assert_equal 2, firm.clients.size

The database was changed since you did that test, so it's failing because now there are 3 clients...
I think you should rebase and change that test, because you are basing your test just in a quantity of data that could change over time on the test database... broking your test just because you forbidden the data to grow up...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cefigueiredo

When a block is not given to one?/none?, there is no point in loading the collection just to check if there is only one element or not, even when a limit is set...

Because even when a limit is given, the attributes of the element does not matter. What matter is just know if there is only one element or not.

So, it would be better do:

def one?
  if block_given?
    to_a.one? { |*block_args| yield(*block_args) }
  else
    size == 1
  end
end

With that, when calling one? and giving a block, it still loads the entire collection as an array and call the method one? from Enumerator. But when a block is not given, it call size, that translating to query is just a SELECT COUNT(*) that is far more efficient than retrieve all the data, and then, it checks if the size is equal 1...

@rafaelfranca rafaelfranca modified the milestone: 4.2.0, 5.0.0
@rafaelfranca

Merged at d7a7a05

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 28, 2013
  1. @egilburg

    Optimize none? and one? relation query methods to use LIMIT and COUNT.

    egilburg authored
    Use SQL COUNT and LIMIT 1 queries for none? and one? methods if no block or limit is given,
    instead of loading the entire collection to memory. The any? and many? methods already
    follow this behavior.
This page is out of date. Refresh to see the latest.
View
22 activerecord/CHANGELOG.md
@@ -1,3 +1,25 @@
+* Use SQL COUNT and LIMIT 1 queries for `none?` and `one?` methods if no block or limit is given,
+ instead of loading the entire collection to memory.
+ This applies to relations (e.g. `User.all`) as well as associations (e.g. `account.users`)
+
+ # Before:
+
+ users.none?
+ # SELECT "users".* FROM "users"
+
+ users.one?
+ # SELECT "users".* FROM "users"
+
+ # After:
+
+ users.none?
+ # SELECT 1 AS one FROM "users" LIMIT 1
+
+ users.one?
+ # SELECT COUNT(*) FROM "users"
+
+ *Eugene Gilburg*
+
* Allow for the name of the schema_migrations table to be configured.
*Jerad Phelps*
View
6 activerecord/lib/active_record/associations/collection_association.rb
@@ -301,7 +301,8 @@ def empty?
end
# Returns true if the collections is not empty.
- # Equivalent to +!collection.empty?+.
+ # If block given, loads all records and checks for one or more matches.
+ # Otherwise, equivalent to +!collection.empty?+.
def any?
if block_given?
load_target.any? { |*block_args| yield(*block_args) }
@@ -311,7 +312,8 @@ def any?
end
# Returns true if the collection has more than 1 record.
- # Equivalent to +collection.size > 1+.
+ # If block given, loads all records and checks for two or more matches.
+ # Otherwise, equivalent to +collection.size > 1+.
def many?
if block_given?
load_target.many? { |*block_args| yield(*block_args) }
View
8 activerecord/lib/active_record/null_relation.rb
@@ -30,10 +30,18 @@ def empty?
true
end
+ def none?
+ true
+ end
+
def any?
false
end
+ def one?
+ false
+ end
+
def many?
false
end
View
18 activerecord/lib/active_record/relation.rb
@@ -247,6 +247,15 @@ def empty?
limit_value == 0 ? true : !exists?
end
+ # Returns true if there are no records.
+ def none?
+ if block_given?
+ to_a.none? { |*block_args| yield(*block_args) }
+ else
+ empty?
+ end
+ end
+
# Returns true if there are any records.
def any?
if block_given?
@@ -256,6 +265,15 @@ def any?
end
end
+ # Returns true if there is exactly one record.
+ def one?
+ if block_given?
+ to_a.one? { |*block_args| yield(*block_args) }
+ else
+ limit_value ? to_a.one? : size == 1
+ end
+ end
+
# Returns true if there is more than one record.
def many?
if block_given?
View
76 activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1492,6 +1492,82 @@ def test_calling_many_should_return_true_if_more_than_one
assert_equal 2, firm.clients.size
end
+ def test_calling_none_should_count_instead_of_loading_association
+ firm = companies(:first_firm)
+ assert_queries(1) do
+ firm.clients.none? # use count query
+ end
+ assert !firm.clients.loaded?
+ end
+
+ def test_calling_none_on_loaded_association_should_not_use_query
+ firm = companies(:first_firm)
+ firm.clients.collect # force load
+ assert_no_queries { assert ! firm.clients.none? }
+ end
+
+ def test_calling_none_should_defer_to_collection_if_using_a_block
+ firm = companies(:first_firm)
+ assert_queries(1) do
+ firm.clients.expects(:size).never
+ firm.clients.none? { true }
+ end
+ assert firm.clients.loaded?
+ end
+
+ def test_calling_none_should_return_true_if_none
+ firm = companies(:another_firm)
+ assert firm.clients_like_ms.none?
+ assert_equal 0, firm.clients_like_ms.size
+ end
+
+ def test_calling_none_should_return_false_if_any
+ firm = companies(:first_firm)
+ assert !firm.limited_clients.none?
+ assert_equal 1, firm.limited_clients.size
+ end
+
+ def test_calling_one_should_count_instead_of_loading_association
+ firm = companies(:first_firm)
+ assert_queries(1) do
+ firm.clients.one? # use count query
+ end
+ assert !firm.clients.loaded?
+ end
+
+ def test_calling_one_on_loaded_association_should_not_use_query
+ firm = companies(:first_firm)
+ firm.clients.collect # force load
+ assert_no_queries { assert ! firm.clients.one? }
+ end
+
+ def test_calling_one_should_defer_to_collection_if_using_a_block
+ firm = companies(:first_firm)
+ assert_queries(1) do
+ firm.clients.expects(:size).never
+ firm.clients.one? { true }
+ end
+ assert firm.clients.loaded?
+ end
+
+ def test_calling_one_should_return_false_if_zero
+ firm = companies(:another_firm)
+ assert ! firm.clients_like_ms.one?
+ assert_equal 0, firm.clients_like_ms.size
+ end
+
+ def test_calling_one_should_return_true_if_one
+ firm = companies(:first_firm)
+ assert firm.limited_clients.one?
+ assert_equal 1, firm.limited_clients.size
+ end
+
+ def test_calling_one_should_return_false_if_more_than_one
+ firm = companies(:first_firm)
+ assert ! firm.clients.one?
+ assert_equal 2, firm.clients.size

The database was changed since you did that test, so it's failing because now there are 3 clients...
I think you should rebase and change that test, because you are basing your test just in a quantity of data that could change over time on the test database... broking your test just because you forbidden the data to grow up...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+
def test_joins_with_namespaced_model_should_use_correct_type
old = ActiveRecord::Base.store_full_sti_class
ActiveRecord::Base.store_full_sti_class = true
View
34 activerecord/test/cases/relations_test.rb
@@ -287,7 +287,9 @@ def test_null_relation_content_size_methods
assert_equal 0, Developer.none.size
assert_equal 0, Developer.none.count
assert_equal true, Developer.none.empty?
+ assert_equal true, Developer.none.none?
assert_equal false, Developer.none.any?
+ assert_equal false, Developer.none.one?
assert_equal false, Developer.none.many?
end
end
@@ -996,6 +998,38 @@ def test_many_with_limits
assert ! posts.limit(1).many?
end
+ def test_none
+ posts = Post.all
+ assert_queries(1) do
+ assert ! posts.none? # Uses COUNT()
+ end
+
+ assert ! posts.loaded?
+
+ assert_queries(1) do
+ assert posts.none? {|p| p.id < 0 }
+ assert ! posts.none? {|p| p.id == 1 }
+ end
+
+ assert posts.loaded?
+ end
+
+ def test_one
+ posts = Post.all
+ assert_queries(1) do
+ assert ! posts.one? # Uses COUNT()
+ end
+
+ assert ! posts.loaded?
+
+ assert_queries(1) do
+ assert ! posts.one? {|p| p.id < 3 }
+ assert posts.one? {|p| p.id == 1 }
+ end
+
+ assert posts.loaded?
+ end
+
def test_build
posts = Post.all
Something went wrong with that request. Please try again.