From ed95d906b3d8bd686caf63d4e16744014ad9d109 Mon Sep 17 00:00:00 2001 From: myronmarston-toast Date: Tue, 4 Aug 2026 22:42:07 -0700 Subject: [PATCH] Key aggregated value aggregations off the GraphQL alias (#1329) computed_index_field_name only used name_in_index for the aggregated value function's leaf, while every parent path segment already used alias-aware name_in_graphql_query. That's what forced an argument- bearing function (upcoming approximatePercentile) to invent a synthetic leaf name built independently on the query-building and resolver sides, which then had to agree byte-for-byte. Computation now carries a `leaf` PathSegment built via the same PathSegment.for factory on both sides, so the leaf key derives from the alias like every other segment. This removes the need for synthetic key naming for argument-bearing functions entirely. Behavior change (not a pure refactor): two aliases of the same function under one field used to collapse into a single computation (equal value objects, same key). Now their leaf segments differ, so two identical datastore aggregations are sent -- correct since each field resolves via its own alias. Deduping by clause content would require threading an alias-to-canonical-key map from query building into the resolver, reintroducing the coupling this change removes. Prep refactor #1 of 2 for PR #1327; the percentile function itself is not added here. --- .../graphql/aggregation/computation.rb | 7 ++-- .../graphql/aggregation/query_adapter.rb | 4 +-- .../resolvers/aggregated_values.rb | 2 +- .../graphql/aggregation/computation.rbs | 6 ++-- .../spec/acceptance/aggregations_spec.rb | 15 +++++++++ .../spec/support/aggregations_helpers.rb | 4 +-- .../graphql/aggregation/computation_spec.rb | 20 +++++++++++ .../graphql/aggregation/query_adapter_spec.rb | 14 +++++--- .../resolvers/aggregations_spec.rb | 33 +++++++++++++++++++ 9 files changed, 89 insertions(+), 16 deletions(-) diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/computation.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/computation.rb index dd8e5b938..d577a390c 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/computation.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/computation.rb @@ -6,8 +6,9 @@ # # frozen_string_literal: true -require "elastic_graph/graphql/aggregation/key" require "elastic_graph/graphql/aggregation/field_path_encoder" +require "elastic_graph/graphql/aggregation/key" +require "elastic_graph/graphql/aggregation/path_segment" module ElasticGraph class GraphQL @@ -18,14 +19,14 @@ module Aggregation # https://www.elastic.co/guide/en/elasticsearch/reference/7.12/search-aggregations-metrics-max-aggregation.html # https://www.elastic.co/guide/en/elasticsearch/reference/7.12/search-aggregations-metrics-min-aggregation.html # https://www.elastic.co/guide/en/elasticsearch/reference/7.12/search-aggregations-metrics-sum-aggregation.html - Computation = ::Data.define(:source_field_path, :computed_index_field_name, :detail) do + Computation = ::Data.define(:source_field_path, :leaf, :detail) do # @implements Computation def key(aggregation_name:) Key::AggregatedValue.new( aggregation_name: aggregation_name, field_path: source_field_path.map(&:name_in_graphql_query), - function_name: computed_index_field_name + function_name: leaf.name_in_graphql_query ).encode end diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/query_adapter.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/query_adapter.rb index 16130667c..fe62a4bf2 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/query_adapter.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/query_adapter.rb @@ -188,11 +188,11 @@ def build_computations_from(node_node, from_field_path: []) get_children_nodes(node).map do |fn_node| computed_field = field_from_node(fn_node) - computation_detail = field_from_node(fn_node).computation_detail # : SchemaArtifacts::RuntimeMetadata::ComputationDetail + computation_detail = computed_field.computation_detail # : SchemaArtifacts::RuntimeMetadata::ComputationDetail Aggregation::Computation.new( source_field_path: field_path, - computed_index_field_name: computed_field.name_in_index, + leaf: PathSegment.for(field: computed_field, lookahead: fn_node), detail: computation_detail ) end diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/resolvers/aggregated_values.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/resolvers/aggregated_values.rb index 1ef2735ea..2df1cdb53 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/resolvers/aggregated_values.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/resolvers/aggregated_values.rb @@ -21,7 +21,7 @@ def resolve(field:, object:, args:, context:, lookahead:) key = Key::AggregatedValue.new( aggregation_name: aggregation_name, field_path: field_path.map(&:name_in_graphql_query), - function_name: field.name_in_index + function_name: PathSegment.for(field: field, lookahead: lookahead).name_in_graphql_query ) result = Support::HashUtil.verbose_fetch(bucket, key.encode) diff --git a/elasticgraph-graphql/sig/elastic_graph/graphql/aggregation/computation.rbs b/elasticgraph-graphql/sig/elastic_graph/graphql/aggregation/computation.rbs index bf198c54d..50825a14e 100644 --- a/elasticgraph-graphql/sig/elastic_graph/graphql/aggregation/computation.rbs +++ b/elasticgraph-graphql/sig/elastic_graph/graphql/aggregation/computation.rbs @@ -3,7 +3,7 @@ module ElasticGraph module Aggregation class Computation attr_reader source_field_path: fieldPath - attr_reader computed_index_field_name: ::String + attr_reader leaf: PathSegment attr_reader detail: SchemaArtifacts::RuntimeMetadata::ComputationDetail def key: (aggregation_name: ::String) -> ::String @@ -11,13 +11,13 @@ module ElasticGraph def initialize : ( source_field_path: fieldPath, - computed_index_field_name: ::String, + leaf: PathSegment, detail: SchemaArtifacts::RuntimeMetadata::ComputationDetail ) -> void def with: ( ?source_field_path: fieldPath, - ?computed_index_field_name: ::String, + ?leaf: PathSegment, ?detail: SchemaArtifacts::RuntimeMetadata::ComputationDetail ) -> instance end diff --git a/elasticgraph-graphql/spec/acceptance/aggregations_spec.rb b/elasticgraph-graphql/spec/acceptance/aggregations_spec.rb index 9672bab16..2429639b3 100644 --- a/elasticgraph-graphql/spec/acceptance/aggregations_spec.rb +++ b/elasticgraph-graphql/spec/acceptance/aggregations_spec.rb @@ -151,6 +151,21 @@ module ElasticGraph size_uniq_count = widget_ungrouped_aggregated_values_for("size { approximate_distinct_value_count }") expect(size_uniq_count).to eq({"size" => {case_correctly("approximate_distinct_value_count") => 2}}) + # Verify that two aliases of the same aggregated value function under one field each resolve correctly, + # rather than colliding on a single datastore aggregation. + aliased_mins = widget_ungrouped_aggregated_values_for(<<~QUERY) + #{amount_cents} { + #{case_correctly("exact_min")} + #{case_correctly("my_min")}: #{case_correctly("exact_min")} + } + QUERY + expect(aliased_mins).to eq({ + amount_cents => { + case_correctly("exact_min") => 100, + case_correctly("my_min") => 100 + } + }) + aggregations = group_widget_currencies_by_widget_name expect(aggregations).to eq [ {"count" => 1, case_correctly("grouped_by") => {case_correctly("widget_name") => "w100"}}, diff --git a/elasticgraph-graphql/spec/support/aggregations_helpers.rb b/elasticgraph-graphql/spec/support/aggregations_helpers.rb index cfec7dc72..90156c2cc 100644 --- a/elasticgraph-graphql/spec/support/aggregations_helpers.rb +++ b/elasticgraph-graphql/spec/support/aggregations_helpers.rb @@ -21,12 +21,12 @@ module ElasticGraph module AggregationsHelpers - def computation_of(*field_names_in_index, function, computed_field_name: function.to_s, field_names_in_graphql_query: field_names_in_index) + def computation_of(*field_names_in_index, function, computed_field_name: function.to_s, leaf_alias: computed_field_name, field_names_in_graphql_query: field_names_in_index) source_field_path = build_field_path(names_in_index: field_names_in_index, names_in_graphql_query: field_names_in_graphql_query) GraphQL::Aggregation::Computation.new( source_field_path: source_field_path, - computed_index_field_name: computed_field_name, + leaf: GraphQL::Aggregation::PathSegment.new(name_in_graphql_query: leaf_alias, name_in_index: computed_field_name), detail: SchemaArtifacts::RuntimeMetadata::ComputationDetail.new( function: function, empty_bucket_value: (function == :sum || function == :cardinality) ? 0 : nil diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/computation_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/computation_spec.rb index a191c21df..2304f891a 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/computation_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/computation_spec.rb @@ -27,6 +27,20 @@ module Aggregation expect(computation.key(aggregation_name: "my_aggs")).to eq aggregated_value_key_of("oof", "rab", "average", aggregation_name: "my_aggs").encode end + + it "uses the leaf's GraphQL query alias rather than its name in the index" do + computation = computation_of("foo", "bar", :min, computed_field_name: "exact_min", leaf_alias: "myMin") + + expect(computation.key(aggregation_name: "my_aggs")).to eq aggregated_value_key_of("foo", "bar", "myMin", aggregation_name: "my_aggs").encode + end + + it "differs between two aliases of the same function under the same field, so they don't collapse into one computation" do + computation = computation_of("foo", :min, computed_field_name: "exact_min", leaf_alias: "exactMin") + aliased_computation = computation_of("foo", :min, computed_field_name: "exact_min", leaf_alias: "myMin") + + expect(computation).not_to eq(aliased_computation) + expect(computation.key(aggregation_name: "my_aggs")).not_to eq(aliased_computation.key(aggregation_name: "my_aggs")) + end end describe "#clause" do @@ -47,6 +61,12 @@ module Aggregation expect(computation.clause).to eq({"avg" => {"field" => "foo.c.bar.d"}}) end + + it "is unaffected by an alias on the leaf" do + computation = computation_of("foo", "bar", :avg, computed_field_name: "approximate_avg", leaf_alias: "myAvg") + + expect(computation.clause).to eq({"avg" => {"field" => "foo.bar"}}) + end end end end diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/query_adapter_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/query_adapter_spec.rb index b9e6627c5..b33d128c9 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/query_adapter_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/query_adapter_spec.rb @@ -1440,7 +1440,11 @@ def build_sub_aggregation_query(args, default_page_size:, max_page_size:) name: "widget_aggregations", computations: [ computation_of("amount_cents", :avg, computed_field_name: "approximate_avg", field_names_in_graphql_query: ["ac1"]), - computation_of("amount_cents", :avg, computed_field_name: "approximate_avg", field_names_in_graphql_query: ["ac2"]), + # `aa1` and `aa2` are two aliases of the same function under the same field. Since the leaf key is + # derived from the alias, these produce two distinct computations (and, ultimately, two identical + # datastore aggregation clauses) rather than collapsing into one. + computation_of("amount_cents", :avg, computed_field_name: "approximate_avg", leaf_alias: "aa1", field_names_in_graphql_query: ["ac2"]), + computation_of("amount_cents", :avg, computed_field_name: "approximate_avg", leaf_alias: "aa2", field_names_in_graphql_query: ["ac2"]), computation_of("amount_cents", :sum, computed_field_name: "exact_sum", field_names_in_graphql_query: ["ac2"]) ] )]) @@ -1793,8 +1797,8 @@ def build_sub_aggregation_query(args, default_page_size:, max_page_size:) expect(aggregations).to eq([aggregation_query_of( name: "wa", computations: [ - computation_of("amount_cents", :avg, computed_field_name: "approximate_avg", field_names_in_graphql_query: ["ac"]), - computation_of("amount_cents", :max, computed_field_name: "exact_max", field_names_in_graphql_query: ["ac"]) + computation_of("amount_cents", :avg, computed_field_name: "approximate_avg", leaf_alias: "aa", field_names_in_graphql_query: ["ac"]), + computation_of("amount_cents", :max, computed_field_name: "exact_max", leaf_alias: "em", field_names_in_graphql_query: ["ac"]) ], groupings: [ field_term_grouping_of("size", field_names_in_graphql_query: ["s"]), @@ -1900,8 +1904,8 @@ def build_sub_aggregation_query(args, default_page_size:, max_page_size:) expect(aggregations).to eq([aggregation_query_of( name: "wa", computations: [ - computation_of("amount_cents", :avg, computed_field_name: "approximate_avg", field_names_in_graphql_query: ["ac"]), - computation_of("amount_cents", :max, computed_field_name: "exact_max", field_names_in_graphql_query: ["ac"]) + computation_of("amount_cents", :avg, computed_field_name: "approximate_avg", leaf_alias: "aa", field_names_in_graphql_query: ["ac"]), + computation_of("amount_cents", :max, computed_field_name: "exact_max", leaf_alias: "em", field_names_in_graphql_query: ["ac"]) ], groupings: [ field_term_grouping_of("size", field_names_in_graphql_query: ["s"]), diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/aggregations_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/aggregations_spec.rb index 8c6b731b1..59040b0b4 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/aggregations_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/aggregations_spec.rb @@ -284,6 +284,39 @@ module Aggregation ] end + it "uses GraphQL field aliases when resolving the aggregation function leaf fields, allowing the same function to be requested multiple times under one field" do + aggs = { + aggregated_value_key_of("amount_cents", "es") => {"value" => 900.0}, + aggregated_value_key_of("amount_cents", "exact_sum") => {"value" => 900.0}, + aggregated_value_key_of("cost", "amount_cents", "mx") => {"value" => 400.0} + } + + response = resolve_target_nodes(<<~QUERY, aggs: aggs) + target: widget_aggregations { + nodes { + aggregated_values { + amount_cents { + es: exact_sum + exact_sum + } + cost { + amount_cents { mx: exact_max } + } + } + } + } + QUERY + + expect(response).to eq [ + { + "aggregated_values" => { + "amount_cents" => {"es" => 900, "exact_sum" => 900}, + "cost" => {"amount_cents" => {"mx" => 400}} + } + } + ] + end + it "resolves aggregated Date/DateTime/LocalTime values" do aggs = { aggregated_value_key_of("created_at", "exact_min") => {"value" => 1696854612000.0, "value_as_string" => "2023-10-09T12:30:12.000Z"},