Skip to content

fix: find proper parent when sizedFields parent is a non-list wrapper - #1493

Merged
ysmolski merged 3 commits into
masterfrom
yury/eng-9574-actual-cost-averaging-breaks-when-sizedfields-parent-is-a
May 14, 2026
Merged

fix: find proper parent when sizedFields parent is a non-list wrapper#1493
ysmolski merged 3 commits into
masterfrom
yury/eng-9574-actual-cost-averaging-breaks-when-sizedfields-parent-is-a

Conversation

@ysmolski

Copy link
Copy Markdown
Contributor

Instead of just trying the parent of the fields specifed in the sizedFields and failing on non-list wrapper, find nearest list-typed ancestor to avoid double counting of nested lists.

Closes #1492

Instead of just trying the parent of the fields specifed
in the sizedFields and failing on non-list wrapper, find nearest
list-typed ancestor to avoid double counting of nested lists.

@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 13, 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: 4020d73f-4dd5-4e4c-9874-f39f2c024570

📥 Commits

Reviewing files that changed from the base of the PR and between 31e7111 and 8d13dea.

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

📝 Walkthrough

Walkthrough

This PR fixes actual cost inflation when a non-list wrapper field is annotated with @listSize and contains sizedFields. The fix changes how the cost calculation finds the parent list size: instead of trimming the JSON path string, it walks the node parent chain to find the nearest list-returning ancestor. Supporting refactoring extracts the root coordinate to a constant, and a regression test validates the fix.

Changes

Cost Calculation Fix for Non-List Wrapper Parents

Layer / File(s) Summary
List parent lookup refactor in cost calculation
v2/pkg/engine/plan/cost.go
The actual cost multiplier calculation for list-returning fields now traverses the CostTreeNode parent chain to locate the nearest ancestor with returnsListType==true, then reads actualListSizes from that ancestor's jsonPath, instead of using string-based path prefix trimming.
Root coordinate constant extraction
v2/pkg/engine/plan/cost_visitor.go
Introduces a package-level costTreeRootNodeCoords variable to hold the sentinel FieldCoordinate and replaces its inline literal usage in NewCostVisitor.
Regression test for non-list wrapper sizedFields
execution/engine/execution_engine_cost_test.go
Adds a test case validating that a non-list wrapper field (items_page) with @listSize over a list parent (boards) computes both estimated and actual cost as 120, confirming the fix prevents cost inflation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: fixing parent detection when sizedFields parent is a non-list wrapper.
Description check ✅ Passed The description is related to the changeset, explaining the problem (double counting) and the solution (find nearest list-typed ancestor).
Linked Issues check ✅ Passed The changes directly address issue #1492 by modifying cost.go to walk up parent chain to find nearest list ancestor, implemented in cost_visitor.go and tested in cost_test.go.
Out of Scope Changes check ✅ Passed All changes are scoped to cost calculation logic and its tests; no unrelated modifications are present.

✏️ 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-9574-actual-cost-averaging-breaks-when-sizedfields-parent-is-a

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

@ysmolski
ysmolski merged commit 6b96976 into master May 14, 2026
10 checks passed
@ysmolski
ysmolski deleted the yury/eng-9574-actual-cost-averaging-breaks-when-sizedfields-parent-is-a branch May 14, 2026 13:12
ysmolski pushed a commit that referenced this pull request May 15, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.3.0](v2.2.0...v2.3.0)
(2026-05-15)


### Features

* support dot-path in slicingArguments
([#1485](#1485))
([2cb8d5e](2cb8d5e))


### Bug Fixes

* find proper parent when sizedFields parent is a non-list wrapper
([#1493](#1493))
([6b96976](6b96976)),
closes
[#1492](#1492)

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


##
[1.15.0](execution/v1.14.0...execution/v1.15.0)
(2026-05-15)


### Features

* support dot-path in slicingArguments
([#1485](#1485))
([2cb8d5e](2cb8d5e))


### Bug Fixes

* find proper parent when sizedFields parent is a non-list wrapper
([#1493](#1493))
([6b96976](6b96976)),
closes
[#1492](#1492)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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