feat: compute static costs #1359
Conversation
And keep one test failing since we cannnot read variables yet.
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
📝 WalkthroughWalkthroughAdds a static-cost subsystem (visitor, cost calculator, per-data-source cost config), integrates it into planning and plans, propagates calculators and variables into execution (getCachedPlan now returns a calculator), renames visitor/AST APIs, removes internal prepare usage in execution, and extends tests to assert static-cost results. Changes
Sequence Diagram(s)sequenceDiagram
participant Planner as Planner
participant Walker as Walker
participant StaticVisitor as StaticCostVisitor
participant DS as DataSource
participant CostCalc as CostCalculator
participant Plan as Plan
Planner->>Planner: Check Configuration.ComputeStaticCost
alt ComputeStaticCost enabled
Planner->>StaticVisitor: NewStaticCostVisitor(walker, operation, definition)
Planner->>Walker: Register StaticVisitor enter/leave handlers
loop Walk operation
StaticVisitor->>StaticVisitor: EnterField(fieldRef)
StaticVisitor->>StaticVisitor: LeaveField(fieldRef)
end
StaticVisitor->>Planner: finalCostTree()
Planner->>CostCalc: NewCostCalculator()
loop Per data source
Planner->>DS: GetCostConfig()
DS-->>Planner: DataSourceCostConfig
Planner->>CostCalc: SetDataSourceCostConfig(dsHash, config)
end
Planner->>CostCalc: Set cost tree
Planner->>Plan: SetStaticCostCalculator(costCalc)
else
Planner->>Plan: Build plan without static cost
end
sequenceDiagram
participant Execute as Execute
participant Cache as PlanCache
participant Plan as Plan
participant CostCalc as CostCalculator
participant Request as Request
Execute->>Cache: getCachedPlan(...) -> (plan, costCalc)
alt costCalc != nil
Execute->>CostCalc: SetVariables(variables)
Execute->>CostCalc: (optional) cost := CostCalc.GetStaticCost(config, variables)
Execute->>Request: Request.ComputeStaticCost(costCalc, config, variables)
end
Execute->>Plan: Execute plan
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
34-62: Public API break: addingGetCostConfig()toDataSourceforces all external implementations to update.The
plan.DataSourceinterface is explicitly documented as intended for external implementation ("It's up to you to implement the DataSource interface"). This new method is a source-breaking change for any external consumers. Consider whether this interface should remain public in v2, or if breaking changes are acceptable for pre-release versions. If external stability is required, use an optional interface pattern or provide a compatible default via an embedded interface.Also applies to: 280-296
🤖 Fix all issues with AI agents
In @execution/engine/execution_engine.go:
- Around line 71-72: The struct field formatting for lastPlan (type plan.Plan)
is misaligned causing gci/import/format lint failures; run the project's
formatting command (e.g., `gci write` or `go fmt`/project-standard formatter) to
reformat the file so the field alignment and imports conform, then commit the
updated file so the pipeline passes.
In @v2/pkg/engine/plan/planner.go:
- Around line 61-79: Plan() can panic because p.planningVisitor.costCalculator
may be nil when SetConfig() enabled ComputeStaticCost after NewPlanner(), and
its internal tree state is never cleared between Plan() calls; inside Plan()
ensure that when config.ComputeStaticCost is true you initialize
p.planningVisitor.costCalculator if nil (instantiate via NewCostCalculator and
reapply data source cost configs by iterating config.DataSources and calling
SetDataSourceCostConfig with ds.Hash()), and also reset the calculator's
accumulated state before each planning run (either call a reset method on
CostCalculator like Reset/ResetTree if available or recreate the CostCalculator
instance) so subsequent Plan() calls do not accumulate previous costs; update
Plan() to use the safe, non-nil costCalculator when calling GetTotalCost().
In @v2/pkg/engine/plan/static_cost.go:
- Around line 97-103: NewDataSourceCostConfig currently initializes Weights and
Types but leaves ListSizes nil, which can cause a panic when callers modify it;
update NewDataSourceCostConfig to also initialize ListSizes (e.g., set
ListSizes: make(map[string]int)) in the returned *DataSourceCostConfig so all
maps are ready for use when functions interact with ListSizes.
In @v2/pkg/engine/plan/visitor.go:
- Around line 408-410: The visitor method enterFieldCost (and related debug
prints in the same visitor block around lines 422-485) currently emits
fmt.Printf and unconditional logging which pollutes stdout; remove those direct
prints and unconditional logs and instead gate any diagnostic output behind the
existing debug flag or the visitor's logger (e.g., use v.debugEnabled or
v.logger.Debugf if available) so normal runs produce no stdout side effects;
update enterFieldCost and the surrounding debug statements to either drop the
prints entirely or wrap them in a debug check that uses the structured logger.
- Around line 424-485: The extractFieldArguments function must stop panicking on
PrintValueBytes errors and correctly assign literal types: replace panic(err)
with non-fatal handling (log/error return or skip the problematic argument) and
ensure argInfo.typeName is derived from the argument's kind for literal values
(switch on argValue.Kind to set "Int"/"Float"/"String"/"Boolean"/"Enum" or the
appropriate built-in names) while using v.Definition.TypeNameString only when
argValue refers to a true type reference; update any similar logic in the
sibling block around lines ~504-579, and keep argInfo.ValueBytes populated only
when PrintValueBytes succeeds so downstream list-size/AssumedSize logic receives
correct literal types and no unexpected panics occur.
🧹 Nitpick comments (10)
v2/pkg/engine/plan/configuration.go (1)
49-50: Consider adding a documentation comment for consistency.Other configuration fields in this struct have documentation comments explaining their purpose (e.g.,
DisableResolveFieldPositions,EnableOperationNamePropagation). Adding a brief comment would maintain consistency and help users understand when to enable this option.📝 Suggested documentation
ValidateRequiredExternalFields bool + // ComputeStaticCost enables calculation of estimated static cost for queries. ComputeStaticCost boolv2/pkg/engine/plan/datasource_configuration.go (1)
344-349: Consider returning an immutable view (or documenting mutability) forCostConfig.
GetCostConfig()returns the pointer stored in metadata. If callers may mutate it after planner creation, cost behavior becomes non-deterministic across requests. If mutability is intended, document it; otherwise consider cloning config/maps on set or on read.execution/engine/execution_engine_test.go (2)
203-217: Make static-cost assertions explicit; currentexpectedStaticCost != 0can’t validate legitimate “0 cost” cases.Using
0as “unset” prevents asserting a real 0-cost plan. Prefer*int/hasExpectedStaticCost boolor a sentinel likeexpectedStaticCostSet.Also applies to: 219-255, 337-342
272-280: Avoid forcing verbose planner debug output for the entire test suite.Enabling
PlanningVisitor/PrintQueryPlansetc. for all tests tends to bloat logs and slow CI. Suggest defaulting these to false and enabling only for the static cost tests (or behindtesting.Verbose()).v2/pkg/engine/plan/static_cost.go (6)
116-125: Nil receiver returns 0 instead of default object weight.
ObjectTypeWeightreturns0when receiver is nil (line 119), but non-nil receivers fall back toStaticCostDefaults.Object(which is1). This inconsistency could cause unexpected cost differences depending on whether a nil config is passed vs an empty one.Consider returning
StaticCostDefaults.Objectfor consistency, or document this as intentional behavior for nil configs.
173-207: Remove debug print statements before merging.Lines 182 and 200 contain
fmt.Printfdebug statements that should be removed or replaced with proper structured logging before this code is production-ready.Proposed fix
func (node *CostTreeNode) maxWeightImplementingField(config *DataSourceCostConfig, fieldName string) *FieldWeight { var maxWeight *FieldWeight for _, implTypeName := range node.implementingTypeNames { // Get the cost config for the field of an implementing type. coord := FieldCoordinate{implTypeName, fieldName} fieldWeight := config.Weights[coord] if fieldWeight != nil { if fieldWeight.HasWeight && (maxWeight == nil || fieldWeight.Weight > maxWeight.Weight) { - fmt.Printf("found better maxWeight for %v: %v\n", coord, fieldWeight) maxWeight = fieldWeight } } } return maxWeight } func (node *CostTreeNode) maxMultiplierImplementingField(config *DataSourceCostConfig, fieldName string, arguments map[string]ArgumentInfo) *FieldListSize { var maxMultiplier int var maxListSize *FieldListSize for _, implTypeName := range node.implementingTypeNames { coord := FieldCoordinate{implTypeName, fieldName} listSize := config.ListSizes[coord] if listSize != nil { multiplier := listSize.multiplier(arguments) if maxListSize == nil || multiplier > maxMultiplier { - fmt.Printf("found better multiplier for %v: %v\n", coord, multiplier) maxMultiplier = multiplier maxListSize = listSize } } } return maxListSize }
334-353: Silent early return may hide traversal bugs.Line 344-346: If
current.fieldRef != fieldRef, the function returns silently without calculating costs. This could mask issues whereEnterField/LeaveFieldcalls are mismatched. Consider adding a warning log or panic in debug mode to catch traversal inconsistencies.current := c.stack[lastIndex] if current.fieldRef != fieldRef { + // Consider logging: mismatched fieldRef indicates traversal bug return }
363-398: Remove debug print statements.Lines 374 and 387 contain
fmt.Printfdebug/warning statements that should use proper structured logging or be removed before production.Proposed fix
for _, dsHash := range node.dataSourceHashes { dsCostConfig, ok := c.costConfigs[dsHash] if !ok { - fmt.Printf("WARNING: no cost dsCostConfig for data source %v\n", dsHash) continue } fieldWeight := dsCostConfig.Weights[node.fieldCoord] listSize := dsCostConfig.ListSizes[node.fieldCoord] // The cost directive is not allowed on fields in an interface. // The cost of a field on an interface can be calculated based on the costs of // the corresponding field on each concrete type implementing that interface, // either directly or indirectly through other interfaces. if fieldWeight != nil && node.isEnclosingTypeAbstract && parent.returnsAbstractType { // Composition should not let interface fields have weights, so we assume that // the enclosing type is concrete. - fmt.Printf("WARNING: cost directive on field %v of interface %v\n", node.fieldCoord, parent.fieldCoord) }
432-439: Clarify argument type fallback logic.The
elsebranch (line 437) appliesObjectTypeWeightto arguments that are neitherisSimplenorisInputObject. It's unclear what argument types would reach this code path. Consider adding a comment explaining valid cases, or if this is unreachable, use a panic/assertion.
465-475:Calculate()is invoked on every call toGetTree()andGetTotalCost().Both methods call
Calculate()unconditionally. If a caller invokes both methods, or calls one method multiple times, the cost tree is recalculated redundantly. Consider caching the result or adding a "calculated" flag.Proposed improvement
// Add a calculated flag to CostTree type CostTree struct { Root *CostTreeNode Total int calculated bool } func (t *CostTree) Calculate() { if t.calculated { return } if t.Root != nil { t.Total = t.Root.TotalCost() } t.calculated = true }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
execution/engine/execution_engine.goexecution/engine/execution_engine_test.gov2/pkg/ast/ast_type.gov2/pkg/engine/plan/configuration.gov2/pkg/engine/plan/datasource_configuration.gov2/pkg/engine/plan/node_selection_visitor.gov2/pkg/engine/plan/path_builder_visitor.gov2/pkg/engine/plan/plan.gov2/pkg/engine/plan/planner.gov2/pkg/engine/plan/static_cost.gov2/pkg/engine/plan/visitor.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-02T08:25:26.682Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
Applied to files:
v2/pkg/engine/plan/node_selection_visitor.go
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/plan/node_selection_visitor.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/plan/visitor.go
🧬 Code graph analysis (5)
v2/pkg/engine/plan/datasource_configuration.go (1)
v2/pkg/engine/plan/static_cost.go (1)
DataSourceCostConfig(74-95)
execution/engine/execution_engine.go (1)
v2/pkg/engine/plan/plan.go (1)
Plan(14-19)
v2/pkg/engine/plan/static_cost.go (3)
v2/pkg/engine/plan/federation_metadata.go (1)
FieldCoordinate(92-95)v2/pkg/engine/plan/datasource_configuration.go (1)
DSHash(15-15)v2/pkg/ast/path.go (1)
FieldName(23-23)
v2/pkg/engine/plan/planner.go (4)
v2/pkg/engine/plan/static_cost.go (2)
CostCalculator(279-288)NewCostCalculator(291-306)v2/pkg/engine/plan/visitor.go (1)
Visitor(33-72)v2/pkg/astvisitor/visitor.go (1)
Walker(65-98)v2/pkg/engine/plan/configuration.go (1)
FieldConfiguration(116-133)
execution/engine/execution_engine_test.go (2)
v2/pkg/engine/resolve/resolve.go (1)
ResolverOptions(92-170)v2/pkg/engine/plan/static_cost.go (3)
DataSourceCostConfig(74-95)FieldWeight(20-32)FieldListSize(35-51)
🪛 GitHub Actions: execution
execution/engine/execution_engine.go
[error] 72-72: golangci-lint: File is not properly formatted (gci).
execution/engine/execution_engine_test.go
[error] 340-340: Test failed: static cost computation expected 48 but got 32 in 'multiple_slicing_arguments_as_literals'.
[error] 6002-6002: Test failure due to mismatch in static cost computation: expected 48, actual 32 in 'multiple_slicing_arguments_as_literals'.
🪛 GitHub Check: Linters (1.25)
execution/engine/execution_engine.go
[failure] 72-72:
File is not properly formatted (gci)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Success
🔇 Additional comments (13)
v2/pkg/engine/plan/path_builder_visitor.go (1)
46-47: LGTM!The comment clarification improves documentation by specifying that the map key represents a
fieldRef, making the code's intent clearer.execution/engine/execution_engine.go (1)
219-219: LGTM - Test observability field assignment.Storing the plan for test inspection is a reasonable approach. The field is unexported and documented for testing purposes only.
v2/pkg/engine/plan/node_selection_visitor.go (1)
37-37: LGTM - Comment clarifies semantic meaning.The comment update clarifies that the
intkey represents afieldRef, which improves code readability. The actual type remainsmap[int]struct{}.Note: The AI-generated summary states the type changed to
map[fieldRef]struct{}, but the actual code shows the type is stillmap[int]struct{}. The change appears to be documentation-only.v2/pkg/ast/ast_type.go (2)
253-263: LGTM - Clean extraction of type resolution logic.The method correctly unwraps wrapped types (NonNull, List) until reaching the underlying named type. This is already used by
ResolveTypeNameBytesabove, making this a good refactoring for reuse.Minor style note: There's an extra blank line before the closing brace on line 261.
265-274: LGTM - Useful utility for list-aware type resolution.This method provides a way to unwrap non-null wrappers while preserving list type information, which is useful for cost calculations and other type introspection scenarios.
v2/pkg/engine/plan/plan.go (2)
21-55: LGTM: simple, non-invasive storage/accessors for StaticCost.Straightforward getters/setters on both plan types; default zero cost is reasonable when not computed.
14-19: No action needed—interface expansion is expected in v2 RC.While adding methods to an interface is technically source-breaking in Go, v2 is in Release Candidate phase where breaking changes in minor releases are explicitly expected and documented. The Plan interface is an internal engine component with no external implementations. This concern does not require addressing before v2.0.0.
v2/pkg/engine/plan/static_cost.go (6)
5-17: LGTM!The default cost values are sensible and well-documented. The structure provides clear separation of concerns for different GraphQL element types.
19-32: LGTM!The
HasWeightflag pattern effectively distinguishes between "weight explicitly set to 0" and "no weight configured", which is important for correct cost calculation semantics.
53-70: LGTM!The multiplier calculation logic correctly implements the fallback chain: slicing argument values → AssumedSize → global default. The use of
-1as a sentinel and taking the maximum among slicing arguments is appropriate.
209-237: LGTM!The
TotalCostrecursive calculation correctly handles nil nodes, applies multipliers to children costs, and floors negative argument/directive costs to zero. The documented deviation from the IBM spec (multiplying field cost by the multiplier) is reasonable and explained in the comments.
239-263: LGTM!The
ArgumentInfostruct is well-documented with clear explanations of each field's purpose, including the future use case forcoordCountswith input objects.
265-276: LGTM!The
CostTreestructure andCalculatemethod are straightforward with appropriate nil checking.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@execution/engine/execution_engine_test.go`:
- Around line 337-342: Replace the nil-check assertion on engine.lastPlan with a
fail-fast require to avoid panics: when verifying lastPlan in the test
(referencing engine.lastPlan and local variable lastPlan) use require.NotNil(t,
lastPlan) instead of assert.NotNil so the test stops immediately if lastPlan is
nil before calling lastPlan.GetStaticCostCalculator() and
costCalc.GetTotalCost().
In `@v2/pkg/engine/plan/plan.go`:
- Around line 26-28: GetStaticCost on SynchronousResponsePlan can panic if
StaticCostCalculator is nil; modify SynchronousResponsePlan.GetStaticCost to
guard against a nil StaticCostCalculator (e.g., if s.StaticCostCalculator == nil
return 0) before calling GetTotalCost() so it safely returns a default cost when
ComputeStaticCost is disabled.
In `@v2/pkg/engine/plan/static_cost.go`:
- Around line 204-206: Remove the stray fmt.Printf debug prints in the cost
calculation path (the statements printing "found better maxWeight for %v: %v"
and the other debug lines around where maxWeight, fieldWeight and coord are
used); either delete these fmt.Printf calls or wrap them behind an existing
debug/logger flag so they don't write to stdout in production, ensuring you use
the project's logger (or a debug conditional) consistent with other logging in
static_cost.go and leave the core logic that updates maxWeight/fieldWeight/coord
unchanged.
♻️ Duplicate comments (5)
v2/pkg/engine/plan/visitor.go (3)
466-471: Debug prints remain in production code.The
fmt.Printfstatements on lines 467-468 and 471 will pollute stdout in production. This was previously flagged.
521-530: Debug print and incorrect type name derivation for literal values.Line 521 contains a debug
fmt.Printf. Additionally, lines 525 and 529 usev.Operation.TypeNameString(argValue.Ref)for literal values (Boolean, Enum, String, Float, Integer), butargValue.Refis a value reference, not a type reference. This results in incorrect/empty type names and likely contributes to the failing test mentioned in the PR description.
553-553: Additional debug prints in argument extraction.Lines 553 and 572-574 contain
fmt.Printfstatements that should be removed or gated behind a debug flag.Also applies to: 568-575
v2/pkg/engine/plan/static_cost.go (1)
117-123:ListSizesmap is not initialized in constructor.
NewDataSourceCostConfiginitializesWeightsandTypesbut omitsListSizes, which will cause a nil map panic if callers attempt to add entries.execution/engine/execution_engine_test.go (1)
6003-6048: The slicing arguments test is now enabled and functional.This test was previously skipped due to issues with extracting integer literal values from slicing arguments. The fix (per commit "handle slicingArguments correctly") enables proper cost calculation using the maximum of the slicing arguments (
last: 12= 12) as the list size multiplier.
🧹 Nitpick comments (2)
v2/pkg/engine/plan/static_cost.go (1)
452-453: Typo in comment: "cose" should be "cost".Proposed fix
-// LeaveField calculates the cose of the current node and pop from the cost stack. +// LeaveField calculates the cost of the current node and pops from the cost stack.execution/engine/execution_engine_test.go (1)
272-280: Consider disabling verbose debug flags in committed test code.Multiple debug flags (
PrintOperationTransformations,PrintPlanningPaths,PrintQueryPlans,ConfigurationVisitor,PlanningVisitor) are enabled. This can clutter test output and slow down test execution. These appear to be development-time debugging aids that should typically be disabled before merging.♻️ Suggested change
engineConf.plannerConfig.Debug = plan.DebugConfiguration{ - PrintOperationTransformations: true, - PrintPlanningPaths: true, + PrintOperationTransformations: false, + PrintPlanningPaths: false, // PrintNodeSuggestions: true, - PrintQueryPlans: true, - ConfigurationVisitor: true, - PlanningVisitor: true, + PrintQueryPlans: false, + ConfigurationVisitor: false, + PlanningVisitor: false, // DatasourceVisitor: true, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
execution/engine/execution_engine.goexecution/engine/execution_engine_test.gov2/pkg/engine/plan/plan.gov2/pkg/engine/plan/planner.gov2/pkg/engine/plan/static_cost.gov2/pkg/engine/plan/visitor.gov2/pkg/engine/resolve/context.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/plan/planner.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-09T15:30:57.980Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:30:57.980Z
Learning: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/resolve/context.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/plan/visitor.go
📚 Learning: 2025-11-17T12:47:08.376Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/grpctest/mockservice_resolve.go:721-734
Timestamp: 2025-11-17T12:47:08.376Z
Learning: In Go, protobuf-generated getter methods (GetX()) are nil-safe: they check if the receiver is nil and return the field's default value (nil for message fields, zero values for primitives). This means chained calls like `req.GetFieldArgs().GetThreshold().GetValue()` are safe even when intermediate values are nil. Setters and clearers, however, are NOT nil-safe and will panic on nil receivers.
Applied to files:
v2/pkg/engine/plan/visitor.go
🧬 Code graph analysis (6)
v2/pkg/engine/resolve/context.go (2)
v2/pkg/engine/resolve/variables.go (1)
Variables(27-27)v2/pkg/ast/ast_value.go (1)
Value(32-36)
execution/engine/execution_engine.go (6)
v2/pkg/engine/plan/plan.go (1)
Plan(14-18)v2/pkg/engine/resolve/variables.go (1)
Variables(27-27)v2/pkg/operationreport/operationreport.go (1)
Report(9-12)v2/pkg/engine/plan/static_cost.go (1)
CostCalculator(407-417)v2/pkg/pool/hash64.go (1)
Hash64(10-16)v2/pkg/engine/plan/planner.go (1)
NewPlanner(38-89)
v2/pkg/engine/plan/plan.go (2)
v2/pkg/engine/plan/static_cost.go (1)
CostCalculator(407-417)v2/pkg/engine/resolve/response.go (2)
GraphQLResponse(34-42)GraphQLSubscription(12-16)
v2/pkg/engine/plan/visitor.go (5)
v2/pkg/engine/plan/static_cost.go (3)
CostCalculator(407-417)CostTreeNode(149-193)ArgumentInfo(374-404)v2/pkg/engine/plan/plan.go (3)
Kind(7-7)SynchronousResponsePlan(20-24)SynchronousResponsePlan(38-40)v2/pkg/engine/plan/federation_metadata.go (1)
FieldCoordinate(92-95)v2/pkg/engine/plan/datasource_configuration.go (1)
DSHash(15-15)v2/pkg/ast/ast_value.go (7)
ValueKindBoolean(22-22)ValueKindEnum(29-29)ValueKindString(21-21)ValueKindFloat(24-24)ValueKindInteger(23-23)ValueKindVariable(25-25)ValueKindList(27-27)
execution/engine/execution_engine_test.go (5)
v2/pkg/engine/plan/configuration.go (2)
FieldConfigurations(105-105)FieldConfiguration(116-133)execution/engine/execution_engine.go (1)
NewExecutionEngine(108-144)v2/pkg/engine/resolve/node_custom.go (1)
CustomResolve(7-9)v2/pkg/engine/plan/type_field.go (1)
TypeField(3-8)v2/pkg/engine/plan/static_cost.go (3)
DataSourceCostConfig(94-115)FieldWeight(24-36)FieldListSize(39-56)
v2/pkg/engine/plan/static_cost.go (2)
v2/pkg/engine/plan/federation_metadata.go (1)
FieldCoordinate(92-95)v2/pkg/astvisitor/visitor.go (2)
EnterField(735-735)LeaveField(736-736)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (17)
v2/pkg/engine/resolve/context.go (2)
19-27: LGTM!The field relocation and added documentation for
VariablesandRemapVariablesimprove code clarity. The comments clearly explain the purpose of each field and their relationship during resolution.
211-216: Deep copy ofRemapVariablesis correctly implemented.The clone logic properly creates a new map and copies entries, ensuring the cloned context is independent of the original.
v2/pkg/engine/plan/plan.go (1)
14-17: Interface extension and implementations look correct.The
Planinterface now exposesGetStaticCostCalculator(), and bothSynchronousResponsePlanandSubscriptionResponsePlanimplement it correctly, returning the calculator or nil.Also applies to: 30-32, 42-50
v2/pkg/engine/plan/visitor.go (2)
488-504: Data source hash retrieval is correct.The
getFieldDataSourceHashesfunction correctly maps planner IDs to data source hashes with proper bounds checking. The nil return when no planners are found is acceptable as the cost calculator handles this case.
1186-1197: Plan initialization with cost calculator looks correct.Both
SubscriptionResponsePlanandSynchronousResponsePlanare properly initialized with thecostCalculatorreference.v2/pkg/engine/plan/static_cost.go (4)
58-90:multipliermethod logic is sound.The method correctly picks the maximum value among slicing arguments, falls back to
AssumedSize, and finally toStaticCostDefaults.List. The variable handling with nil checks is appropriate.
231-261: Recursive cost calculation with multiplier application is correct.The
totalCostmethod properly:
- Sets costs via
setCostsAndMultiplier- Recursively sums children costs
- Floors negative argument/directive costs to zero
- Applies multiplier to field + children costs
419-432:NewCostCalculatorcorrectly initializes the cost tree.The root node with
multiplier: 1and the stack initialization are appropriate for the tree-building pattern.
454-470:LeaveFieldsilently returns iffieldRefdoesn't match.If
current.fieldRef != fieldRef, the method returns without popping the stack. This could leave the stack in an inconsistent state if there's a mismatch between EnterField and LeaveField calls.Consider adding a warning or debug log when this mismatch occurs to aid debugging:
if current.fieldRef != fieldRef { + // This shouldn't happen in normal operation - consider logging return }execution/engine/execution_engine.go (3)
65-66:lastPlanfield added for test visibility.Storing the plan after execution enables test assertions on static cost. The field is appropriately non-exported and documented.
212-219: Cost calculator variable injection is correctly placed.Setting variables on the cost calculator after plan retrieval ensures the calculator has access to runtime variable values for accurate cost computation (e.g., for slicing arguments).
242-269:getCachedPlancorrectly returns cost calculator alongside plan.The function properly:
- Returns
(nil, nil)on errors- Retrieves the cost calculator from cached plans
- Attaches the cost calculator to newly created plans
The cost calculator is shared between executions using the same cached plan, but this is safe because execution is synchronous and atomic per request.
SetVariables()is called once before resolution begins, and the calculator's variables field is not accessed again during that execution. The cost calculator itself is never invoked during execution (GetTotalCost() is unused in the codebase except in tests), so no race condition exists.execution/engine/execution_engine_test.go (5)
203-217: LGTM!The new fields in
ExecutionEngineTestCaseare well-structured and support the static cost feature appropriately. TheskipReasonpattern is a good practice for documenting why tests are skipped.
250-255: LGTM!The
computeStaticCost()option function follows the established functional options pattern used elsewhere in this file.
894-894: LGTM!The field name correction from
primaryFunctionstoprimaryFunctionaligns with the standard Star Wars schema where a Droid has a single primary function.
5550-5570: LGTM!The static cost computation test suite is well-structured with comprehensive coverage of various scenarios including weighted fields, interfaces, fragments, and list sizes. The inline comments documenting the expected cost calculations are helpful for maintainability.
6157-6157: LGTM!The updated
getCachedPlancalls correctly discard the new second return value using_, which is appropriate for these tests that only need to verify plan caching behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/plan/static_cost.go`:
- Around line 323-327: The lookup of dsCostConfig from configs[dsHash] can yield
a nil pointer (since SetDataSourceCostConfig accepts nil), so before accessing
dsCostConfig.Weights in the block that follows add a nil guard: check if
dsCostConfig == nil and if so log or warn and continue (same behavior as the
existing missing-config case). Update the code path around the configs[dsHash]
retrieval in static_cost.go (the block that currently does fmt.Printf("WARNING:
no cost dsCostConfig for data source %v\n", dsHash) and then accesses
dsCostConfig.Weights) to handle a nil dsCostConfig safely to avoid panics.
♻️ Duplicate comments (2)
v2/pkg/engine/plan/static_cost.go (1)
325-339: Avoid fmt.Printf in cost calculation paths.These warnings will pollute stdout in production. Prefer the project logger or remove the prints.
execution/engine/execution_engine_test.go (1)
337-343: Fail fast if the static cost calculator is nil.This avoids a potential panic if a test sets
expectedStaticCostbut the calculator isn’t present.💡 Proposed tweak
if testCase.expectedStaticCost != 0 { lastPlan := engine.lastPlan require.NotNil(t, lastPlan) costCalc := lastPlan.GetStaticCostCalculator() + require.NotNil(t, costCalc, "static cost calculator should not be nil") gotCost := costCalc.GetStaticCost() // fmt.Println(costCalc.DebugPrint()) require.Equal(t, testCase.expectedStaticCost, gotCost) }
🧹 Nitpick comments (2)
v2/pkg/engine/plan/static_cost.go (2)
317-321: Reset directivesCost with other per-call fields.This keeps the computation idempotent if directive weights are added later.
♻️ Proposed tweak
- node.fieldCost = 0 - node.argumentsCost = 0 - node.multiplier = 0 + node.fieldCost = 0 + node.argumentsCost = 0 + node.directivesCost = 0 + node.multiplier = 0
495-559: Compute costs before printing in DebugPrint.
DebugPrintprints cached costs before recalculating them; ifGetStaticCost()hasn’t been called, output can be stale. Consider computing once at the top and reusing the result.♻️ Proposed tweak
func (node *CostTreeNode) debugPrint(sb *strings.Builder, configs map[DSHash]*DataSourceCostConfig, variables *astjson.Value, depth int) { // implementation is a bit crude and redundant, we could skip calculating nodes all over again. // but it should suffice for debugging tests. if node == nil { return } + subtreeCost := node.staticCost(configs, variables) indent := strings.Repeat(" ", depth) @@ - subtreeCost := node.staticCost(configs, variables) fmt.Fprintf(sb, "%s cost=%d\n", indent, subtreeCost)
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)
v2/pkg/engine/plan/configuration.go (1)
3-7: Fix gci import grouping.Linters report a gci formatting failure; remove the extra blank line between third‑party imports (or run the formatter).
🧹 Proposed fix
import ( "github.com/jensneuse/abstractlogger" - "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/resolve" )
♻️ Duplicate comments (2)
execution/engine/execution_engine_test.go (1)
338-343: Guard against nil static-cost calculator.If
GetStaticCostCalculator()returns nil,GetStaticCost()will panic. Fail fast before use.🧪 Proposed fix
lastPlan := engine.lastPlan require.NotNil(t, lastPlan) costCalc := lastPlan.GetStaticCostCalculator() +require.NotNil(t, costCalc, "static cost calculator should not be nil") gotCost := costCalc.GetStaticCost() require.Equal(t, testCase.expectedStaticCost, gotCost)v2/pkg/engine/plan/static_cost.go (1)
315-324: Handle nil cost configs to avoid panic.
SetDataSourceCostConfigaccepts a nil config; dereferencing it here will panic. Add a nil guard when fetching from the map.🛡️ Proposed fix
- dsCostConfig, ok := configs[dsHash] - if !ok { - dsCostConfig = &DataSourceCostConfig{} - // Save it for later use by other fields: - configs[dsHash] = dsCostConfig - } + dsCostConfig, ok := configs[dsHash] + if !ok || dsCostConfig == nil { + dsCostConfig = NewDataSourceCostConfig() + // Save it for later use by other fields: + configs[dsHash] = dsCostConfig + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/plan/configuration.go`:
- Around line 50-54: NewCostCalculator currently accepts
StaticCostDefaultListSize as-is, so when ComputeStaticCost is true a zero or
negative value will undercount list multipliers; update the constructor or the
planner/config instantiation to validate and enforce a safe default (>=1) for
StaticCostDefaultListSize: if ComputeStaticCost is true and
StaticCostDefaultListSize <= 0 then set it to 1 (or return an error), and add a
short comment documenting the invariant; reference the config field
StaticCostDefaultListSize and the NewCostCalculator function when making the
change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/plan/static_cost.go`:
- Around line 316-324: The code assumes configs[dsHash] yields a non-nil
*DataSourceCostConfig but if SetDataSourceCostConfig(dsHash, nil) was called you
can get (nil, true) and panic when reading dsCostConfig.Weights; fix by checking
whether dsCostConfig == nil after the map lookup and if so allocate a new
DataSourceCostConfig (and store it back into configs[dsHash]) before using its
fields; reference the variables/types configs, dsCostConfig,
DataSourceCostConfig and the use site where node.fieldCoord is accessed so the
nil check/initialization happens prior to reading Weights or ListSizes.
♻️ Duplicate comments (1)
v2/pkg/engine/plan/static_cost.go (1)
329-334: Add nil guard forparentbefore accessingparent.returnsAbstractType.If
parentis nil (e.g., at the root of the tree), accessingparent.returnsAbstractTypewill panic. While this may be unlikely in normal usage, a defensive check improves robustness.Proposed fix
- if fieldWeight != nil && node.isEnclosingTypeAbstract && parent.returnsAbstractType { + if fieldWeight != nil && parent != nil && node.isEnclosingTypeAbstract && parent.returnsAbstractType { // Composition should not let interface fields have weights, so we assume that // the enclosing type is concrete. fmt.Printf("WARNING: cost directive on field %v of interface %v\n", node.fieldCoord, parent.fieldCoord) } - if node.isEnclosingTypeAbstract && parent.returnsAbstractType { + if parent != nil && node.isEnclosingTypeAbstract && parent.returnsAbstractType {
🧹 Nitpick comments (2)
v2/pkg/engine/plan/static_cost.go (2)
332-332: Remove debugfmt.Printfstatement.This warning print to stdout should be removed or replaced with proper logging. Debug prints can pollute output in production.
Proposed fix
- fmt.Printf("WARNING: cost directive on field %v of interface %v\n", node.fieldCoord, parent.fieldCoord) + // Note: Composition should not allow interface fields to have weights. + // If this code path is reached, it indicates a composition bug.
380-385: TODO acknowledged for input object argument costs.The TODO at line 380 is tracked in the file's documented TBD list. The extra blank line before the closing brace (line 385) is a minor style nit.
Minor cleanup
} else if arg.isInputObject { // TODO: arguments should include costs of input object fields } else { node.argumentsCost += dsCostConfig.ObjectTypeWeight(arg.typeName) } - }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/plan/static_cost.go`:
- Around line 473-480: The CostCalculator is not thread-safe: SetVariables
mutates c.variables and GetStaticCost calls c.tree.staticCost which (via
setCostsAndMultiplier) mutates node fields (fieldCost, argumentsCost,
multiplier), causing data races when the cached CostCalculator is reused
concurrently. Fix by avoiding shared mutation: either make CostCalculator
request-scoped (clone the calculator or its tree before calling
SetVariables/GetStaticCost), add synchronization around variable reads/writes
and tree mutations (e.g., a mutex on CostCalculator used in SetVariables and
GetStaticCost), or refactor staticCost/setCostsAndMultiplier to compute costs
purely functionally without mutating node fields and use c.variables immutably;
update usages of CostCalculator, SetVariables, GetStaticCost, staticCost, and
setCostsAndMultiplier accordingly.
🧹 Nitpick comments (1)
v2/pkg/engine/plan/static_cost.go (1)
482-492: DebugPrint only outputs the first root field.For multi-root operations,
children[0]drops the rest. Consider iterating all root children.♻️ Suggested tweak
- c.tree.children[0].debugPrint(&sb, c.costConfigs, c.variables, c.defaultListSize, 0) + for _, child := range c.tree.children { + child.debugPrint(&sb, c.costConfigs, c.variables, c.defaultListSize, 0) + }
… yury/eng-8635-engine-compute-static-cost
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@execution/graphql/request.go`:
- Around line 197-199: The method on type Request is misnamed
ComputerStaticCost; rename it to ComputeStaticCost and update all
references/call sites to use Request.ComputeStaticCost instead of
ComputerStaticCost; keep the body unchanged (it should still call
calc.GetStaticCost(config, variables) and assign to r.staticCost) and update any
interfaces or tests that reference the old name to avoid build errors.
- Around line 9-12: The import block in request.go is misformatted causing the
CI pipeline to fail; fix it by reordering and formatting imports using the
project's formatter (e.g., run "gci write" or "gofmt -w") so the imports
"github.com/wundergraph/astjson",
"github.com/wundergraph/graphql-go-tools/v2/pkg/ast",
"github.com/wundergraph/graphql-go-tools/v2/pkg/astparser", and
"github.com/wundergraph/graphql-go-tools/v2/pkg/engine/plan" follow the
project's import grouping/order conventions and then commit the formatted file.
♻️ Duplicate comments (1)
v2/pkg/engine/plan/static_cost_visitor.go (1)
154-206: Handle literal argument values inextractFieldArguments.Only variables are processed; literal arguments (e.g.,
first: 5) currently end up with emptyArgumentInfo, which makes list-size multipliers fall back toAssumedSize. This breaks static cost for literal slicing args. Please add handling for literal kinds (ValueKindInteger,ValueKindFloat,ValueKindString,ValueKindBoolean, etc.) using the existing AST helpers for value extraction.#!/bin/bash # Find how literal values are extracted elsewhere rg -n "ValueKindInteger|ValueKindFloat|ValueKindString|ValueKindBoolean" v2/pkg/engine/plan -g '*.go' rg -n "IntValue|FloatValue|ValueToInt|ValueToString|ValueToFloat" v2/pkg/ast -g '*.go'
🧹 Nitpick comments (2)
v2/pkg/engine/plan/static_cost.go (1)
319-323: Remove or gate the debugfmt.Printfstatement.This warning print statement will pollute stdout in production. Consider removing it or gating it behind a debug flag/logger.
Proposed fix
if fieldWeight != nil && node.isEnclosingTypeAbstract && parent.returnsAbstractType { // Composition should not let interface fields have weights, so we assume that // the enclosing type is concrete. - fmt.Printf("WARNING: cost directive on field %v of interface %v\n", node.fieldCoord, parent.fieldCoord) + // This should not happen if composition is correct. }execution/engine/execution_engine_test.go (1)
338-343: Consider allowing explicit assertions for a zero static cost.The
expectedStaticCost != 0guard skips validation when zero is the expected value. If0is a valid expectation, consider adding an explicit flag (e.g.,hasExpectedStaticCost) or switching to a pointer.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@execution/engine/execution_engine.go`:
- Around line 216-221: The code leaves operation.staticCost stale when
costCalculator is nil; update the call site so ComputeStaticCost is invoked
unconditionally on operation (e.lastPlan = cachedPlan; then call
operation.ComputeStaticCost(...)) and modify ComputeStaticCost to internally
handle a nil costCalculator (or add a nil-safe wrapper) so it resets staticCost
when costCalculator == nil; reference operation.ComputeStaticCost,
costCalculator, e.config.plannerConfig, and execContext.resolveContext.Variables
to locate and change the logic.
- Around line 65-66: The test-only field lastPlan on ExecutionEngine is written
by Execute and raced across concurrent ServeHTTP requests; add synchronization
by introducing a mutex (e.g., lastPlanMu sync.RWMutex) on the ExecutionEngine
struct, lock it for writes around assignments to lastPlan in Execute and for
reads in a new exported getter LastPlan() that returns the plan safely, and
update tests to call engine.LastPlan() instead of accessing engine.lastPlan
directly.
In `@execution/graphql/request.go`:
- Around line 49-50: The Request struct's staticCost field can retain a stale
value when no calculator is present; update the cost-computation path (e.g., in
Request.ComputeStaticCost and any helpers that set StaticCost()/StaticCost) to
explicitly reset request.staticCost to zero (or a defined zero-state) whenever
the calculator is nil or cost calculation is skipped, and keep the nil-guard so
callers can call ComputeStaticCost unconditionally; reference Request,
staticCost, ComputeStaticCost, and StaticCost() when making the change.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.246](v2.0.0-rc.245...v2.0.0-rc.246) (2026-01-27) ### Features * compute static costs ([#1359](#1359)) ([004f68e](004f68e)) --- 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 compute static costs feature in v2 * **Chores** * Released v2 version 2.0.0-rc.246 <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
First release of essential features for the static cost analysis on fields, lists.
All parameters of "cost" and "listSize" directives are supported, except for "sizedFields":
Notes
Planning visitor collects information for the costCalculator via EnterField and LeaveField hooks.
Calculator builds a tree of nodes, each node corresponding to the requested field.
After the planning is done, a callee could get a ref to the calculator and request cost calculation.
Cost calculation walks the previously built tree and using variables provided with operation,
estimates the static cost.
It builds on top of IBM spec for @cost and @listsize directive with a few changes:
this weight (along with children's costs) is multiplied too.
A few things on the TBD list:
Fixes ENG-8732
Summary by CodeRabbit
New Features
Configuration
API
Execution / Planning
Tests
✏️ Tip: You can customize this high-level summary in your review settings.