Add approximatePercentile aggregated value function by nikhilkumarjadhav-toast · Pull Request #1327 · block/elasticgraph

nikhilkumarjadhav-toast · 2026-08-04T16:46:21Z

Summary

Adds an approximatePercentile field to the aggregatedValues API for Float, Int, JsonSafeLong, LongString, Date, DateTime, and LocalTime fields, backed by the datastore's percentiles aggregation.

type FloatAggregatedValues {
  approximateAvg: Float
  approximateSum: Float!
  exactMin: Float
  exactMax: Float
  approximatePercentile(percentile: Float!): Float
}

Callers request a specific percentile rank via the percentile argument (e.g. percentile: 50 for the median), and can request multiple ranks in a single query by aliasing the field selection:

{
  amountCents {
    p50: approximatePercentile(percentile: 50)
    p99: approximatePercentile(percentile: 99)
  }
}

This shape (as opposed to a list-return shape accepting an array of percentiles) keeps query validity statically verifiable: each aliased selection independently validates its own rank and always returns exactly one value, avoiding the ambiguity a list-argument API would have around duplicate or out-of-range requests. Percentiles are computed using an approximate algorithm (t-digest) regardless of field type, so there's no exactPercentile counterpart the way there is for min/max/sum.

@myronmarston and I discussed and agreed on this API shape beforehand.

Implementation notes

ComputationDetail gains an optional function_arg_name, and Computation gains a function_arg_value, so a function that needs a query-time argument (like percentile) can fold the resolved value into both the emitted datastore clause and the aggregation key. This avoids key collisions between multiple aliased selections of the same field with different argument values.
The datastore's percentiles aggregation is requested with keyed: false and a single-element percents array per Computation, so the response is a small array ("values" => [{"key" => ..., "value" => ...}]) rather than a string-keyed hash — this avoids reconstructing the datastore's float-formatted string key to look up the single requested value.
Verified end-to-end against a running datastore (both a Float/Int field and a DateTime field, confirming value_as_string formatting carries through the same as the other aggregated value functions) before writing the automated tests.

Test plan

Unit tests: Computation#key/#clause for the percentiles function; QueryAdapter builds distinct computations for aliased selections with different percentile args; resolver tests including empty-bucket (null) and DateTime formatting
Integration test against a real datastore: median/p99 over a small dataset, plus the empty-result-set case
Acceptance tests: aliased multi-percentile query, and percentile alongside groupedBy
Extended existing built_in_types_spec.rb/for_built_in_types_spec.rb SDL and runtime-metadata assertions for the new field on all 7 supported types
script/quick_build passes (spellcheck, lint, type_check, schema_artifacts:check, full spec suite at 100% line/branch coverage, site validation)
Added a validated example query (config/site/examples/music) and doc-site prose in query-api/aggregations/aggregated-values.md

CLAassistant · 2026-08-04T16:46:29Z

All committers have signed the CLA.

myronmarston

Great work @nikhilkumarjadhav-toast! As noted in my comments below, I'm working on #1331 and #1332 which should make this much easier. You'll want to rebase on top of that and rework your solution after those land.

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.

Aggregated value functions were treated uniformly only because every one of them takes no arguments and returns a flat {"value" => ...} response. A function needing a request argument and returning a nested response has nowhere for that knowledge to live, so it would leak as `if function == :percentiles` conditionals across the clause builder, the resolver, and the empty-bucket builder. Each function now routes through an adapter owning all of its datastore-specific behavior: datastore aggregation name, GraphQL argument extraction, extra clause options, reading the value out of the response, and fabricating a response for a bucket the datastore omitted. Adding a function becomes a registry entry plus one adapter. Decoupling the metadata name from the datastore aggregation name is what lets the two diverge. Fields resolve the name to an adapter once at boot (fields are built once and cached), so an unregistered name fails at boot rather than mid-query, and the registry has one query-time reader. Arguments flow in two phases because argument names are customizable via schema element names, which the computation value object cannot see when building a clause: query building calls extract_args at the boundary, storing a canonically-keyed hash; clause building later calls the pure clause_options. Threading element names onto the computation instead would pollute its identity, which is used to dedupe computations in a Set. The empty-bucket value moves from per-field metadata into the registry. It is fully derivable from the function (verified across all 45 aggregated value fields), so per-field storage was redundant and let a schema-definition author pair a function with a wrong empty value. The adapter returns the entire fabricated response rather than a bare value the builder must wrap, removing a read-path/write-path asymmetry. The argument plumbing ships now even though no function uses it yet: the point of this prep work is that the framework is ready, and deferring it would leave the follow-up changing the adapter interface itself. `computes :sum` reads as part of the field DSL, which is otherwise unprefixed. It validates against the registry at dump time, mirroring how elasticgraph-schema_definition already depends on elasticgraph-graphql to validate scalar coercion adapters; a mistyped name is a plausible slip. Prep refactor #2 of 2 for PR #1327; the percentile function itself is not added here. Closes #1330.

myronmarston

Another thing that I thought of after submitting my review above: we should make sure that out-of-range percentiles are handled properly. I checked out your branch and ran bundle exec rake boot_locally to try it out and found that percentile values out side the 0 to 100 range cause OpenSearch/Elasticsearch to throw exceptions:

We should return a GraphQL validation error instead. (Would be good to cover that in the acceptance test).

Aggregated value functions were treated uniformly only because every one of them takes no arguments and returns a flat {"value" => ...} response. A function needing a request argument and returning a nested response has nowhere for that knowledge to live, so it would leak as `if function == :percentiles` conditionals across the clause builder, the resolver, and the empty-bucket builder. Each function now routes through an adapter owning all of its datastore-specific behavior: datastore aggregation name, GraphQL argument extraction, extra clause options, reading the value out of the response, and fabricating a response for a bucket the datastore omitted. Adding a function becomes a registry entry plus one adapter. Decoupling the metadata name from the datastore aggregation name is what lets the two diverge. Fields resolve the name to an adapter once at boot (fields are built once and cached), so an unregistered name fails at boot rather than mid-query, and the registry has one query-time reader. Arguments flow in two phases because argument names are customizable via schema element names, which the computation value object cannot see when building a clause: query building calls extract_args at the boundary, storing a canonically-keyed hash; clause building later calls the pure clause_options. Threading element names onto the computation instead would pollute its identity, which is used to dedupe computations in a Set. The empty-bucket value moves from per-field metadata into the registry. It is fully derivable from the function (verified across all 45 aggregated value fields), so per-field storage was redundant and let a schema-definition author pair a function with a wrong empty value. The adapter returns the entire fabricated response rather than a bare value the builder must wrap, removing a read-path/write-path asymmetry. The argument plumbing ships now even though no function uses it yet: the point of this prep work is that the framework is ready, and deferring it would leave the follow-up changing the adapter interface itself. `computes :sum` reads as part of the field DSL, which is otherwise unprefixed. It validates against the registry at dump time, mirroring how elasticgraph-schema_definition already depends on elasticgraph-graphql to validate scalar coercion adapters; a mistyped name is a plausible slip. Prep refactor #2 of 2 for PR #1327; the percentile function itself is not added here. Closes #1330.

## Summary Prep refactor #1 of 2 for PR #1327 (`approximatePercentile`). The percentile function itself is not added here. An aggregated value function field that is aliased in a GraphQL query now resolves through a datastore aggregation key derived from that alias, rather than from the field's `name_in_index`. Previously, the leaf segment of an aggregated value key was the only path segment keyed off `name_in_index` -- every parent segment already used the alias-aware `name_in_graphql_query`. That inconsistency would have forced an argument-bearing function (like the upcoming `approximatePercentile`) to invent a synthetic leaf name (e.g. `approximate_percentile(50.0)`), built independently in the query-building code and the resolver, which would then have to agree byte-for-byte. Making the leaf alias-derived removes the need for synthetic key naming entirely. - `Computation` replaces its `computed_index_field_name` string attribute with a `leaf` attribute holding a `PathSegment`. Its `name_in_index` is intentionally unused -- the function name isn't part of the datastore index path, which `clause` derives entirely from `source_field_path`. - Both the query-building side (`QueryAdapter`) and the resolver side (`Resolvers::AggregatedValues`) now derive the leaf name through the same `PathSegment.for` factory, so there's one rule instead of two implementations that must agree. - `computed_index_field_name` is deleted (not left unused), since the datastore clause already derives its index path from `source_field_path`. ## Behavior change This is **not** a pure refactor. Two aliases of the same function under one field used to collapse into a single computation (the value objects were equal, held in a `Set`, with an identical key). After this change their leaf segments differ, so two identical datastore aggregations are sent: ```graphql aggregatedValues { amount { exactMin, myMin: exactMin } } # before: ONE agg clause; both fields read it # after: TWO identical agg clauses, keyed by `exactMin` and `myMin` ``` Correct in both cases -- each field resolves via its own alias. Accepted deliberately: - The redundancy only occurs when a client asks for the same value twice under two names, which is pathological, and the cost is a duplicate metric aggregation on an already-loaded shard. - Deduplicating by clause content would require threading an alias-to-canonical-key map from query building into the resolver, reintroducing the coupling this ticket exists to delete. - Rejecting aliases outright is a non-starter: the percentile function requires aliases to request multiple ranks. ## Test plan - [x] Unit test: an aliased aggregated value function field produces a key built from the alias - [x] Unit test: two aliases of the same function under one field produce two distinct keys - [x] Unit test: the datastore clause's index field path is unaffected by leaf aliasing - [x] Query-building unit test covering an aliased function field - [x] Acceptance test issuing a GraphQL query with an aliased aggregated value function and asserting the resolved value - [x] RBS signatures updated; `script/type_check` passes - [x] `script/run_gem_specs elasticgraph-graphql` passes (100% line/branch coverage maintained) - [x] `script/quick_build` passes Closes #1329 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Aggregated value functions were treated uniformly only because every one of them takes no arguments and returns a flat {"value" => ...} response. A function needing a request argument and returning a nested response has nowhere for that knowledge to live, so it would leak as `if function == :percentiles` conditionals across the clause builder, the resolver, and the empty-bucket builder. Each function now routes through an adapter owning all of its datastore-specific behavior: datastore aggregation name, GraphQL argument extraction, extra clause options, reading the value out of the response, and fabricating a response for a bucket the datastore omitted. Adding a function becomes a registry entry plus one adapter. Decoupling the metadata name from the datastore aggregation name is what lets the two diverge. Fields resolve the name to an adapter once at boot (fields are built once and cached), so an unregistered name fails at boot rather than mid-query, and the registry has one query-time reader. Arguments flow in two phases because argument names are customizable via schema element names, which the computation value object cannot see when building a clause: query building calls extract_args at the boundary, storing a canonically-keyed hash; clause building later calls the pure clause_options. Threading element names onto the computation instead would pollute its identity, which is used to dedupe computations in a Set. The empty-bucket value moves from per-field metadata into the registry. It is fully derivable from the function (verified across all 45 aggregated value fields), so per-field storage was redundant and let a schema-definition author pair a function with a wrong empty value. The adapter returns the entire fabricated response rather than a bare value the builder must wrap, removing a read-path/write-path asymmetry. The argument plumbing ships now even though no function uses it yet: the point of this prep work is that the framework is ready, and deferring it would leave the follow-up changing the adapter interface itself. `computes :sum` reads as part of the field DSL, which is otherwise unprefixed. It validates against the registry at dump time, mirroring how elasticgraph-schema_definition already depends on elasticgraph-graphql to validate scalar coercion adapters; a mistyped name is a plausible slip. Prep refactor #2 of 2 for PR #1327; the percentile function itself is not added here. Closes #1330.

Prep refactor #2 of 2 paving the way for #1327 (`approximatePercentile`). This does **not** add the percentile function — it makes the framework ready for it. Closes #1330. Stacked on #1331 — review that one first. ## Why ElasticGraph treats aggregated value functions uniformly, expressing differences via runtime metadata rather than special casing. That works only while every function takes no arguments and returns a flat `{"value" => ...}` response. A function that needs a request argument and returns a nested response has nowhere for that knowledge to live, so it leaks as `if function == :percentiles` conditionals across three files: the clause builder, the resolver, and the empty-bucket builder. After this change, adding a function is a registry entry plus one adapter — no changes to any of those three. ## The adapter interface | Method | Responsibility | |---|---| | `datastore_function_name` | The datastore's aggregation type (may differ from the ElasticGraph function name) | | `extract_args(args, element_names)` | GraphQL args → canonically-keyed args hash | | `clause_options(function_args)` | Extra keys merged into the aggregation clause alongside `field` | | `extract_result(raw)` | Locate the value hash within the datastore's response | | `empty_bucket_result` | The complete fabricated response for a bucket the datastore omitted | All five existing functions differ only in datastore name and empty-bucket value, so one `SimpleMetric` data class parameterized on those two covers all of them; its other three methods are no-ops. A function whose behavior isn't a parameterization of anything would register as a singleton module instead. ## Key decisions - **Metadata names the adapter; the adapter owns the datastore aggregation name.** Breaking that coupling lets the two diverge, which the percentile function needs. For all five existing functions the metadata string is unchanged. - **Fields resolve the name to an adapter once, at boot.** Fields are built once and cached, so resolution is per field rather than per query, the registry has one query-time reader, and an unregistered name fails at boot rather than mid-query. - **Arguments flow in two phases: extract, then apply.** Argument names are customizable via schema element names, which the computation value object can't see when building a clause. Query building calls `extract_args` at the boundary; clause building later calls the pure `clause_options`. Threading element names onto the computation would pollute its identity, which is used to dedupe computations in a `Set`. - **The empty-bucket value moves into the registry.** It's fully derivable from the function (verified across all 45 aggregated value fields: sum/cardinality → `0`; avg/min/max → `nil`), so per-field storage was redundant and let an author pair a function with a wrong empty value. The adapter returns the entire fabricated response rather than a bare value the builder must wrap. - **Argument plumbing ships now**, though no function uses it yet. The point of these prep tickets is that the framework is ready; deferring would leave the follow-up changing the adapter interface itself. ## Renames | Before | After | |---|---| | `ComputationDetail` | deleted (class, spec, RBS, requires) | | `GraphQLField#computation_detail` | `computation_function` (a bare `Symbol`) | | `GraphQLField#with_computation_detail` | deleted | | `Field#runtime_metadata_computation_detail` | `computes(function)` | | `Schema::Field#computation_detail` | `function_adapter` | | `Computation#detail` | `function_adapter` + `function_args` | Runtime metadata per field collapses from three keys to one: ```yaml approximate_sum: computation_function: sum ``` `computes :sum` reads as part of the field DSL, which is otherwise unprefixed (`documentation`, `mapping`, `json_schema`). It validates the name against the registry at dump time, mirroring how `elasticgraph-schema_definition` already depends on `elasticgraph-graphql` to validate scalar coercion adapters — no new gem dependency. ## Verification - `script/type_check` — clean - `script/lint` — 891 files, no offenses - Full suite — 5227 examples, 0 failures - Mutation-checked the empty-bucket path: breaking `empty_bucket_result` fails 2 tests Note: `script/quick_build` exits non-zero on SimpleCov's 100%-coverage gate (79 uncovered lines across 16 files, none touched here). Confirmed pre-existing — the base commit fails identically with the same 79 lines. ## Follow-up With both prep changes landed, #1327 reduces to a percentile adapter, one registry entry, the field definition, two schema element names, docs, and tests. No framework changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

nikhilkumarjadhav-toast · 2026-08-06T16:02:29Z

Another thing that I thought of after submitting my review above: we should make sure that out-of-range percentiles are handled properly. I checked out your branch and ran bundle exec rake boot_locally to try it out and found that percentile values out side the 0 to 100 range cause OpenSearch/Elasticsearch to throw exceptions:

We should return a GraphQL validation error instead. (Would be good to cover that in the acceptance test).

Thanks for pointing out this edge case, I'll fix it in the next commit.

Adds an approximate-percentile aggregation to the aggregatedValues API for Float, Int, JsonSafeLong, LongString, Date, DateTime, and LocalTime fields. Callers request a specific percentile rank via a `percentile` argument (e.g. `percentile: 50` for the median), and can request multiple ranks in a single query by aliasing the field selection. The aliasing-based shape keeps query validity statically verifiable: each aliased selection independently validates its own rank and always returns exactly one value, with no ambiguity around duplicate or out-of-range requests the way a list-argument API would have. An out-of-range `percentile` resolves to `null` with a precisely-pathed GraphQL error rather than failing the entire aggregations subtree, so sibling fields (including other, valid `approximatePercentile` selections) still resolve normally.

nikhilkumarjadhav-toast · 2026-08-06T22:02:22Z

Updated to support granular errors.

The percentile arg doc in built_in_types.rb already spelled this out; the guide's prose was missed when applying the same suggestion there.

myronmarston

Nice work--this is almost ready to merge!

Co-authored-by: Myron Marston <myron.marston@gmail.com>

…tion/schema_elements/built_in_types.rb Co-authored-by: Myron Marston <myron.marston@gmail.com>

…ction_adapter.rb Co-authored-by: Myron Marston <myron.marston@gmail.com>

…ame, stronger test - Replace exception-based control flow in FunctionAdapter#extract_args with a block-yielded error message, propagated through QueryAdapter and the resolver. Avoids relying on exceptions for control flow, documents the validation contract explicitly, and skips the cost of capturing a stack trace for something that isn't actually exceptional. - Rename the BY_NAME registry key and computes tag from :percentiles to :percentile (singular, matching every other function), updating all specs and regenerating schema artifacts. - Rename resolve_target_nodes's allow_errors param to expect_errors and assert errors are actually present when set, so it can't silently pass on a test that has no errors. - Strengthen expected_aggregated_amounts_of with verified p50/p75 assertions (exact values confirmed against a real OpenSearch percentiles aggregation) so the acceptance suite can't be satisfied by a min/max-only implementation.

The previous commit hardcoded exact p50/p75 values verified against only one local OpenSearch 3.6.0 instance. CI runs this suite against four backends (Elasticsearch 9.0.0/9.4.2, OpenSearch 2.19.0/3.6.0), and for small datasets different backends' percentile algorithms legitimately use different (equally valid) interpolation conventions for ranks that don't land exactly on one data point--e.g. p50 of [100, 200] is 150 on some backends and 200 on others. Replace the exact-value assertions with a bounds check (a real Float within [min, max]), which every backend's percentile algorithm must satisfy regardless of interpolation convention, while still ruling out an implementation that only special-cases percentile 0/100. Verified locally against both OpenSearch 3.6.0 and Elasticsearch 9.4.2.

nikhilkumarjadhav-toast · 2026-08-07T19:40:26Z

@myronmarston This is ready for another look — replied to and applied all 6 comments from your latest review:

"percentile rank" → "percentile" wording (both spots)
percentiles: → percentile: registry key rename, refs updated, artifacts regenerated
allow_errors → expect_errors with a real non-empty assertion
Strengthened the percentile acceptance test with p50/p75 (not just p0/p100)
Adopted your block-yielded-error refactor as-is across function_adapter.rb, query_adapter.rb, the resolver, both .rbs files, and specs

Latest commit: b3d18ac6.

myronmarston

🚀

nikhilkumarjadhav-toast requested review from BrianSigafoos-SQ, bsorbo, ellisandrews-toast, jwils, jwondrusch, marcdaniels-toast, myronmarston and rossroberts-toast as code owners August 4, 2026 16:46

myronmarston self-assigned this Aug 5, 2026

myronmarston requested changes Aug 5, 2026

View reviewed changes

nikhilkumarjadhav-toast force-pushed the percentile-aggregated-values branch from 242d09b to d3c4c5b Compare August 6, 2026 21:55

nikhilkumarjadhav-toast requested a review from myronmarston August 6, 2026 22:04

Apply the 0=min/100=max clarification to the guide doc too

056741e

The percentile arg doc in built_in_types.rb already spelled this out; the guide's prose was missed when applying the same suggestion there.

myronmarston requested changes Aug 7, 2026

View reviewed changes

Update config/site/src/query-api/aggregations/aggregated-values.md

8e4b0b5

Co-authored-by: Myron Marston <myron.marston@gmail.com>

nikhilkumarjadhav-toast and others added 4 commits August 7, 2026 11:13

Update elasticgraph-schema_definition/lib/elastic_graph/schema_defini…

ba5e9ed

…tion/schema_elements/built_in_types.rb Co-authored-by: Myron Marston <myron.marston@gmail.com>

Update elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/fun…

86e20b1

…ction_adapter.rb Co-authored-by: Myron Marston <myron.marston@gmail.com>

nikhilkumarjadhav-toast requested a review from myronmarston August 7, 2026 19:43

myronmarston reviewed Aug 7, 2026

View reviewed changes

myronmarston enabled auto-merge (squash) August 7, 2026 21:15

nikhilkumarjadhav-toast requested a review from myronmarston August 7, 2026 21:25

myronmarston disabled auto-merge August 7, 2026 21:30

myronmarston approved these changes Aug 7, 2026

View reviewed changes

myronmarston merged commit 99630d9 into block:main Aug 7, 2026
31 of 33 checks passed

myronmarston mentioned this pull request Aug 8, 2026

PR-1327 followups: drop dead require, tighten percentile test, hoist expect_errors return, drop internal-abstraction spec #1337

Merged

Uh oh!

Conversation

nikhilkumarjadhav-toast commented Aug 4, 2026

Summary

Implementation notes

Test plan

Uh oh!

CLAassistant commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myronmarston left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

myronmarston left a comment

Choose a reason for hiding this comment

Uh oh!

nikhilkumarjadhav-toast commented Aug 6, 2026

Uh oh!

nikhilkumarjadhav-toast commented Aug 6, 2026

Uh oh!

myronmarston left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikhilkumarjadhav-toast commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myronmarston left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CLAassistant commented Aug 4, 2026 •

edited

Loading

nikhilkumarjadhav-toast commented Aug 7, 2026 •

edited

Loading