Key aggregated value aggregations off the GraphQL alias - #1331
Merged
Conversation
myronmarston
marked this pull request as ready for review
August 5, 2026 06:31
myronmarston
requested review from
BrianSigafoos-SQ,
bsorbo,
ellisandrews-toast,
jwils,
jwondrusch,
marcdaniels-toast and
rossroberts-toast
as code owners
August 5, 2026 06:31
This was referenced Aug 5, 2026
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.
myronmarston
force-pushed
the
myron/key-aggregated-values-off-alias
branch
from
August 5, 2026 17:21
c506716 to
ed95d90
Compare
Collaborator
|
this looks good. there's one nit i noticed:
I maybe missed it but i don't see this documented |
rossroberts-toast
approved these changes
Aug 5, 2026
Collaborator
Author
|
🤖 Re: @rossroberts-toast's comment
Good catch -- that line in the PR description was inaccurate, there was never a comment on the attribute itself. Updated the PR description to explain the unused-ness inline instead of claiming it's documented elsewhere. |
myronmarston
added a commit
that referenced
this pull request
Aug 5, 2026
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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-awarename_in_graphql_query. That inconsistency would have forced an argument-bearing function (like the upcomingapproximatePercentile) 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.Computationreplaces itscomputed_index_field_namestring attribute with aleafattribute holding aPathSegment. Itsname_in_indexis intentionally unused -- the function name isn't part of the datastore index path, whichclausederives entirely fromsource_field_path.QueryAdapter) and the resolver side (Resolvers::AggregatedValues) now derive the leaf name through the samePathSegment.forfactory, so there's one rule instead of two implementations that must agree.computed_index_field_nameis deleted (not left unused), since the datastore clause already derives its index path fromsource_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:Correct in both cases -- each field resolves via its own alias. Accepted deliberately:
Test plan
script/type_checkpassesscript/run_gem_specs elasticgraph-graphqlpasses (100% line/branch coverage maintained)script/quick_buildpassesCloses #1329
🤖 Generated with Claude Code