Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Use LIMIT SQL word in first - Closes #2783 #2789

Merged
merged 2 commits into from Sep 5, 2011

Conversation

Projects
None yet
2 participants
Contributor

dmathieu commented Sep 1, 2011

Currently, #first and #last are using to_a, which retrieves all entries from the database and loads them in memory.
That's quite inefficient and useless.

This PR changes this behavior, to only retrieve the required entries.

@jonleighton jonleighton and 1 other commented on an outdated diff Sep 4, 2011

...verecord/lib/active_record/relation/finder_methods.rb
@@ -134,7 +134,7 @@ module ActiveRecord
def last(*args)
if args.any?
if args.first.kind_of?(Integer) || (loaded? && !args.first.kind_of?(Hash))
- to_a.last(*args)
+ order("#{primary_key} DESC").limit(*args).reverse
@jonleighton

jonleighton Sep 4, 2011

Member

I am concerned about this clashing with relations that already have an order. Can you add a test to ensure it works as expected?

@dmathieu

dmathieu Sep 5, 2011

Contributor

You're right. Actually, it seems it's not possible to get last with LIMIT properly (=> without making a count request, nor reordering).
I'm reverting it to what it was.

@jonleighton

jonleighton Sep 5, 2011

Member

Maybe you could use the above if the relation has no existing ordering, and otherwise fall back to to_a?

@dmathieu

dmathieu Sep 5, 2011

Contributor

I've just pushed it.

@jonleighton jonleighton commented on the diff Sep 4, 2011

activerecord/test/cases/finder_test.rb
@@ -243,6 +243,11 @@ class FinderTest < ActiveRecord::TestCase
end
end
+ def test_first_and_last_with_integer_should_use_sql_limit
+ assert_sql(/LIMIT 2/) { Topic.first(2).entries }
+ assert_sql(/LIMIT 5/) { Topic.last(5).entries }
+ end
@jonleighton

jonleighton Sep 4, 2011

Member

Please could you test against actual records, rather than the SQL, so we can be sure it works as imagined. Also would be good to assert that the return value is an ActiveRecord::Relation.

@dmathieu

dmathieu Sep 5, 2011

Contributor

Well, the value returned by first and last is already tested here.
I'll add a test checking that the value is an ActiveRecord::Relation.

@jonleighton

jonleighton Sep 5, 2011

Member

Ok, despite what I said earlier, I now think we should actually convert the values to a flat Array because:

  • last cannot always be a Relation, and I think it is good to have consistency between first and last
  • I can't think of a use-case for wanting first or last to be a Relation
@dmathieu

dmathieu Sep 5, 2011

Contributor

Done.

@jonleighton

jonleighton Sep 5, 2011

Member

Thanks. Do you want to implement what I suggested above? ("Maybe you could use the above if the relation has no existing ordering, and otherwise fall back to to_a?") If not I am happy to merge this as-is, but if you want to then I'll wait for that.

@dmathieu

dmathieu Sep 5, 2011

Contributor

Yes, I'm currently on it.

Contributor

dmathieu commented Sep 5, 2011

I've just updated the pull request with @jonleighton's comments.

@jonleighton jonleighton and 1 other commented on an outdated diff Sep 5, 2011

activerecord/test/cases/finder_test.rb
@@ -243,6 +243,26 @@ class FinderTest < ActiveRecord::TestCase
end
end
+ def test_first_and_last_with_integer_should_use_sql_limit
+ assert_sql(/LIMIT 2/) { Topic.first(2).entries }
+ assert_sql(/LIMIT 5/) { Topic.last(5).entries }
+ end
+
+ def test_last_with_integer_and_order_should_keep_the_order
+ assert_equal Topic.order("title").last(2), Topic.order("title").to_a.last(2)
@jonleighton

jonleighton Sep 5, 2011

Member

Could you switch the arguments here please? The expected value should come first.

@dmathieu

dmathieu Sep 5, 2011

Contributor

Done.

@jonleighton jonleighton commented on the diff Sep 5, 2011

...verecord/lib/active_record/relation/finder_methods.rb
@@ -134,7 +134,11 @@ module ActiveRecord
def last(*args)
if args.any?
if args.first.kind_of?(Integer) || (loaded? && !args.first.kind_of?(Hash))
- to_a.last(*args)
+ if order_values.empty?
@jonleighton

jonleighton Sep 5, 2011

Member

Sorry to make yet another comment, but we need to check reorder_values too. Sorry, I should have caught that before.

@dmathieu

dmathieu Sep 5, 2011

Contributor

No problem. Fixed and pushed.

@jonleighton jonleighton added a commit that referenced this pull request Sep 5, 2011

@jonleighton jonleighton Merge pull request #2789 from dmathieu/limit_first_last
Use LIMIT SQL word in first - Closes #2783
e221108

@jonleighton jonleighton merged commit e221108 into rails:master Sep 5, 2011

Member

jonleighton commented Sep 5, 2011

Thanks. Do you mind backporting to 3-1-stable (including a CHANGELOG entry)?

@jonleighton jonleighton added a commit that referenced this pull request Sep 5, 2011

@jonleighton jonleighton Merge pull request #2871 from dmathieu/limit-3-1-stable
Backport #2789 to 3-1-stable
76d3b47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment