Skip to content

fix: calculate costs for abstract fields without double counting - #1521

Merged
ysmolski merged 10 commits into
masterfrom
yury/eng-9581-inline-fragments-on-abstract-list-types-charge-all-items
Jun 5, 2026
Merged

fix: calculate costs for abstract fields without double counting#1521
ysmolski merged 10 commits into
masterfrom
yury/eng-9581-inline-fragments-on-abstract-list-types-charge-all-items

Conversation

@ysmolski

Copy link
Copy Markdown
Contributor

This PR fixes how costs for abstract types are calculated. Especially for fragments used on abstract types.

For example, for actual costs, instead of summing each type of the union, engine sums only costs for types actually seen in the returned results.

For estimated costs, instead of summing all the implementing nodes of abstract types, we just pick the implementing type with the maximum cost and add that to the costs of fields selected on the abstract type itself.

I had to recalculate and verify numbers in tests.

Unfortunately, this fix is not final. There is unfinished work with how the cost of the field itself is selected. Right now we pick the maximum weight across implementing types and dataSources independently from children costs selection. This concerns the estimation the most. The better approach would be to be tie this process with the selection of the type with maximum cost.

@ysmolski
ysmolski requested a review from a team as a code owner May 29, 2026 14:46

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1fbb71ed-50c0-4db0-a8ec-2fd2b6e5feaa

📥 Commits

Reviewing files that changed from the base of the PR and between e6ace3c and 8f673b4.

📒 Files selected for processing (3)
  • execution/engine/execution_engine_cost_test.go
  • execution/graphql/request.go
  • v2/pkg/engine/plan/cost.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • execution/graphql/request.go
  • v2/pkg/engine/plan/cost.go
  • execution/engine/execution_engine_cost_test.go

📝 Walkthrough

Walkthrough

This PR replaces integer list-size tracking with TypeNameStats (Size + per-typename counts). The resolver records per-path TypeNameStats, Context exposes TypeNameStats, cost computation uses TypeNameStats for actual multipliers, call sites are updated, and tests are re-baselined.

Changes

TypeNameStats Migration

Layer / File(s) Summary
TypeNameStats model and runtime collection
v2/pkg/engine/resolve/resolvable.go
Introduce TypeNameStats and collect per-path Size and __typename distributions during array/object pre-walk.
Context and resolver propagation
v2/pkg/engine/resolve/context.go, v2/pkg/engine/resolve/resolve.go
Replace ActualListSizes with TypeNameStats on Context; resolver assigns resolvable.typeNameStats into ctx.TypeNameStats and Free clears it.
Cost defaults and list-size initialization
v2/pkg/engine/plan/cost.go
Add unexported default weights and sentinels; initialize FieldListSize.multiplier and fallback behavior for list sizing.
Core cost computation and abstract handling
v2/pkg/engine/plan/cost.go
Introduce costNodeResult; refactor node cost aggregation, field/arg/directive accumulation, and switch abstract-child aggregation and multiplier logic to use TypeNameStats in actual mode.
Cost public API and debug printing
v2/pkg/engine/plan/cost.go
Change CostCalculator.ActualCost/DebugPrint to accept map[string]resolve.TypeNameStats and emit structured per-node component debug output.
Execution/request wiring
execution/graphql/request.go, execution/engine/execution_engine.go
Request.ComputeActualCost and synchronous ExecutionEngine.Execute now pass TypeNameStats into cost calculation.
Tests, fixtures, and rebaselines
execution/engine/execution_engine_cost_test.go, execution/engine/execution_engine_test.go
Re-baseline many expected estimated/actual costs, add a comprehensive abstract-list concrete-fragment test suite, update fixtures to include concrete __typename entries, and add t.Parallel() in several subtests.
Object abstractness helper
v2/pkg/engine/resolve/node_object.go
Add Object.isAbstract() helper to detect abstract-returning object semantics used by resolver/type-stat collection.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: calculate costs for abstract fields without double counting' accurately and specifically describes the main change in the PR, which refactors cost calculation for abstract GraphQL types.
Description check ✅ Passed The description clearly explains how costs for abstract types are calculated differently for actual and estimated costs, with specific examples and acknowledgment of incomplete work.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch yury/eng-9581-inline-fragments-on-abstract-list-types-charge-all-items

Comment @coderabbitai help to get the list of available commands and usage tips.

This PR fixes how costs for abstract types are calculated.
Especially for fragments used on abstract types.

For example, for actual costs, instead of summing each type of the
union, engine sums only costs for types actually seen in the returned
results.

For estimated costs, instead of summing all the implementing nodes of
abstract types, we just pick the implementing type with the maximum cost
and add that to the costs of fields selected on the abstract type
itself.

I had to recalculate and verify numbers in tests.

Unfortunately, this fix is not final. There is unfinished work with how
the cost of the field itself is selected. Right now we pick the maximum
weight across implementing types and dataSources independently from
children costs selection. This concerns the estimation the most.
The better approach would be to be tie this process with the selection of
the type with maximum cost.
@ysmolski
ysmolski force-pushed the yury/eng-9581-inline-fragments-on-abstract-list-types-charge-all-items branch from f831874 to d4a6742 Compare May 29, 2026 14:48
@ysmolski

ysmolski commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@ysmolski

ysmolski commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@claude review once

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, comment @claude review on this pull request to trigger a review.

@ysmolski
ysmolski merged commit 4175a9e into master Jun 5, 2026
10 checks passed
@ysmolski
ysmolski deleted the yury/eng-9581-inline-fragments-on-abstract-list-types-charge-all-items branch June 5, 2026 09:58
asoorm pushed a commit that referenced this pull request Jun 5, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.4.3](v2.4.2...v2.4.3)
(2026-06-05)


### Bug Fixes

* calculate costs for abstract fields without double counting
([#1521](#1521))
([4175a9e](4175a9e))
* **jsonschema:** align nullability with JSON Schema 2020-12
([#1518](#1518))
([6fcdf8c](6fcdf8c))

---
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>
ysmolski pushed a commit that referenced this pull request Jun 8, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.15.5](execution/v1.15.4...execution/v1.15.5)
(2026-06-08)


### Bug Fixes

* calculate costs for abstract fields without double counting
([#1521](#1521))
([4175a9e](4175a9e))
* fallback to abstract name if no __typename was sent
([#1527](#1527))
([2548d5a](2548d5a))

---
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants