fix: do not charge children of null-parents - #1574
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRuntime-based actual cost calculation now uses runtime type-name stats for abstract/interface billing, separates field and descendant scaling, and updates execution tests for nullable-parent and overlap cases. ChangesRuntime-based actual cost calculation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/resolvable.go (1)
1036-1040: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment describes the wrong branch.
The comment "Fall back to the declared abstract type name when the subgraph did not return
__typename" is placed inside theif typeName != nilblock, but that branch is exactly when the subgraph did return__typename(overriding the default). The actual fallback happens implicitly whentypeName == nil(i.e.,namestays asobj.TypeName), which has no comment at all.📝 Suggested comment fix
- name := obj.TypeName + // Fall back to the declared type name when the subgraph did not return __typename. + name := obj.TypeName if typeName != nil { - // Fall back to the declared abstract type name when the subgraph did not return __typename. name = string(typeName) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/resolvable.go` around lines 1036 - 1040, The inline comment in the type-name resolution logic is attached to the wrong branch in the resolvable handling code. Update the comment near the `name := obj.TypeName` / `if typeName != nil` logic so it describes the actual behavior: the override happens when `typeName` is present, and the fallback to `obj.TypeName` happens when `typeName == nil`. Keep the fix localized to the `resolvable` type-name selection path and align the wording with the `typeName` conditional.execution/engine/execution_engine_cost_test.go (1)
7461-7462: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
t.Parallel()aftert.Skip()is unreachable.
t.Skipcallsruntime.Goexit(), so line 7462 and the entire setup below it never execute. Since this block is intentionally disabled (not implemented yet), it's harmless, but consider dropping the trailingt.Parallel()(and/or guarding the block) to avoid implying it runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/engine/execution_engine_cost_test.go` around lines 7461 - 7462, The test block in execution_engine_cost_test.go has an unreachable t.Parallel() after t.Skip("not implemented yet"), so remove the trailing t.Parallel() from that skipped case (or otherwise guard the block) to avoid suggesting the test executes. Use the surrounding test function containing the skipped subtest to locate and clean up the dead setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/pkg/engine/plan/cost.go`:
- Around line 939-947: Remove the unconditional stdout debug output from the
cost hot path in CostCalculator.EstimateCost and CostCalculator.ActualCost.
Delete the fmt.Println(c.DebugPrint(...)) calls, or guard them behind an
explicit debug flag/logger so production cost calculation stays side-effect
free; keep the actual return path using c.tree.cost(input) unchanged.
- Around line 824-830: The childMultiplier logic in the cost calculation
currently skips missing typeStats entries and falls back to full charging, which
is incorrect for object fields that are always null. In the cost.go path that
updates nodeCost.childMultiplier, treat a missing input.typeStats[node.jsonPath]
entry the same as Size == 0 by assigning a 0 ratio instead of leaving the
default multiplier in place. Keep the existing behavior for nodeStats.Size > 0,
and update the branch around nodeStats/input.typeStats/nodeCost.childMultiplier
so children are not charged in full when the object never appears.
---
Nitpick comments:
In `@execution/engine/execution_engine_cost_test.go`:
- Around line 7461-7462: The test block in execution_engine_cost_test.go has an
unreachable t.Parallel() after t.Skip("not implemented yet"), so remove the
trailing t.Parallel() from that skipped case (or otherwise guard the block) to
avoid suggesting the test executes. Use the surrounding test function containing
the skipped subtest to locate and clean up the dead setup.
In `@v2/pkg/engine/resolve/resolvable.go`:
- Around line 1036-1040: The inline comment in the type-name resolution logic is
attached to the wrong branch in the resolvable handling code. Update the comment
near the `name := obj.TypeName` / `if typeName != nil` logic so it describes the
actual behavior: the override happens when `typeName` is present, and the
fallback to `obj.TypeName` happens when `typeName == nil`. Keep the fix
localized to the `resolvable` type-name selection path and align the wording
with the `typeName` conditional.
🪄 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: 907c5b54-c499-45a0-a91e-8156fc9fbdae
📒 Files selected for processing (3)
execution/engine/execution_engine_cost_test.gov2/pkg/engine/plan/cost.gov2/pkg/engine/resolve/resolvable.go
… yury/eng-9806-cost-is-charged-for-fields-even-though-they-are-not-resolved
423a2b0 to
9553689
Compare
…ven-though-they-are-not-resolved
🤖 I have created a release *beep* *boop* --- ## [2.9.1](v2.9.0...v2.9.1) (2026-07-07) ### Bug Fixes * do not charge children of null-parents ([#1574](#1574)) ([cf436ec](cf436ec)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [1.17.0](execution/v1.16.0...execution/v1.17.0) (2026-07-07) ### Features * add defer support part 4 ([#1547](#1547)) ([8891a0e](8891a0e)) ### Bug Fixes * do not charge children of null-parents ([#1574](#1574)) ([cf436ec](cf436ec)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
This fix includes basic and often used cases, but it does not include some combinations of abstract fields and fragments. For those I have included the test cases (disabled right now). I have simplified how parents are used in calculations. I have added children multiplier to distiungish them from the field cost which is always multiplied even when null was returned on that field. This solution can be sophisticated further if there is a need for it. Potentially, I could take different approach of walking the response from subgraphs and match them with Cost Tree. That would remove many heuristics and exceptions and make things much more simple. But that would happen in a separate PR. `typeNameStats` is populated only when CC is enabled.
🤖 I have created a release *beep* *boop* --- ## [2.9.1](v2.9.0...v2.9.1) (2026-07-07) ### Bug Fixes * do not charge children of null-parents ([#1574](#1574)) ([cf436ec](cf436ec)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [1.17.0](execution/v1.16.0...execution/v1.17.0) (2026-07-07) ### Features * add defer support part 4 ([#1547](#1547)) ([8891a0e](8891a0e)) ### Bug Fixes * do not charge children of null-parents ([#1574](#1574)) ([cf436ec](cf436ec)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
This fix includes basic and often used cases, but it does not include
some combinations of abstract fields and fragments.
For those I have included the test cases (disabled right now).
I have simplified how parents are used in calculations.
I have added children multiplier to distiungish them from the field cost
which is always multiplied even when null was returned on that field.
This solution can be sophisticated further if there is a need for it.
Potentially, I could take different approach of walking the response
from subgraphs and match them with Cost Tree. That would remove
many heuristics and exceptions and make things much more simple.
But that would happen in a separate PR.
typeNameStatsis populated only when CC is enabled.