Always use SQL limit in Relation#last when limit argument given

[bogdan]

Using Relation#last causes entire relation to be loaded into memory.
In most cases it is unacceptable:

    Topic.order(:created_at).last(2)
      # => SELECT * FROM topics ORDER BY created_at

We can still use reverse_order to let SQL do heavy lifting (AR is quite bad at that).

    Topic.order(:created_at).last(2)
      # => SELECT * FROM topics ORDER BY created_at DESC LIMIT 2

Unfortunately back to 2011 in this commit 5f5527c it wasn't done. And 2 x unfortunately this commit added 2 tests that need to be modified now.

@dmathieu do you still remember something about it?

[egilburg]

Could you add a test to specifically check against full relation being loaded in memory? This way the limit won't be just considered an implementation detail that could regress in future.

[bogdan]

@egilburg added an assertion that relation wasn't loaded during after last call.

[sgrif]

Can we avoid the ternary here?

[bogdan]

done

[sgrif]

I don't think we need this assertion, if the intent is to make sure that we don't load the entire relation into memory

[sgrif]

So this will break the following case:

ActiveRecord::Schema.define do
  create_table :foos, primary_key: false do |t|
    t.string :name
  end
end

class Foo < ActiveRecord::Base
end

10.times { |n| Foo.create!(name: n) }

Foo.last(5)

If there's no primary key, and no existing order, we need to load the entire set into memory.

[bogdan]

@sgrif added a test and fixed the case you've described also removed unneeded assertion

[bogdan]

@sgrif after the merge of #18928, I've upgraded this PR to offer a smoother migration with deprecation warning. Please take a look.

[eileencodes]

@bogdan was this supposed to warn if the code is using a relation incorrectly? We have something like this in our app:

records_before = records.where('id <= ?', recording_id).order(id: :desc).limit(page_size / 2).last

and there was no deprecation warning telling me to add to_a. Took a little bit to track down what change I needed to make where.

[kaspth]

Should we also print the exception message so people know what they'd see in the future?

[bogdan]

I don't see a lot of benefit in that. Can you explain the use case when it is useful?

[kaspth]

Saw you raised IrreversibleOrderError with two different messages in #18928. At least the first one gives you more info than this warning does alone.

On the other hand, you do specify that people should just add to_a which I guess would be the fix anyway. So I'll backtrack: this is fine as is 👍

[bogdan]

@eileencodes I don't understand why you think you should make a change. to_a is only suggested when very complex SQL order is used like coalesce(a, b) etc. In your case, order is trivial and you don't have to change anything.

[eileencodes]

I thought I should make a change because something is not working correctly. We're getting the wrong record for last in our app. I'm going to investigate more today. It doesn't seem there are any tests that test the last record is the expected last record so it's difficult to track down correct behavior for all cases of last.

For our case we have order values so we are moving into reverse_order. From there reverse_order is running o.reverse. The first record after that is incorrect. Reversing the records again in Rails gets me the correct record.

[eileencodes]

I take back there being no tests that test the last is the correct last record (https://github.com/rails/rails/blob/master/activerecord/test/cases/relations_test.rb), but I'm unsure why changing result.first to result.reverse.first gets the right result in our app AND no tests fail.

[bogdan]

@eileencodes thanks for spotting out that. Can you also check if result.last also fixes your problem? I'll try to tackle down the problem on Monday.

[eileencodes]

I don't know what I was thinking when I said result.reverse.first worked. result.last throws an exception. But! I have a repro script. I think it has to do with collecting records through another record when it's an association relation and not just a relation. You can see in the SQL it's changing the order values (or flipping them maybe?)

post.comments.last => this should be DESC but is ASC
Comment Load (0.1ms) SELECT "comments".* FROM "comments" WHERE "comments"."post_id" = ? ORDER BY "comments"."id" ASC LIMIT ? [["post_id", 1], ["LIMIT", 1]]

post.comments.last.to_a => this is correctly DESC
Comment Load (0.1ms) SELECT "comments".* FROM "comments" WHERE "comments"."post_id" = ? ORDER BY "comments"."id" DESC LIMIT ? [["post_id", 1], ["LIMIT", 3]]

require 'active_record'
require 'minitest/autorun'

ActiveRecord::Base.logger = ActiveSupport::Logger.new($stdout)

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.connection.instance_eval do
  create_table "comments", force: true do |t| 
    t.string  "content"
    t.integer "post_id"
  end 

  create_table "posts", force: true do |t| 
    t.string "name"
  end 
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

class Post < ActiveRecord::Base
  has_many :comments
end

class BugTest < Minitest::Test
  def test_last
    (1..5).each do |n| 
      Post.create!(name: "this is a post #{n}")
    end 

    Post.all.each do |post|
      (1..5).each do |i| 
        x = i == 1 ? 1 : 4 
        Comment.create!(content: "test content #{i}", post_id: post.id)
      end 
    end 

    post = Post.find(1)
    comments = post.comments

    # assert order
    assert_equal ["test content 5", "test content 4", "test content 3"], comments.order(id: :desc).limit(3).map(&:content)
    # fail
    assert_equal "test content 3", comments.order(id: :desc).limit(3).last.content
    # pass
    assert_equal "test content 3", comments.order(id: :desc).limit(3).to_a.last.content
  end 
end

[bogdan]

Probably, the following will fix the issue:

result = result.limit!(limit || 1).to_a

[matthewd]

I'd go with result.to_a.reverse, to skip the array in the no-limit case.. but yeah, that looks to be the problem / solution. Just need to work out how to plug the hole in the tests to match.

[sgrif]

I've temporarily reverted this, as we are looking to release beta2 and there's still concerns with #23377 that need to be addressed. Once the issues with this have been addressed, we can bring this back in for RC1.