Skip to content

feat: enable to ignore cost weights on implementing types#1542

Merged
ysmolski merged 4 commits into
masterfrom
yury/eng-9569-interface-field-cost-uses-max-across-implementations-instead
Jun 18, 2026
Merged

feat: enable to ignore cost weights on implementing types#1542
ysmolski merged 4 commits into
masterfrom
yury/eng-9569-interface-field-cost-uses-max-across-implementations-instead

Conversation

@ysmolski

@ysmolski ysmolski commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

This PR adds a feature flag that allows to emulate behavior
of Apollo that ignored weights on implementing types when
the fields on interface does not have cost weight assigned.

This PR adds a feature flag that allows to emulate behavior
of Apollo that ignored weights on implementing types when
the fields on interface does not have cost weight assigned.
@ysmolski ysmolski requested a review from a team as a code owner June 17, 2026 09:32
@ysmolski ysmolski changed the title feat: enable to ignore implementing types on abstract fields feat: enable to ignore cost weights on implementing types Jun 17, 2026
@coderabbitai

coderabbitai Bot commented Jun 18, 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: ab116206-2a7f-4585-8417-30fae60eacbf

📥 Commits

Reviewing files that changed from the base of the PR and between 507f612 and 89e8ecf.

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

📝 Walkthrough

Walkthrough

Adds IgnoreImplementingTypeWeights to Configuration, stores it in CostCalculator, and threads it through costInput as ignoreImplementingTypeWeights. Two guard conditions in costsAndMultiplier disable implementing-type field weight and directive argument weight resolution when the flag is set. newCostInput is refactored to accept *CostCalculator directly, and defaultListSize is floored to 1. Engine-level test harness and a full suite of cost test cases are added.

Changes

IgnoreImplementingTypeWeights feature

Layer / File(s) Summary
Configuration field and CostCalculator storage
v2/pkg/engine/plan/configuration.go, v2/pkg/engine/plan/cost.go
Adds IgnoreImplementingTypeWeights bool to Configuration and stores it in CostCalculator.ignoreImplementingTypeWeights; NewCostCalculator also floors defaultListSize to at least 1 via max.
costInput refactor and costsAndMultiplier gates
v2/pkg/engine/plan/cost.go
Refactors newCostInput to accept *CostCalculator and wires ignoreImplementingTypeWeights into costInput; two conditions in costsAndMultiplier (field weight resolution and directive argument weight summation) are gated on the new flag; DebugPrint input construction is updated to the new signature.
Test harness option and engine-level tests
execution/engine/execution_engine_test.go, execution/engine/execution_engine_cost_test.go
Adds ignoreImplementingTypeWeights field to _executionTestOptions, wires it into plannerConfig in runExecutionTest, adds costsIgnoreImplementingTypeWeights() helper, and introduces the skipImplementingTypesOnAbstract suite covering abstract-only, concrete fragment, assumedSize list, named fragment, and interface-field regression scenarios.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • wundergraph/graphql-go-tools#1465: Adds directive-argument weight handling across implementing fields on abstract paths in cost.go, directly in the same logic paths this PR now gates with the new flag.
  • wundergraph/graphql-go-tools#1521: Modifies abstract cost aggregation in cost.go to avoid double-counting based on TypeNameStats, intersecting with the same implementing-type weight resolution code this PR conditionally skips.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: enable to ignore cost weights on implementing types' clearly summarizes the main change, which adds a feature flag to ignore cost weights on implementing types.
Description check ✅ Passed The description explains that the PR adds a feature flag to emulate Apollo's behavior of ignoring weights on implementing types when interface fields lack cost weights, which aligns with the changeset.
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-9569-interface-field-cost-uses-max-across-implementations-instead

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 589-590: The actual-cost path for abstract-list normalization is
not respecting the skipImplementingTypesOnAbstract flag. In the section around
line 775 where implementing field weights are read (around lines 790-794), add a
check to skip reading implementing field weights when
skipImplementingTypesOnAbstract is enabled, similar to how it's checked at line
589. Additionally, add a regression test that sets
IgnoreImplementingTypeWeights=true with an abstract-list child field containing
non-zero default type or scalar weights, then verify that implementing field
weights remain zero in actual-cost mode and are not reintroduced despite the
non-zero defaults.
🪄 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: f8510e00-4df0-43b9-9d54-71f9850cfed2

📥 Commits

Reviewing files that changed from the base of the PR and between 6a5eb1a and 507f612.

📒 Files selected for processing (4)
  • execution/engine/execution_engine_cost_test.go
  • execution/engine/execution_engine_test.go
  • v2/pkg/engine/plan/configuration.go
  • v2/pkg/engine/plan/cost.go

Comment thread v2/pkg/engine/plan/cost.go Outdated
@ysmolski ysmolski merged commit 1ade300 into master Jun 18, 2026
10 checks passed
@ysmolski ysmolski deleted the yury/eng-9569-interface-field-cost-uses-max-across-implementations-instead branch June 18, 2026 14:36
ysmolski pushed a commit that referenced this pull request Jun 18, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.5.0](v2.4.6...v2.5.0)
(2026-06-18)


### Features

* enable to ignore cost weights on implementing types
([#1542](#1542))
([1ade300](1ade300))


### Bug Fixes

* **cost:** determine correctly the type of list-wrapped scalars and
enums
([#1546](#1546))
([2575480](2575480))
* remove connections from transport connection pool
([#1541](#1541))
([ca47200](ca47200))
* **resolve:** fix GC-liveness bug in the loadBatchEntityFetch
([#1536](#1536))
([2ad6fc4](2ad6fc4))

---
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 18, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.16.0](execution/v1.15.6...execution/v1.16.0)
(2026-06-18)


### Features

* enable to ignore cost weights on implementing types
([#1542](#1542))
([1ade300](1ade300))


### Bug Fixes

* **cost:** determine correctly the type of list-wrapped scalars and
enums
([#1546](#1546))
([2575480](2575480))

---
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