Skip to content

Key aggregated value aggregations off the GraphQL alias #1329

Description

@myronmarston

Prep refactor #1 of 2, paving the way for PR #1327 (approximatePercentile). Neither prep ticket adds the percentile function itself.

What to build

An aggregated value function field that is aliased in a GraphQL query resolves through a datastore aggregation key derived from that alias, rather than from the field's name_in_index.

Today the leaf segment of an aggregated value key is the only path segment keyed off name_in_index — every parent segment already uses the alias-aware name_in_graphql_query. That inconsistency is what forces an argument-bearing function to invent a synthetic leaf name (e.g. approximate_percentile(50.0)), built independently in the query-building code and the resolver, which must then agree byte-for-byte. Making the leaf alias-derived removes the need for synthetic key naming entirely, so a function taking arguments needs no key-related metadata at all.

Why the alias is sufficient

GraphQL's FieldsWillMerge validation forbids two selections sharing a response key with different arguments. Requesting two percentile ranks therefore requires aliases (p50:, p99:), which makes alias-derived leaf keying collision-free by construction — no argument value needs to be folded into the key.

Secondary benefit: a synthetic name like approximate_percentile(50.0) would embed . (the field-path encoder's delimiter) inside a single key segment. Alias-derived names avoid that class of problem.

Design decisions

The leaf is a separate attribute, not appended to the existing source field path. The Computation value object replaces its computed_index_field_name string attribute with a leaf attribute holding a path segment. Rejected alternatives:

  • Appending the leaf to source_field_path — the clause builder would have to remember to drop the last element, and forgetting silently produces a wrong index path.
  • A plain string leaf name — both call sites would then duplicate the alias || name idiom instead of sharing the existing path-segment factory, a mild echo of the coupling this ticket removes.

The leaf path segment's name_in_index is unused (the function name is not part of the datastore index path). Document that on the attribute.

computed_index_field_name becomes dead once the key stops using it — the clause builder derives its index path from source_field_path — so delete it rather than leaving it unused.

Both the query-building side and the resolver side must derive the leaf name through the same existing path-segment factory, so there is one rule rather than two implementations that must agree.

Intentional behavior change

Two aliases of the same function under one field currently collapse into a single computation (the value objects are equal, they are held in a Set, and the key is identical either way). After this change their leaf segments differ, so two identical datastore aggregations are sent:

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 precisely the two-sides-must-agree coupling this ticket exists to delete.
  • Rejecting aliases outright is a non-starter: the percentile function requires aliases to request multiple ranks.

This makes the ticket a behavior change, not a pure refactor. The PR description must say so rather than claiming no functional change.

Acceptance criteria

  • TDD order followed: each test written and observed failing at runtime (not merely failing to load) before the corresponding implementation
  • Unit test: an aliased aggregated value function field produces a key built from the alias
  • Unit test: two aliases of the same function under one field produce two distinct keys (locks in the accepted behavior change)
  • Unit test: the datastore clause's index field path is unaffected by aliasing
  • Query-building unit test covering an aliased function field
  • Acceptance test issuing a GraphQL query with an aliased aggregated value function and asserting the resolved value
  • Computation's computed_index_field_name attribute is deleted, not merely unused
  • The leaf attribute carries a comment noting its index name is intentionally unused
  • Query building and resolution both derive the leaf name via the shared path-segment factory
  • RBS signatures updated; script/type_check passes
  • Aggregation spec helpers updated for the renamed attribute (approximately 20 call sites)
  • script/run_specs passes with 100% coverage maintained
  • script/quick_build passes
  • PR description states the aliasing behavior change explicitly and does not describe the change as a pure refactor

Blocked by

None — can start immediately.

Metadata

Metadata

Assignees

Labels

ready-for-agentTicket is fully specified and agent-grabbablerubyPull requests that update Ruby code

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions