feat: compute dynamic (actual) cost#1376
Merged
Conversation
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy,
dkorittki and
jensneuse
as code owners
Contributor
📝 WalkthroughWalkthroughReplaces static-cost APIs with estimated and actual cost computation: renames ComputeStaticCost→ComputeEstimatedCost/EstimateCost and Static→Estimated, threads actualListSizes from resolver into cost trees, computes ActualCost after resolution, and updates planner/visitor and plan public APIs accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Engine as ExecutionEngine
participant Planner as Planner
participant Resolver as Resolver
participant Calc as CostCalculator
Client->>Engine: Send GraphQL request
Engine->>Planner: Build plan (attach CostCalculator if ComputeCosts)
Planner->>Calc: EstimateCost (build estimated cost tree)
Engine->>Resolver: Execute plan / resolve response
Resolver->>Resolver: Record ActualListSizes per-field
Resolver-->>Engine: Return response + ActualListSizes
Engine->>Calc: ActualCost(config, ActualListSizes)
Engine-->>Client: Return response with estimatedCost and actualCost
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@execution/graphql/request.go`:
- Around line 213-220: The ComputeActualCost method currently sets r.actualCost
= 0 when actualListSizes is nil; instead, if calc != nil you should call
calc.ActualCost with an empty map when actualListSizes == nil so non-list costs
are still computed. Update Request.ComputeActualCost to build a zero-length
map[string]int (or equivalent) and pass that to
plan.CostCalculator.ActualCost(config, variables, actualListSizes) when the
incoming actualListSizes is nil; keep the existing behavior of setting
r.actualCost = 0 only when calc is nil.
🧹 Nitpick comments (1)
execution/engine/execution_engine_test.go (1)
339-347: Zero-cost expectations currently won’t be asserted.
The!= 0guards skip validation for legitimate zero-cost cases (e.g., negative weights). Consider a*intor explicithasExpectedCostflag so zero can be asserted.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
execution/engine/execution_engine_test.go (1)
203-217:⚠️ Potential issue | 🟡 MinorAllow asserting zero expected costs.
The
!= 0guards skip legitimate zero expectations (e.g., the negative-weights test), so those checks never run. Consider explicit flags (or pointer fields) to signal “assert even when zero.”✅ Proposed fix (explicit expectation flags)
type ExecutionEngineTestCase struct { schema *graphql.Schema operation func(t *testing.T) graphql.Request dataSources []plan.DataSource fields plan.FieldConfigurations engineOptions []ExecutionOptions customResolveMap map[string]resolve.CustomResolve skipReason string indentJSON bool expectedResponse string expectedJSONResponse string expectedFixture string expectedEstimatedCost int expectedActualCost int + expectEstimatedCost bool + expectActualCost bool }- if testCase.expectedEstimatedCost != 0 { + if testCase.expectedEstimatedCost != 0 || testCase.expectEstimatedCost { gotCost := operation.EstimatedCost() require.Equal(t, testCase.expectedEstimatedCost, gotCost) } - if testCase.expectedActualCost != 0 { + if testCase.expectedActualCost != 0 || testCase.expectActualCost { gotActualCost := operation.ActualCost() require.Equal(t, testCase.expectedActualCost, gotActualCost) }t.Run("negative weights - cost is never negative", runWithoutError( ExecutionEngineTestCase{ // ... expectedEstimatedCost: 0, + expectEstimatedCost: true, }, computeCosts(), ))Also applies to: 339-347, 5769-5827
… yury/eng-8843-engine-compute-dynamic-actual-cost
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/resolve/resolve.go`:
- Line 321: Import ordering in the file containing the symbol ActualListSizes is
failing the gci linter; fix it by running gci to auto-reorder the import block
(or manually reorder imports into standard library, blank line, third-party,
blank line, local project imports) so the import groups and spacing match the
project's gci configuration, then stage the updated import block and re-run
tests/linter.
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/resolvable.go (1)
895-904: Consider using a pooled buffer for path construction.The helper allocates a new slice on each call via
append. While acceptable for current usage, if this becomes a hot path, consider usingpool.BytesBuffer(already imported viapoolpackage) similar torenderFieldPath().💡 Optional optimization using pooled buffer
// Helper to build JSON path (field names only, no array indices) func (r *Resolvable) currentFieldPath() string { - var parts []string + buf := pool.BytesBuffer.Get() + defer pool.BytesBuffer.Put(buf) + first := true for _, elem := range r.path { if elem.Name != "" { - parts = append(parts, elem.Name) + if !first { + buf.WriteByte('.') + } + buf.WriteString(elem.Name) + first = false } } - return strings.Join(parts, ".") + return buf.String() }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/resolve/resolve.go`:
- Around line 355-356: The ArenaResolveGraphQLResponse path forgets to propagate
actual list sizes: after calling t.resolvable.Resolve() and before returning the
ArenaResolveGraphQLResponse, set resp.ActualListSizes =
t.resolvable.actualListSizes (same as ResolveGraphQLResponse does) so that
ArenaResolveGraphQLResponse includes the actual list size data for correct cost
calculations; locate the assignment near the return in the
ArenaResolveGraphQLResponse function and add the same assignment referencing
t.resolvable.actualListSizes.
… yury/eng-8843-engine-compute-dynamic-actual-cost
devsergiy
approved these changes
Feb 16, 2026
… yury/eng-8843-engine-compute-dynamic-actual-cost
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/engine/execution_engine_test.go`:
- Around line 347-355: The test currently skips cost assertions when
expectedEstimatedCost or expectedActualCost are zero due to `!= 0` guards, so
change the test to use explicit expectation flags (e.g., add bool fields
`expectEstimatedCost` and `expectActualCost` to the testCase struct) or use
pointers for `expectedEstimatedCost`/`expectedActualCost` so nil means "don't
assert" and zero is assertable; then update the assertion logic to check the
flag/pointer (call operation.EstimatedCost() and operation.ActualCost() when the
corresponding `expect...` is true or the pointer is non-nil) and update all test
case entries (including the negative-weights test) to set the new flags or
provide pointers so zero costs are actually asserted.
In `@v2/pkg/engine/plan/cost.go`:
- Around line 508-520: ActualCost currently computes a non-zero value when
actualListSizes is nil, which can misrepresent a reset/unused actual cost;
update the CostCalculator.ActualCost method to guard for a nil actualListSizes
and return 0 immediately (preserving existing behavior only when actualListSizes
!= nil) before building costConfigs and calling c.tree.cost with actualCostMode
so stale multipliers aren't used.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v2/pkg/engine/plan/visitor.go (1)
1083-1085:⚠️ Potential issue | 🔴 CriticalReset visitor caches on each Plan() call to prevent stale data when Planner is reused.
When a Planner instance is reused to plan multiple operations (as shown in planner_test.go where
sharedPlanner.Plan()is called twice with different operations), the Visitor's cache maps (pathCache,fieldPlanners,plannerFields,fieldConfigs,exportedVariables,skipIncludeOnFragments,indirectInterfaceFields) retain stale data from the previous operation. Since CostVisitor holds a direct reference tofieldPlanners(line 166:p.costVisitor.fieldPlanners = p.planningVisitor.fieldPlanners), clearing in place rather than creating new maps is necessary to preserve this reference.🛠️ Suggested fix (clear maps in place)
+func (v *Visitor) resetPerDocument() { + // slices + v.objects = v.objects[:0] + v.currentFields = v.currentFields[:0] + v.currentField = nil + + // maps (clear in place to keep shared references) + for k := range v.fieldConfigs { delete(v.fieldConfigs, k) } + for k := range v.exportedVariables { delete(v.exportedVariables, k) } + for k := range v.skipIncludeOnFragments { delete(v.skipIncludeOnFragments, k) } + for k := range v.indirectInterfaceFields { delete(v.indirectInterfaceFields, k) } + for k := range v.pathCache { delete(v.pathCache, k) } + for k := range v.plannerFields { delete(v.plannerFields, k) } + for k := range v.fieldPlanners { delete(v.fieldPlanners, k) } + for k := range v.fieldEnclosingTypeNames { delete(v.fieldEnclosingTypeNames, k) } +} + func (v *Visitor) EnterDocument(operation, definition *ast.Document) { + v.resetPerDocument() v.Operation, v.Definition = operation, definition }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/plan/visitor.go` around lines 1083 - 1085, Visitor.EnterDocument currently only sets v.Operation and v.Definition but does not clear the Visitor's cache maps, causing stale state when a Planner is reused; in EnterDocument clear the maps in place (not by assigning new map objects) for pathCache, fieldPlanners, plannerFields, fieldConfigs, exportedVariables, skipIncludeOnFragments, and indirectInterfaceFields so existing references (notably p.costVisitor.fieldPlanners which points to v.fieldPlanners) remain valid and see the cleared state before planning the new operation.v2/pkg/engine/plan/cost.go (1)
257-286:⚠️ Potential issue | 🟠 MajorActual‑cost rounding at each node can undercount sparse nested lists.
Because actual mode uses average multipliers (<1), rounding inside each node before parent scaling can zero out non‑zero totals (e.g., 1 item across 3 parents → avg 0.33 → rounds to 0, then parent multiplies → 0). Consider carrying float costs through actual‑mode recursion and rounding once at the root (or deferring rounding until after parent scaling).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/plan/cost.go` around lines 257 - 286, The current per-node rounding in the cost calculation (in the block using node.costsAndMultiplier, childrenCost loop, multiplier handling and final cost += int(math.RoundToEven(...))) causes undercounting for actual-mode average multipliers; modify the cost computation to propagate float64 costs through recursion: change local cost/childrenCost/fieldCost/argsCost/directivesCost to float64 (or keep a float64 accumulator), compute cost += (childrenCost + fieldCost) * multiplier as a float without calling math.RoundToEven at each node, and only round/convert to int at the top-level root return point (or in the public Cost() wrapper) so rounding is deferred until after parent scaling; ensure multiplier enforcement for non-list fields remains (set multiplier=1 as float64) and keep the non-negative clamp after final rounding.
🧹 Nitpick comments (2)
v2/pkg/engine/plan/cost_visitor.go (1)
22-23: Align the comment with the non-pointer field.
The field is now a map value, so “Pointer” is misleading.Suggested comment tweak
- // Pointer to Visitor.fieldPlanners. We capture this early before this map is actually filled. + // Reference to Visitor.fieldPlanners. We capture this early before this map is actually filled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/plan/cost_visitor.go` around lines 22 - 23, The comment for the fieldPlanners map is misleading because it says "Pointer" while the field is a map value; update the comment for the Visitor.fieldPlanners declaration to reflect that it's a map value captured early (e.g., "Map to Visitor.fieldPlanners. We capture this early before this map is actually filled.") and ensure it references the field name fieldPlanners (type map[int][]int) so it matches the code.v2/pkg/engine/plan/cost.go (1)
589-600: Debug output multiplier should mirror non‑list override.
cost()forces multiplier=1 for non‑list fields, but debug output prints multiplier=0, which can mislead. Consider applying the same override before printing.Suggested tweak
fieldCost, argsCost, dirsCost, multiplier := node.costsAndMultiplier(configs, variables, defaultListSize, actualListSizes) +if multiplier == 0 && !node.returnsListType { + multiplier = 1.0 +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/plan/cost.go` around lines 589 - 600, The debug printing block uses the raw multiplier returned from node.costsAndMultiplier but cost() overrides multiplier to 1 for non-list fields, causing the debug output to show 0; update the code that prints the debug info (the fmt.Fprintf block using fieldCost, argsCost, dirsCost, multiplier) to apply the same non-list override logic used in cost() (i.e., if the field is not a list and multiplier == 0 then set multiplier = 1) before formatting, so the logged multiplier mirrors the actual cost calculation; reference node.costsAndMultiplier and the cost() override behavior when locating where to apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/engine/execution_engine.go`:
- Around line 235-243: The current guard skips calling
operation.ComputeActualCost when resp.ActualListSizes is nil, leaving stale
actualCost values on reused Request objects; in the SynchronousResponsePlan
handling (after e.resolver.ResolveGraphQLResponse returns resp), always invoke
operation.ComputeActualCost(costCalculator, e.config.plannerConfig,
execContext.resolveContext.Variables, resp.ActualListSizes) unconditionally
(remove the "resp != nil && resp.ActualListSizes != nil" check) so
ComputeActualCost runs even when ActualListSizes is nil and resets costs; keep
the ResolveGraphQLResponse error handling and return behavior unchanged.
In `@v2/pkg/engine/plan/cost.go`:
- Around line 417-434: The current empty-list branch in the cost calculation
(inside the logic that reads actualListSizes for node.jsonPath) sets multiplier
= 1.0 which still multiplies childrenCost; change this so children of an empty
list are not charged: when totalCount == 0 set multiplier = 0 (so childrenCost
becomes zero) and separately add the base resolver cost (e.g., add fieldCost or
the equivalent one-time cost for node) so the field itself is charged once;
alternatively propagate a boolean (e.g., skipChildren or isEmptyList) from this
block to the caller that computes childrenCost and use it to skip adding
childrenCost when true. Ensure you update references to multiplier,
childrenCost, and fieldCost (and any callers expecting multiplier behavior)
accordingly.
---
Outside diff comments:
In `@v2/pkg/engine/plan/cost.go`:
- Around line 257-286: The current per-node rounding in the cost calculation (in
the block using node.costsAndMultiplier, childrenCost loop, multiplier handling
and final cost += int(math.RoundToEven(...))) causes undercounting for
actual-mode average multipliers; modify the cost computation to propagate
float64 costs through recursion: change local
cost/childrenCost/fieldCost/argsCost/directivesCost to float64 (or keep a
float64 accumulator), compute cost += (childrenCost + fieldCost) * multiplier as
a float without calling math.RoundToEven at each node, and only round/convert to
int at the top-level root return point (or in the public Cost() wrapper) so
rounding is deferred until after parent scaling; ensure multiplier enforcement
for non-list fields remains (set multiplier=1 as float64) and keep the
non-negative clamp after final rounding.
In `@v2/pkg/engine/plan/visitor.go`:
- Around line 1083-1085: Visitor.EnterDocument currently only sets v.Operation
and v.Definition but does not clear the Visitor's cache maps, causing stale
state when a Planner is reused; in EnterDocument clear the maps in place (not by
assigning new map objects) for pathCache, fieldPlanners, plannerFields,
fieldConfigs, exportedVariables, skipIncludeOnFragments, and
indirectInterfaceFields so existing references (notably
p.costVisitor.fieldPlanners which points to v.fieldPlanners) remain valid and
see the cleared state before planning the new operation.
---
Duplicate comments:
In `@execution/engine/execution_engine_test.go`:
- Around line 347-355: The test currently skips assertions when
expectedEstimatedCost or expectedActualCost equal 0 because of the "!= 0"
guards; change the test case fields to allow explicit zero expectations (e.g.,
make expectedEstimatedCost and expectedActualCost pointers like *int or add
boolean flags such as hasExpectedEstimatedCost/hasExpectedActualCost) and update
the test logic to check for nil/flag rather than != 0, then compare
operation.EstimatedCost() and operation.ActualCost() to the dereferenced pointer
(or asserted value) so zero-cost expectations are asserted correctly.
---
Nitpick comments:
In `@v2/pkg/engine/plan/cost_visitor.go`:
- Around line 22-23: The comment for the fieldPlanners map is misleading because
it says "Pointer" while the field is a map value; update the comment for the
Visitor.fieldPlanners declaration to reflect that it's a map value captured
early (e.g., "Map to Visitor.fieldPlanners. We capture this early before this
map is actually filled.") and ensure it references the field name fieldPlanners
(type map[int][]int) so it matches the code.
In `@v2/pkg/engine/plan/cost.go`:
- Around line 589-600: The debug printing block uses the raw multiplier returned
from node.costsAndMultiplier but cost() overrides multiplier to 1 for non-list
fields, causing the debug output to show 0; update the code that prints the
debug info (the fmt.Fprintf block using fieldCost, argsCost, dirsCost,
multiplier) to apply the same non-list override logic used in cost() (i.e., if
the field is not a list and multiplier == 0 then set multiplier = 1) before
formatting, so the logged multiplier mirrors the actual cost calculation;
reference node.costsAndMultiplier and the cost() override behavior when locating
where to apply the fix.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@execution/graphql/request.go`:
- Around line 213-220: In ComputeActualCost, add a nil guard for the
actualListSizes parameter so that when actualListSizes == nil the method resets
r.actualCost = 0 and returns immediately (clearing stale cost when reusing
Request objects); otherwise keep the current behavior of calling
calc.ActualCost(config, actualListSizes) when calc != nil. Ensure you reference
the Request.ComputeActualCost method, the r.actualCost field, and the
actualListSizes parameter in the change.
In `@v2/pkg/engine/plan/cost.go`:
- Around line 417-434: When actualListSizes[node.jsonPath] reports totalCount ==
0 we should not charge children but still charge the field's resolver call;
change the single multiplier logic in the block handling !isEstimation to split
into two values (e.g., fieldMultiplier and childrenMultiplier) instead of using
multiplier for both. Keep fieldMultiplier = 1.0 when totalCount == 0 (to charge
the resolver) and set childrenMultiplier = 0.0 (to avoid charging children), and
update the cost aggregation sites that currently use multiplier (where
childrenCost and the field's own cost are combined) to apply childrenMultiplier
to childrenCost and fieldMultiplier to the field's base cost; reference
isEstimation, actualListSizes, node.jsonPath, multiplier (replace) and
childrenCost to find and update usages.
This was referenced Feb 19, 2026
ysmolski
pushed a commit
that referenced
this pull request
Feb 19, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.253](v2.0.0-rc.252...v2.0.0-rc.253) (2026-02-19) ### Features * compute dynamic (actual) cost ([#1376](#1376)) ([d6dc786](d6dc786)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
jensneuse
pushed a commit
that referenced
this pull request
Feb 20, 2026
🤖 I have created a release *beep* *boop* --- ## [1.9.0](execution/v1.8.1...execution/v1.9.0) (2026-02-20) ### Features * compute dynamic (actual) cost ([#1376](#1376)) ([d6dc786](d6dc786)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added dynamic cost computation capability <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
This PR implements actual cost calculation for GraphQL operations alongside
the existing estimated cost. The actual cost is computed after query execution
using real response data (list sizes, field presence) instead of estimated multipliers
from slicing arguments.
The implementation consolidates estimated and actual cost logic into a unified cost()
method that uses float64 multipliers for actual costs.
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests