Skip to content

Commit 5d5f88c

Browse files
committed
Only calculate aggregations when requested
Why these changes are being introduced: Aggregations are currently calculated even when they are not requested in the GraphQL query. It would be more efficient to calculate them only when they are needed. Relevant ticket(s): - [USE-491](https://mitlibraries.atlassian.net/browse/USE-491) How this addresses that need: This adds a `requested_aggregations` method to Query Type that evaluates which aggregations are requested in the query. This information is then used in the Aggregations and Opensearch models to calculate only the requested aggregations. Side effects of this change: None.
1 parent e915ecb commit 5d5f88c

5 files changed

Lines changed: 128 additions & 13 deletions

File tree

app/graphql/types/query_type.rb

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,8 @@ def search(searchterm:, citation:, contributors:, funding_information:, geodista
109109
query = construct_query(searchterm, citation, contributors, funding_information, geodistance, geobox, identifiers,
110110
locations, subjects, title, source, boolean_type, filters, per_page, query_mode)
111111

112-
results = Opensearch.new.search(from, query, Timdex::OSClient, highlight: highlight_requested?, index: index, fulltext: fulltext, query_mode: query_mode)
112+
results = Opensearch.new.search(from, query, Timdex::OSClient, highlight: highlight_requested?, index: index,
113+
fulltext: fulltext, query_mode: query_mode, requested_aggregations: requested_aggregations)
113114

114115
response = {}
115116
response[:hits] = results['hits']['total']['value']
@@ -122,6 +123,12 @@ def highlight_requested?
122123
context[:tracers].first.log_data[:used_fields].include?('Record.highlight')
123124
end
124125

126+
def requested_aggregations
127+
used_fields = context[:tracers].first.log_data[:used_fields]
128+
used_fields.select { |field| field.start_with?('Aggregations.') }
129+
.map { |field| field.sub('Aggregations.', '').to_sym }
130+
end
131+
125132
# Long-term, we will probably want to define these fields discretely in RecordType. However, this might end up
126133
# adding confusion while we are maintaining deprecated fields in that class. We should refactor this either soon
127134
# after removing GraphQL V1 or as part of that work.
@@ -174,17 +181,19 @@ def source_deprecation_handler(query, new_source, old_source)
174181
end
175182

176183
def collapse_buckets(es_aggs)
184+
return nil if es_aggs.nil? || es_aggs.empty?
185+
177186
{
178-
access_to_files: es_aggs['access_to_files']['only_file_access']['access_types']['buckets'],
179-
contributors: es_aggs['contributors']['contributor_names']['buckets'],
180-
source: es_aggs['source']['buckets'],
181-
subjects: es_aggs['subjects']['subject_names']['buckets'],
182-
places: es_aggs['places']['only_spatial']['place_names']['buckets'],
183-
languages: es_aggs['languages']['buckets'],
184-
literary_form: es_aggs['literary_form']['buckets'],
185-
format: es_aggs['content_format']['buckets'],
186-
content_type: es_aggs['content_type']['buckets']
187-
}
187+
access_to_files: es_aggs.dig('access_to_files', 'only_file_access', 'access_types', 'buckets'),
188+
contributors: es_aggs.dig('contributors', 'contributor_names', 'buckets'),
189+
source: es_aggs.dig('source', 'buckets'),
190+
subjects: es_aggs.dig('subjects', 'subject_names', 'buckets'),
191+
places: es_aggs.dig('places', 'only_spatial', 'place_names', 'buckets'),
192+
languages: es_aggs.dig('languages', 'buckets'),
193+
literary_form: es_aggs.dig('literary_form', 'buckets'),
194+
format: es_aggs.dig('content_format', 'buckets'),
195+
content_type: es_aggs.dig('content_type', 'buckets')
196+
}.compact
188197
end
189198
end
190199
end

app/models/aggregations.rb

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,18 @@ def self.all
1616
}
1717
end
1818

19+
# Return only the aggregations that were requested
20+
# @param requested_names [Array<Symbol>] Array of aggregation names to include (e.g., [:source, :contributors])
21+
# @return [Hash] Filtered aggregations hash with only requested aggregations
22+
def self.for_request(requested_names)
23+
return {} if requested_names.nil? || requested_names.empty?
24+
25+
all_aggs = all
26+
requested_names.each_with_object({}) do |name, result|
27+
result[name] = all_aggs[name] if all_aggs.key?(name)
28+
end
29+
end
30+
1931
def self.access_to_files
2032
{
2133
nested: {

app/models/opensearch.rb

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,13 @@ class Opensearch
33
SIZE = 20
44
MAX_SIZE = 200
55

6-
def search(from, params, client, highlight: false, index: nil, fulltext: false, query_mode: 'keyword')
6+
def search(from, params, client, highlight: false, index: nil, fulltext: false, query_mode: 'keyword',
7+
requested_aggregations: [])
78
@params = params
89
@highlight = highlight
910
@fulltext = fulltext?(fulltext)
1011
@query_mode = query_mode
12+
@requested_aggregations = requested_aggregations
1113
index = default_index unless index.present?
1214
client.search(index:,
1315
body: build_query(from))
@@ -39,10 +41,13 @@ def build_query(from)
3941
from:,
4042
size: calculate_size,
4143
query:,
42-
aggregations: Aggregations.all,
4344
sort: sort_builder.build
4445
}
4546

47+
# Only include aggregations if any were requested
48+
aggregations = Aggregations.for_request(@requested_aggregations)
49+
query_hash[:aggregations] = aggregations if aggregations.present?
50+
4651
source = source_builder.build
4752
query_hash[:_source] = source if source.present?
4853

test/models/aggregations_test.rb

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
require 'test_helper'
2+
3+
class AggregationsTest < ActiveSupport::TestCase
4+
test 'for_request returns all aggregations when all are requested' do
5+
requested = %i[access_to_files contributors content_type content_format languages literary_form places source
6+
subjects]
7+
result = Aggregations.for_request(requested)
8+
9+
assert_equal requested.size, result.size
10+
requested.each do |agg_name|
11+
assert result.key?(agg_name)
12+
end
13+
end
14+
15+
test 'for_request returns only requested aggregations' do
16+
requested = %i[source contributors]
17+
result = Aggregations.for_request(requested)
18+
19+
assert_equal 2, result.size
20+
assert result.key?(:source)
21+
assert result.key?(:contributors)
22+
end
23+
24+
test 'for_request returns empty hash when given empty array' do
25+
result = Aggregations.for_request([])
26+
27+
assert_empty result
28+
assert result.is_a?(Hash)
29+
end
30+
31+
test 'for_request returns empty hash when given nil' do
32+
result = Aggregations.for_request(nil)
33+
34+
assert_empty result
35+
assert result.is_a?(Hash)
36+
end
37+
38+
test 'for_request ignores invalid aggregation names' do
39+
requested = %i[source invalid_agg contributors another_invalid]
40+
result = Aggregations.for_request(requested)
41+
42+
assert_equal 2, result.size
43+
assert result.key?(:source)
44+
assert result.key?(:contributors)
45+
assert_not result.key?(:invalid_agg)
46+
assert_not result.key?(:another_invalid)
47+
end
48+
end

test/models/opensearch_test.rb

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,4 +161,45 @@ class OpensearchTest < ActiveSupport::TestCase
161161
result = os.query
162162
assert_equal(mock_response, result)
163163
end
164+
165+
test 'build_query includes aggregations when requested' do
166+
os = Opensearch.new
167+
os.instance_variable_set(:@params, { q: 'test' })
168+
os.instance_variable_set(:@requested_aggregations, %i[source contributors])
169+
170+
json = JSON.parse(os.build_query(0))
171+
assert json.key?('aggregations')
172+
assert json['aggregations'].key?('source')
173+
assert json['aggregations'].key?('contributors')
174+
assert_not json['aggregations'].key?('languages')
175+
end
176+
177+
test 'build_query excludes aggregations when none requested' do
178+
os = Opensearch.new
179+
os.instance_variable_set(:@params, { q: 'test' })
180+
os.instance_variable_set(:@requested_aggregations, [])
181+
182+
json = JSON.parse(os.build_query(0))
183+
assert_not json.key?('aggregations')
184+
end
185+
186+
test 'build_query excludes aggregations when requested_aggregations is nil' do
187+
os = Opensearch.new
188+
os.instance_variable_set(:@params, { q: 'test' })
189+
os.instance_variable_set(:@requested_aggregations, nil)
190+
191+
json = JSON.parse(os.build_query(0))
192+
assert_not json.key?('aggregations')
193+
end
194+
195+
test 'build_query with aggregations ignores invalid aggregation names' do
196+
os = Opensearch.new
197+
os.instance_variable_set(:@params, { q: 'test' })
198+
os.instance_variable_set(:@requested_aggregations, %i[source invalid_agg])
199+
200+
json = JSON.parse(os.build_query(0))
201+
assert json.key?('aggregations')
202+
assert json['aggregations'].key?('source')
203+
assert_not json['aggregations'].key?('invalid_agg')
204+
end
164205
end

0 commit comments

Comments
 (0)