Skip to content

refactor: improve costs code layering and readability - #1526

Merged
ysmolski merged 4 commits into
masterfrom
yury/refactor_costs
Jun 8, 2026
Merged

refactor: improve costs code layering and readability#1526
ysmolski merged 4 commits into
masterfrom
yury/refactor_costs

Conversation

@ysmolski

@ysmolski ysmolski commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

After many changes I have accumulated some crufty code in costs. This PR tries to add a couple of abstractions and split big methods into smaller and more logical.

After many changes I have accumulated some crufty code in costs.
This PR tries to add a couple of abstractions and split big methods
into smaller and more logical.
@ysmolski
ysmolski requested a review from a team as a code owner June 5, 2026 10:00

@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 Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Refactor bundles cost computation inputs into an internal costInput and thread it through CostTreeNode.cost, costsAndMultiplier, childrenCost, and debug printing; adjust abstract-type aggregation and list-size multiplier behavior to branch between estimation and actual modes using costInput data.

Changes

Cost calculation refactor (costInput)

Layer / File(s) Summary
costInput struct and wiring
v2/pkg/engine/plan/cost.go
Adds costInput, newCostInput, returnedTypeNames; refactors CostTreeNode.cost/childrenCost to consume *costInput.
Abstract-type children cost aggregation
v2/pkg/engine/plan/cost.go
Estimation takes max per-implementing-type fragment cost; actual filters implementing fragments by runtime __typename from costInput.returnedTypeNames before summing.
costsAndMultiplier and default multiplier
v2/pkg/engine/plan/cost.go
Introduces costNodeResult.setDefaultMultiplier; refactors costsAndMultiplier to accept *costInput, initialize multiplier state, iterate data sources from costInput.configs, and read input-object argument values from costInput.vars.
Estimation-mode list-size & sized-field handling
v2/pkg/engine/plan/cost.go
Estimation list multipliers and sized-field propagation use costInput.vars and costInput.defaultListSize; non-list nodes early-return and unset multipliers default to defaultListSize; implementing-type maxima used for sized fields.
Actual-mode list multiplier & narrowing
v2/pkg/engine/plan/cost.go
Nearest enclosing list ancestor sizes read from costInput.typeStats; node multiplier computed as nodeStats.Size / enclosingSize with zero-guards; config lookups and non-list narrowing use costInput.configs and runtime distributions.
Public API entry points
v2/pkg/engine/plan/cost.go
CostCalculator.EstimateCost / ActualCost build costInput via newCostInput(...) and compute rounded cost via c.tree.cost(input).
Debug printing
v2/pkg/engine/plan/cost.go
DebugPrint builds costInput based on presence of typeStats; debugPrint computes subtree totals with node.cost(input), derives node intermediate values with node.costsAndMultiplier(input) + setDefaultMultiplier, reads vars via input.vars, and recurses passing input.

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 'refactor: improve costs code layering and readability' directly and clearly describes the main objective of the pull request, which is a structural refactoring of the costs code to improve its organization and maintainability.
Description check ✅ Passed The description explains the motivation for the refactoring (accumulated crufty code) and the approach taken (adding abstractions and splitting large methods), which is clearly related to the changeset of refactoring the costs code.
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/refactor_costs

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`:
- Line 995: The debug output currently only prints the first root child via
c.tree.children[0].debugPrint(...), missing other top-level fields; update the
code in cost.go to iterate over c.tree.children (e.g., for _, child := range
c.tree.children { child.debugPrint(&sb, input, 0) }) so every root child is
printed, preserving the same arguments and ordering; ensure you remove the
hardcoded index usage and handle the empty children case if needed.
🪄 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: e20ac453-c63c-489e-a922-fc32e7db6311

📥 Commits

Reviewing files that changed from the base of the PR and between de21280 and c043b41.

📒 Files selected for processing (1)
  • v2/pkg/engine/plan/cost.go

Comment thread v2/pkg/engine/plan/cost.go
@ysmolski
ysmolski merged commit c2cfa73 into master Jun 8, 2026
10 checks passed
@ysmolski
ysmolski deleted the yury/refactor_costs branch June 8, 2026 09:51
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