Skip to content

ActiveRecord collection not (obviously) sorted by score #608

New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alexceder opened this issue Aug 1, 2016 · 11 comments
Closed

ActiveRecord collection not (obviously) sorted by score #608

alexceder opened this issue Aug 1, 2016 · 11 comments

Comments

@alexceder
Copy link

All this came about when updating from Rails 4.2 to Rails 5.

I have hade some trouble with Elasticsearch not returning ActiveRecord models in order of score.

Reviewing the response, I can see that they are correctly returned in order by score so that:

response.map(&:id)
# => ["13", "12"]

However this is not what I want:

response.records.map(&:id)
# => [12, 13]

And this is where you might thing.. well you need to call to_a on the records. And yes, that works.

response.records.to_a.map(&:id)
# => [13, 12]

Just as the README states, it runs the corresponding SQL with only a WHILE statement. This confused me, where do you sort!?

https://github.com/elastic/elasticsearch-rails/blob/master/elasticsearch-model/README.md#search-results-as-database-records

The sorting is done when explicitly calling to_a. This is of course fine, but this isn't the big thing that confused me or where my errors appeared.

I first used this solution, but I thought it was rather ugly. This way I get the records sorted properly from the database, on primary key and not undefined behaviour.

response.records.order("FIELD (`table_name`.`primary_key`, 13, 12)").to_a.map(&:id)
# => [13, 12]

But scrap that, I don't want to write custom SQL for which should happen automagically.

So why am I writing this loooong wall of text.. Well, it seems like Rails has changed how it uses implicit convertions.. or something.. so.. This is probably where something happens (or more likely doesn't happen):

# Wrong ordering
render json: records

Because if I explicitly convert to array, all is well.

# Correct ordering
render json: records.to_a

This might be more of a gotcha, but it was a pain for me, and might be for others.

Is there a better way of doing this? Some other intended use? Is this dumb? Could the "default" order be done like I did it? Ideas? Dumb?

@alexceder alexceder changed the title ActiveRecord collection not sorted by score. ActiveRecord collection not (obviously) sorted by score Aug 1, 2016
@dimroc
Copy link

dimroc commented Aug 1, 2016

I also upgraded from Rails 4.2 to Rails 5 and ran into the same sorting issue. I explicitly had a sort term in the ES query using the elasticsearch dsl. The ES response was correctly sorted, but when using records the order was ignored.

@dimroc
Copy link

dimroc commented Aug 1, 2016

As a stop gap, I have been using this custom ElasticRelation class to wrap around records:

rows = ElasticRelation.new Row.search(params).records

It essentially sorts in memory as originally implemented here:

https://github.com/elastic/elasticsearch-rails/blob/master/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb#L35:

  def sorted_by_score
    hits = records.response.response.hits.hits
    records.sort_by do |record|
      hits.index { |hit| hit['_id'].to_s == record.id.to_s }
    end
  end

Figuring out how to plug this into Rails 5 AR is the fun part.

@inkstak
Copy link
Contributor

inkstak commented Aug 9, 2016

AR 5 delegates array methods on records instead of to_a

Delegation: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/delegation.rb#L39

records method: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation.rb#L257

I guess the name of the singleton method redefined by elasticsearch-models should depends on ActiveRecord version

@sr3d
Copy link

sr3d commented Aug 14, 2016

Confirmed about the bug happening for Rails 5 upgrading from Rails 4.2

@blackst0ne
Copy link

Any progress on this?

@johanlunds
Copy link

I think it has been merged. Read the comments on the referenced PR.

@karmi
Copy link
Contributor

karmi commented Mar 25, 2017

First of all, my apologies, @alexceder, for not responding to this issue sooner, and many thanks for the care you took in providing all the details in the issue description. Also, many thanks for everyone responding on this thread!

As @johanlunds says, the fix by @inkstak has indeed been merged through pull request #618.

To reply to the original question by @alexceder, I do agree that the whole story about needing to call to_a on the records object has been confusing, and I can imagine that many people have walked into that trap. However, at the time I couldn't figure out any other way how to do it, because I wanted the records object to be a true instance of ActiveRecord::Relation, so you can call eg. join on it, or do any other SQL tricks. I can imagine there was such a way, but I simply wasn't able to find it.

@alexceder, @sr3d, can you check your code with the current master, which contains the patch, and verify that it works as expected, and that there's less confusion about it in the application code? If you'd find any discrepancies or problems, I'd like to add tests for them in https://github.com/elastic/elasticsearch-rails/blob/master/elasticsearch-model/test/integration/active_record_basic_test.rb#L108-L121 to be able to fix them in an organized manner.

@karmi karmi added the waiting label Mar 29, 2017
@jujudellago
Copy link

I'm not sure if this was fixed for ActiveRecord, but I have exactly the same issue with Mongoid

@arcifius
Copy link

I'm also having this issue with Mongoid ;/

@stale
Copy link

stale bot commented Aug 31, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 31, 2020
@stale stale bot closed this as completed Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants