Skip to content

Commit 0cbdce4

Browse files
authored
fix: make count filters on a sub-aggregation work correctly. (#660)
A `count` filter translates into an OpenSearch/Elasticsearch query filter against a special `__counts` field which is a flat map containing the counts of each list under the current document context. Our special handling for it worked everywhere except on a sub-aggregation. This fixes it by correctly converting the field path to a nested field path. Fixes #635.
1 parent 94b125d commit 0cbdce4

6 files changed

Lines changed: 64 additions & 6 deletions

File tree

elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/nested_sub_aggregation.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ def nested_path_key
2626
end
2727

2828
def build_agg_hash(filter_interpreter, parent_queries:)
29-
detail = query.build_agg_detail(filter_interpreter, field_path: nested_path, parent_queries: parent_queries)
29+
detail = query.build_agg_detail(filter_interpreter, field_path: nested_path, parent_queries: parent_queries, nested: true)
3030
return {} if detail.nil?
3131

3232
parent_query_names = parent_queries.map(&:name)

elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/query.rb

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,11 @@ def build_agg_hash(filter_interpreter)
5757
build_agg_detail(filter_interpreter, field_path: [], parent_queries: [])&.clauses || {}
5858
end
5959

60-
def build_agg_detail(filter_interpreter, field_path:, parent_queries:)
60+
def build_agg_detail(filter_interpreter, field_path:, parent_queries:, nested: false)
6161
return nil if paginator.desired_page_size.zero? || paginator.paginated_from_singleton_cursor?
6262
queries = parent_queries + [self] # : ::Array[Query]
6363

64-
filter_detail(filter_interpreter, field_path) do
64+
filter_detail(filter_interpreter, field_path, nested: nested) do
6565
grouping_adapter.grouping_detail_for(self) do
6666
Support::HashUtil.disjoint_merge(computations_detail, sub_aggregation_detail(filter_interpreter, queries))
6767
end
@@ -70,8 +70,14 @@ def build_agg_detail(filter_interpreter, field_path:, parent_queries:)
7070

7171
private
7272

73-
def filter_detail(filter_interpreter, field_path)
73+
def filter_detail(filter_interpreter, field_path, nested: false)
7474
filtering_field_path = Filtering::FieldPath.of(field_path.filter_map(&:name_in_index))
75+
76+
# When we're dealing with a nested sub-aggregation, we need to apply the nested transformation
77+
# to the filtering field path to ensure count filters work correctly.
78+
# This is necessary because nested fields create separate document contexts in Elasticsearch/OpenSearch.
79+
filtering_field_path = filtering_field_path.nested if nested
80+
7581
filter_clause = filter_interpreter.build_query([filter].compact, from_field_path: filtering_field_path)
7682

7783
inner_detail = yield

elasticgraph-graphql/sig/elastic_graph/graphql/aggregation/query.rbs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,14 +46,16 @@ module ElasticGraph
4646
def build_agg_detail: (
4747
Filtering::FilterInterpreter,
4848
field_path: ::Array[PathSegment],
49-
parent_queries: ::Array[Query]
49+
parent_queries: ::Array[Query],
50+
?nested: bool
5051
) -> AggregationDetail?
5152

5253
private
5354

5455
def filter_detail: (
5556
Filtering::FilterInterpreter,
56-
::Array[PathSegment]
57+
::Array[PathSegment],
58+
?nested: bool
5759
) { () -> AggregationDetail } -> AggregationDetail
5860

5961
def computations_detail: () -> ::Hash[::String, untyped]

elasticgraph-graphql/spec/acceptance/sub_aggregations_spec.rb

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,14 @@ def test_ungrouped_sub_aggregations
111111
expect(seasons).to eq(count_detail_of(0))
112112
expect(seasons_player_seasons).to eq(count_detail_of(0))
113113

114+
# Demonstrate filtering sub-aggregations on `count`.
115+
seasons, seasons_player_seasons = count_seasons_and_season_player_seasons(seasons: {filter: {players_nested: {count: {gt: 1}}}})
116+
expect(seasons).to eq(count_detail_of(1))
117+
expect(seasons_player_seasons).to eq(count_detail_of(3))
118+
seasons, seasons_player_seasons = count_seasons_and_season_player_seasons(seasons: {filter: {players_object: {count: {lt: 2}}}})
119+
expect(seasons).to eq(count_detail_of(5))
120+
expect(seasons_player_seasons).to eq(count_detail_of(0))
121+
114122
player_count, season_count, season_player_count, season_player_season_count = aggregate_sibling_and_deeply_nested_counts
115123
expect(player_count).to eq(count_detail_of(5))
116124
expect(season_count).to eq(count_detail_of(6))

elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,26 @@ class GraphQL
193193
}]
194194
end
195195

196+
it "allows sub-aggregations to filter on the count of list fields" do
197+
query = aggregation_query_of(name: "teams", sub_aggregations: [
198+
nested_sub_aggregation_of(path_in_index: ["current_players_nested"], query: sub_aggregation_query_of(name: "current_players_nested", filter: {
199+
"seasons_nested" => {LIST_COUNTS_FIELD => {"gt" => 1}}
200+
}))
201+
])
202+
203+
results = search_datastore_aggregations(query, index_def_name: "teams")
204+
205+
expect(results).to eq [{
206+
"doc_count" => 0,
207+
"key" => {},
208+
"teams:current_players_nested" => {
209+
"meta" => outer_meta({"bucket_path" => ["current_players_nested:filtered"]}),
210+
"doc_count" => 5,
211+
"current_players_nested:filtered" => {"doc_count" => 1}
212+
}
213+
}]
214+
end
215+
196216
it "treats empty filters as `true`" do
197217
query = aggregation_query_of(name: "teams", sub_aggregations: [
198218
nested_sub_aggregation_of(path_in_index: ["current_players_nested"], query: sub_aggregation_query_of(name: "current_players_nested", filter: {

elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,28 @@ def self.grouped_field_from(agg_hash)
178178
})
179179
end
180180

181+
it "allows sub-aggregations to filter on the count of list fields" do
182+
query = new_query(aggregations: [aggregation_query_of(name: "teams", sub_aggregations: [
183+
nested_sub_aggregation_of(path_in_index: ["current_players_nested"], query: sub_aggregation_query_of(name: "current_players_nested", filter: {
184+
"seasons_nested" => {LIST_COUNTS_FIELD => {"gt" => 1}}
185+
}))
186+
])])
187+
188+
expect(datastore_body_of(query)).to include_aggs({
189+
"teams:current_players_nested" => {
190+
"meta" => outer_meta({"bucket_path" => ["current_players_nested:filtered"]}),
191+
"nested" => {"path" => "current_players_nested"},
192+
"aggs" => {
193+
"current_players_nested:filtered" => {
194+
"filter" => {
195+
bool: {filter: [{range: {"current_players_nested.#{LIST_COUNTS_FIELD}.seasons_nested" => {gt: 1}}}]}
196+
}
197+
}
198+
}
199+
}
200+
})
201+
end
202+
181203
it "treats empty filters treating as `true`" do
182204
query = new_query(aggregations: [aggregation_query_of(name: "teams", sub_aggregations: [
183205
nested_sub_aggregation_of(path_in_index: ["current_players_nested"], query: sub_aggregation_query_of(name: "current_players_nested", filter: {

0 commit comments

Comments
 (0)