planner/core: fix wrong results caused by rule_project_eliminate for queries with Apply#67231
Conversation
…queries with Apply Signed-off-by: guo-shaoge <shaoge1994@163.com>
|
Review failed due to infrastructure/execution failure (Codex API service overloaded - 503 errors). Please re-trigger review when service is available. ℹ️ Learn more details on Pantheon AI. |
|
/ok-to-test |
📝 WalkthroughWalkthroughThe PR adjusts test sharding configuration, introduces a new test verifying CTE WITH clause behavior with Apply operations, and modifies the projection elimination logic to unconditionally apply column replacements without schema validation filtering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/rule_eliminate_projection.go (1)
186-186: Document why replacement is unfiltered here.This call is critical and non-obvious (especially with
Apply/correlated references). Please add a brief intent comment to prevent future regressions from “cleanup” refactors.✍️ Suggested inline comment
- p.ReplaceExprColumns(replace) + // Intentionally apply the full replacement map (no child-schema filtering). + // Apply/correlated plans may need replacements that are not visible in a single child schema. + p.ReplaceExprColumns(replace)As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs, and SHOULD NOT restate what the code already makes clear."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/rule_eliminate_projection.go` at line 186, Add a brief intent comment immediately above the p.ReplaceExprColumns(replace) call explaining why the replacement is intentionally unfiltered: note that replacements must be applied globally here (including into Apply/correlated references) to preserve correct binding of correlated columns and avoid breaking Apply-based plans or leaving stale references; mention the invariant that filtering these replacements would prevent proper propagation of column substitutions for correlated expressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/core/rule_eliminate_projection.go`:
- Line 186: Add a brief intent comment immediately above the
p.ReplaceExprColumns(replace) call explaining why the replacement is
intentionally unfiltered: note that replacements must be applied globally here
(including into Apply/correlated references) to preserve correct binding of
correlated columns and avoid breaking Apply-based plans or leaving stale
references; mention the invariant that filtering these replacements would
prevent proper propagation of column substitutions for correlated expressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bdc7c789-f4bc-43fe-a96e-caccbb2b2b0e
📒 Files selected for processing (3)
pkg/planner/core/casetest/rule/BUILD.bazelpkg/planner/core/casetest/rule/rule_eliminate_projection_test.gopkg/planner/core/rule_eliminate_projection.go
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67231 +/- ##
================================================
- Coverage 77.6966% 77.6780% -0.0187%
================================================
Files 2013 1943 -70
Lines 551311 556218 +4907
================================================
+ Hits 428350 432059 +3709
- Misses 121229 124094 +2865
+ Partials 1732 65 -1667
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hawkingrei, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #67138
Problem Summary:
There is a replaceMap in
rule_eliminate_projection: <col-9, col-6>, which means we should replace allcol-9tocol-6.But since
datasource-2only outputscol-14andcol-15, this piece of code prevents the replacement from happening.Because it assumes that all target columns to be replaced must come from the child output schema. This is true in normal cases, but correlated columns do not actually come from the child output schema—they are instead set explicitly during the execution of Apply.
As a result, col-9 in selection-2 is not replaced with col-6, and the correlated column is not updated correctly.
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.