Fix github pages site to use proper relative links. - #1
Merged
Conversation
It's hosted at https://block.github.io/elasticgraph/ and the links were all broken because they assume it's served from a domain root.
myronmarston
pushed a commit
that referenced
this pull request
Nov 14, 2024
myronmarston
pushed a commit
that referenced
this pull request
Nov 14, 2024
This was referenced Aug 5, 2026
myronmarston
added a commit
that referenced
this pull request
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
added a commit
that referenced
this pull request
Aug 5, 2026
## 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)
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
2 participants
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.
It's hosted at https://block.github.io/elasticgraph/ and the links were all broken because they assume it's served from a domain root.