feat: support costs on arguments of directives#1465
Merged
Conversation
I have opted for the simple base layer without recursion. Directives could have input objects passed to arguments, where fields could have costs. We do not handle such recursion here. I settled on the arguments level of directives.
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy,
dkorittki and
jensneuse
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRenames FieldWeight→FieldCost, adds per-directive-argument weights, propagates FieldCost through cost plumbing, computes directive-argument weights (including max-union across implementations when parent returns abstract types), and updates tests and introspection/schema fixtures to exercise directive-argument cost behavior. (50 words) Changes
Sequence DiagramsequenceDiagram
participant Schema as Schema (Field Def)
participant Visitor as Cost Visitor
participant TreeNode as Cost TreeNode
participant Calculator as Cost Calculator
Schema->>Visitor: Encounter field with directives (e.g. `@approx`)
Visitor->>Visitor: Resolve directive args (provided, default, or null)
Visitor->>TreeNode: Create/attach node with FieldCost and directive-arg keys
TreeNode->>Calculator: Request costs & multiplier for node
Calculator->>Calculator: If parent returns abstract type → gather implementing fields
Calculator->>Calculator: Compute max/union of DirectiveArgumentWeights across impls
Calculator->>Calculator: Sum field cost + directive-argument costs + args cost → total
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (1)
v2/pkg/engine/plan/cost.go (1)
138-142: Keep the default cost config constructor in sync with the new map field.
NewDataSourceCostConfig()still leavesDirectiveArgumentsnil. Reads are fine, but incremental builders now need a separatemakebefore their first assignment. Initializing it in the constructor keeps this section consistent with the other map-backed config blocks.♻️ Suggested change
func NewDataSourceCostConfig() *DataSourceCostConfig { return &DataSourceCostConfig{ - Weights: make(map[FieldCoordinate]*FieldWeight), - ListSizes: make(map[FieldCoordinate]*FieldListSize), - Types: make(map[string]int), + Weights: make(map[FieldCoordinate]*FieldWeight), + ListSizes: make(map[FieldCoordinate]*FieldListSize), + Types: make(map[string]int), + DirectiveArguments: make(map[DirectiveArgCoords]int), } }🤖 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 138 - 142, The NewDataSourceCostConfig constructor currently leaves the DirectiveArguments map nil; update NewDataSourceCostConfig to initialize DirectiveArguments (map[DirectiveArgCoords]int) to an empty map so callers can assign into it without a separate make, keeping it consistent with other map-backed config fields like FieldCosts and ArgumentCosts; ensure the map is created when the DataSourceCostConfig struct is constructed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/plan/cost_visitor.go`:
- Around line 69-70: The code only calls
extractFieldDirectives(fieldDefinitionRef) and thus misses directives present on
implementing fields; update the logic (around where directiveArgs :=
v.extractFieldDirectives(fieldDefinitionRef)) to also resolve and merge
directive args from all concrete implementing field definitions (e.g., iterate
the type/interface implementations for the abstract field and call
extractFieldDirectives on each implementing field), then apply the existing
upper-bound strategy used for abstract-field handling to compute and use the
max/merged directive values; apply the same change for the other occurrences
noted (the block covering the logic analogous to lines 270-319).
---
Nitpick comments:
In `@v2/pkg/engine/plan/cost.go`:
- Around line 138-142: The NewDataSourceCostConfig constructor currently leaves
the DirectiveArguments map nil; update NewDataSourceCostConfig to initialize
DirectiveArguments (map[DirectiveArgCoords]int) to an empty map so callers can
assign into it without a separate make, keeping it consistent with other
map-backed config fields like FieldCosts and ArgumentCosts; ensure the map is
created when the DataSourceCostConfig struct is constructed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eba32a71-439b-4c84-8b80-2fcab5a7e2b7
📒 Files selected for processing (4)
execution/engine/execution_engine_cost_test.gov2/pkg/engine/plan/cost.gov2/pkg/engine/plan/cost_visitor.gov2/pkg/starwars/testdata/star_wars.graphql
… yury/eng-8700-router-support-cost-on-the-arguments-of-directives
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/plan/cost.go`:
- Around line 500-510: The code currently adds all configured
DirectiveArgumentWeights regardless of whether a directive/argument was actually
present; update the data model and summation so only applied/coerced directive
arguments are charged: add a field on CostTreeNode (e.g., AppliedDirectiveArgs
or AppliedDirectives map keyed by directive+arg) that is populated during
parsing/coercion, then change the loops in cost.go that call
parent.maxDirectiveArgumentWeightsImplementingFields(...) and iterate
fieldWeight.DirectiveArgumentWeights to filter by node.AppliedDirectiveArgs (or
have maxDirectiveArgumentWeightsImplementingFields accept the node or its
applied set) so you only accumulate weights for directives/arguments present
after coercion/defaulting for node.fieldCoords.FieldName and fieldWeight.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec696c39-ee40-4e64-a042-8779c761bd63
📒 Files selected for processing (8)
execution/engine/execution_engine_cost_test.goexecution/engine/execution_engine_test.goexecution/engine/testdata/full_introspection.jsonexecution/engine/testdata/full_introspection_with_deprecated.jsonexecution/engine/testdata/full_introspection_with_typenames.jsonv2/pkg/engine/plan/cost.gov2/pkg/engine/plan/cost_visitor.gov2/pkg/starwars/testdata/star_wars.graphql
✅ Files skipped from review due to trivial changes (4)
- execution/engine/testdata/full_introspection_with_typenames.json
- execution/engine/testdata/full_introspection_with_deprecated.json
- execution/engine/testdata/full_introspection.json
- v2/pkg/engine/plan/cost_visitor.go
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/starwars/testdata/star_wars.graphql
- execution/engine/execution_engine_cost_test.go
… yury/eng-8700-router-support-cost-on-the-arguments-of-directives
devsergiy
approved these changes
Apr 23, 2026
…rguments-of-directives
ysmolski
deleted the
yury/eng-8700-router-support-cost-on-the-arguments-of-directives
branch
ysmolski
pushed a commit
that referenced
this pull request
Apr 27, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0](v2.0.0-rc.270...v2.0.0) (2026-04-27) ### Features * support costs on arguments of directives ([#1465](#1465)) ([2eca1ab](2eca1ab)) ### Bug Fixes * grpc datasource stack overflows on recursive input ([#1466](#1466)) ([eba0f58](eba0f58)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
ysmolski
pushed a commit
that referenced
this pull request
Apr 29, 2026
🤖 I have created a release *beep* *boop* --- ## [1.12.0](execution/v1.11.0...execution/v1.12.0) (2026-04-28) ### Features * support costs on arguments of directives ([#1465](#1465)) ([2eca1ab](2eca1ab)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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.
I have opted for the simple base layer without recursion.
Directives could have input objects passed to arguments,
where fields could have costs.
We do not handle such recursion here.
I settled on the arguments level of directives.