Replace ComputationDetail with a function adapter registry - #1332
Draft
myronmarston wants to merge 1 commit into
Draft
Replace ComputationDetail with a function adapter registry#1332myronmarston wants to merge 1 commit into
myronmarston wants to merge 1 commit into
Conversation
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
commented
Aug 5, 2026
| s.object_type "Widget" do |t| | ||
| t.field "id", "ID" | ||
| t.field "total", "Int", graphql_only: true do |f| | ||
| f.computes :percentiles |
Collaborator
Author
There was a problem hiding this comment.
We should use a different name here since we plan to add percentile support in a later PR--that would break this test.
Comment on lines
+11
to
+12
| # `computes` validates the function name against the query engine's registry at dump time, mirroring | ||
| # how this gem already depends on `elasticgraph-graphql` to validate scalar coercion adapters. |
Collaborator
Author
There was a problem hiding this comment.
Suggested change
| # `computes` validates the function name against the query engine's registry at dump time, mirroring | |
| # how this gem already depends on `elasticgraph-graphql` to validate scalar coercion adapters. |
| def clause_options(function_args) | ||
| {"percents" => function_args.fetch(:percents)} | ||
| end | ||
| end |
Collaborator
Author
There was a problem hiding this comment.
Let's just define this as an anonymous singleton object within the example that needs it, rather than as a globally accessible class.
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.
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 == :percentilesconditionals 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
datastore_function_nameextract_args(args, element_names)clause_options(function_args)fieldextract_result(raw)empty_bucket_resultAll five existing functions differ only in datastore name and empty-bucket value, so one
SimpleMetricdata 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
extract_argsat the boundary; clause building later calls the pureclause_options. Threading element names onto the computation would pollute its identity, which is used to dedupe computations in aSet.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.Renames
ComputationDetailGraphQLField#computation_detailcomputation_function(a bareSymbol)GraphQLField#with_computation_detailField#runtime_metadata_computation_detailcomputes(function)Schema::Field#computation_detailfunction_adapterComputation#detailfunction_adapter+function_argsRuntime metadata per field collapses from three keys to one:
computes :sumreads 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 howelasticgraph-schema_definitionalready depends onelasticgraph-graphqlto validate scalar coercion adapters — no new gem dependency.Verification
script/type_check— cleanscript/lint— 891 files, no offensesempty_bucket_resultfails 2 testsNote:
script/quick_buildexits 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