From fe7c8f6b85b08e73f5110962c789e5c749d8ae35 Mon Sep 17 00:00:00 2001 From: myronmarston-toast Date: Fri, 7 Aug 2026 18:03:07 -0700 Subject: [PATCH] PR-1327 followups: drop dead require, tighten percentile test, hoist expect_errors return, drop internal-abstraction spec - rm unused `require "graphql"` in query_adapter.rb (leftover from removed rescue) - percentile_of now brackets [floor,ceil] of nearest-rank instead of full [min,max] - move `return response if expect_errors` into the if-branch in 2 specs - delete function_adapter_spec.rb (tests internal abstraction, not public API); 100% coverage confirmed after removal --- .../graphql/aggregation/query_adapter.rb | 1 - .../spec/acceptance/aggregations_spec.rb | 22 ++-- .../aggregation/function_adapter_spec.rb | 117 ------------------ .../resolvers/aggregation_resolver_support.rb | 5 +- 4 files changed, 17 insertions(+), 128 deletions(-) delete mode 100644 elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/function_adapter_spec.rb 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 531d89820..ce30c8ff1 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/query_adapter.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/query_adapter.rb @@ -17,7 +17,6 @@ require "elastic_graph/graphql/schema/arguments" require "elastic_graph/support/hash_util" require "elastic_graph/support/memoizable_data" -require "graphql" module ElasticGraph class GraphQL diff --git a/elasticgraph-graphql/spec/acceptance/aggregations_spec.rb b/elasticgraph-graphql/spec/acceptance/aggregations_spec.rb index de5e9be6b..abc4d31b1 100644 --- a/elasticgraph-graphql/spec/acceptance/aggregations_spec.rb +++ b/elasticgraph-graphql/spec/acceptance/aggregations_spec.rb @@ -935,18 +935,26 @@ def expected_aggregated_amounts_of(*raw_values) # `p50`/`p75` (unlike p0/p100) can't be satisfied by an implementation that only supports # min/max, so they're what actually proves full percentile support works. We can't pin an # exact expected value here: different datastore versions/backends use different (equally - # valid) interpolation conventions for a rank that doesn't land exactly on one data point, so - # we only assert it's a real number within [min, max] (a property every backend guarantees). - "p50" => percentile_of(min, max), - "p75" => percentile_of(min, max), + # valid) interpolation conventions for a rank that doesn't land exactly on one data point. + # Both the nearest-rank method (OpenSearch) and linear interpolation (Elasticsearch) are + # guaranteed to land between the two order statistics bracketing that rank, so `percentile_of` + # narrows the expectation to that bracket instead of the full `[min, max]` range. + "p50" => percentile_of(raw_values, 50), + "p75" => percentile_of(raw_values, 75), case_correctly("exact_min") => int_of(min), case_correctly("exact_max") => int_of(max) } end - def percentile_of(min, max) - return nil if min.nil? - (be >= min).and(be <= max).and a_kind_of(::Float) + def percentile_of(raw_values, percent) + return nil if raw_values.empty? + + sorted = raw_values.sort + rank = (percent / 100.0) * (sorted.size - 1) + lower_bound = sorted[rank.floor] + upper_bound = sorted[rank.ceil] + + (be >= lower_bound).and(be <= upper_bound).and a_kind_of(::Float) end def verify_all_timestamp_groupings_valid(widget_id, truncation_unit_type:, field:) diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/function_adapter_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/function_adapter_spec.rb deleted file mode 100644 index d2e69bed1..000000000 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/function_adapter_spec.rb +++ /dev/null @@ -1,117 +0,0 @@ -# Copyright 2024 - 2026 Block, Inc. -# -# Use of this source code is governed by an MIT-style -# license that can be found in the LICENSE file or at -# https://opensource.org/licenses/MIT. -# -# frozen_string_literal: true - -require "elastic_graph/graphql/aggregation/function_adapter" -require "elastic_graph/schema_artifacts/runtime_metadata/schema_element_names" - -module ElasticGraph - class GraphQL - module Aggregation - RSpec.describe FunctionAdapter do - describe "BY_NAME" do - it "registers an adapter for each supported aggregated value function" do - expect(FunctionAdapter::BY_NAME.keys).to contain_exactly(:avg, :cardinality, :max, :min, :percentile, :sum) - end - - it "maps each function to its datastore aggregation name" do - expect(FunctionAdapter::BY_NAME.transform_values(&:datastore_function_name)).to eq( - avg: "avg", - cardinality: "cardinality", - max: "max", - min: "min", - percentile: "percentiles", - sum: "sum" - ) - end - - it "fabricates the empty bucket response each function returns when the datastore has no documents to aggregate" do - expect(FunctionAdapter::BY_NAME.transform_values(&:empty_bucket_result)).to eq( - avg: {"value" => nil}, - cardinality: {"value" => 0}, - max: {"value" => nil}, - min: {"value" => nil}, - percentile: {"values" => [{"value" => nil}]}, - sum: {"value" => 0} - ) - end - end - - describe FunctionAdapter::SimpleMetric do - let(:element_names) { SchemaArtifacts::RuntimeMetadata::SchemaElementNames.new(form: :snake_case, overrides: {}) } - let(:adapter) { FunctionAdapter::SimpleMetric.new(datastore_function_name: "avg", empty_bucket_value: nil) } - - it "exposes the datastore function name it was configured with" do - expect(adapter.datastore_function_name).to eq "avg" - end - - it "extracts no args, since these functions take none" do - expect(adapter.extract_args({"some_arg" => 3}, element_names) { |msg| raise msg }).to eq({}) - end - - it "contributes no extra clause options beyond the `field` the clause builder provides" do - expect(adapter.clause_options({})).to eq({}) - end - - it "returns the raw datastore response as the value hash, since these functions return a flat response" do - expect(adapter.extract_result({"value" => 3.7})).to eq({"value" => 3.7}) - end - - it "fabricates an empty bucket response using the configured empty bucket value" do - expect(adapter.empty_bucket_result).to eq({"value" => nil}) - expect(FunctionAdapter::SimpleMetric.new(datastore_function_name: "sum", empty_bucket_value: 0).empty_bucket_result).to eq({"value" => 0}) - end - end - - describe FunctionAdapter::Percentile do - let(:element_names) { SchemaArtifacts::RuntimeMetadata::SchemaElementNames.new(form: :snake_case, overrides: {}) } - let(:adapter) { FunctionAdapter::Percentile.new(datastore_function_name: "percentiles") } - - it "exposes the datastore function name it was configured with" do - expect(adapter.datastore_function_name).to eq "percentiles" - end - - it "extracts the requested percentile from the args" do - expect(extract_args({"percentile" => 50.0})).to eq({percentile: 50.0}) - end - - it "accepts the boundary values 0 and 100" do - expect(extract_args({"percentile" => 0})).to eq({percentile: 0}) - expect(extract_args({"percentile" => 100})).to eq({percentile: 100}) - end - - it "yields an error message--rather than raising--when the requested percentile is outside 0 to 100" do - expect(extract_args({"percentile" => -1})).to eq "`percentile` must be between 0 and 100, but is -1." - expect(extract_args({"percentile" => 150})).to eq "`percentile` must be between 0 and 100, but is 150." - end - - it "yields an error message when the requested percentile is not numeric" do - expect(extract_args({"percentile" => "50"})).to eq '`percentile` must be between 0 and 100, but is "50".' - end - - it "requests a single unkeyed percentile in the clause options" do - expect(adapter.clause_options({percentile: 99.9})).to eq({"percents" => [99.9], "keyed" => false}) - end - - it "extracts the single value from the datastore's array-shaped response" do - expect(adapter.extract_result({"values" => [{"key" => 50.0, "value" => 3.7}]})).to eq({"key" => 50.0, "value" => 3.7}) - end - - it "fabricates an empty bucket response with a nil value" do - expect(adapter.empty_bucket_result).to eq({"values" => [{"value" => nil}]}) - end - - # Returns the extracted args, or--when the args are invalid--the yielded error message, - # so that a single expectation can cover either outcome. - def extract_args(args) - adapter.extract_args(args, element_names) { |message| return message } - end - end - end - end - end -end diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/aggregation_resolver_support.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/aggregation_resolver_support.rb index 8d0c2b07f..853849cd3 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/aggregation_resolver_support.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/aggregation_resolver_support.rb @@ -31,12 +31,11 @@ def resolve_target_nodes( if expect_errors expect(response["errors"]).not_to be_empty + response else expect(response["errors"]).to eq([]).or eq(nil) + response.dig(*path) end - - return response if expect_errors - response.dig(*path) end def datastore_response_payload_with_aggs(aggregations, hit_count)