Skip to content
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

Improve performance of ratings in JSON #954

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/api/v1/bulk/queries_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def create
if Query.import queries_to_import

@case.reload
@queries = @case.queries.includes([ :ratings ])
@queries = @case.queries
@display_order = @queries.map(&:id)

respond_with @queries, @display_order
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v1/queries_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class QueriesController < Api::ApiController
before_action :check_query, only: [ :update, :destroy ]

def index
@queries = @case.queries.includes([ :ratings ])
@queries = @case.queries

@display_order = @queries.map(&:id)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,8 @@ def case_with_all_the_bells_whistles
if current_user
@case = current_user
.cases_involved_with
.where(id: params[:case_id])
.includes(:tries )
.preload([ queries: [ :ratings ], tries: [ :curator_variables, :search_endpoint ] ])
.order('tries.try_number DESC')
.first
.preload(:queries, tries: [ :curator_variables, :search_endpoint ])
.find_by(id: params[:case_id])
end
@case = Case.public_cases.find_by(id: params[:case_id]) if @case.nil?
end
Expand Down
4 changes: 4 additions & 0 deletions app/models/application_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,8 @@

class ApplicationRecord < ActiveRecord::Base
primary_abstract_class

def connection
self.class.connection
end
end
9 changes: 9 additions & 0 deletions app/models/case.rb
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,15 @@ def public_id
Rails.application.message_verifier('magic').generate(id)
end

# Optimized method to retrieve all doc ratings a single time
# Returns [5424, [{"query_id"=>5424, "doc_id"=>"l_21426", "rating"=>1.0}], ...]
def doc_ratings_by_query
@doc_ratings_by_query ||= begin
result = connection.select_all(ratings.select(:query_id, :doc_id, :rating).to_sql)
result.group_by { |r| r['query_id'] }
end
end

private

def set_scorer
Expand Down
2 changes: 1 addition & 1 deletion app/models/rating.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#

class Rating < ApplicationRecord
belongs_to :query
belongs_to :query, touch: true
belongs_to :user, optional: true

# arguably we shouldn't need this, however today you can have a rating object that doesn't have a
Expand Down
2 changes: 1 addition & 1 deletion app/views/api/v1/export/cases/_query.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ json.notes query.notes
json.information_need query.information_need

json.ratings do
json.array! query.ratings, partial: 'rating', as: :rating
@case.doc_ratings_by_query[query.id]&.each { |rating| json.set! rating['doc_id'], rating['rating'] }
end
6 changes: 2 additions & 4 deletions app/views/api/v1/queries/_query.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@ json.query_text query.query_text
json.options query.options
json.notes query.notes
json.information_need query.information_need

# pick the most recent update between a query and it's ratings to represent modified_at
json.modified_at [ query, query.ratings ].flatten.max_by(&:updated_at).updated_at
json.modified_at query.updated_at

json.ratings do
query.ratings.each { |rating| json.set! rating.doc_id, rating.rating }
@case.doc_ratings_by_query[query.id]&.each { |rating| json.set! rating['doc_id'], rating['rating'] }
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class UpdateQueryUpdatedAtWithRatings < ActiveRecord::Migration[7.1]
def change
execute <<-SQL
UPDATE queries SET updated_at = GREATEST(updated_at, (SELECT MAX(updated_at) FROM ratings WHERE query_id = queries.id))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively a new column could be added like ratings_updated_at

SQL
end
end
2 changes: 1 addition & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions test/models/case_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -290,4 +290,26 @@ class CaseTest < ActiveSupport::TestCase
assert_equal 2, the_case.queries_count
end
end

describe 'doc_ratings_by_query' do
let(:the_case) { cases(:random_case) }

it 'returns an empty hash for a new case' do
assert_equal Case.new.doc_ratings_by_query, {}
end

it 'returns an empty hash when there are no queries' do
the_case.queries.destroy_all
assert_equal the_case.doc_ratings_by_query, {}
end

it 'returns a hash of doc ratings by query id' do
assert_equal the_case.doc_ratings_by_query, {
488_291_725 => [
{ 'query_id' => 488_291_725, 'doc_id' => '234', 'rating' => nil },
{ 'query_id' => 488_291_725, 'doc_id' => '123', 'rating' => 5.0 }
],
}
end
end
end