Skip to content

expression, planner: guard mixed outer-join filter propagation | tidb-test=pr/2729 - #67804

Merged
ti-chi-bot[bot] merged 3 commits into
pingcap:masterfrom
hawkingrei:issue-66833-outer-join-const-prop
Apr 19, 2026
Merged

expression, planner: guard mixed outer-join filter propagation | tidb-test=pr/2729#67804
ti-chi-bot[bot] merged 3 commits into
pingcap:masterfrom
hawkingrei:issue-66833-outer-join-const-prop

Conversation

@hawkingrei

@hawkingrei hawkingrei commented Apr 16, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #66833

Problem Summary:

PropConstForOuterJoin could derive a new join-side filter from a WHERE predicate that mixed preserved-side and inner-side columns. For LEFT JOIN ... WHERE COALESCE(...), that derived filter could remove the matched inner row before null extension and produce a wrong result.

What changed and how does it work?

  • Restrict outer-join filterConds -> joinConds derivation to predicates that are fully on the preserved side.
  • Keep the existing join-condition derivation path unchanged.
  • Add a regression to TestOuter2Inner for the exact LEFT JOIN + WHERE COALESCE(...) wrong-result shape from #66833.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Test details:

  • Fail before fix:
    • ./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/rule -run TestOuter2Inner -count=1
  • Pass after fix:
    • ./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/rule -run TestOuter2Inner -count=1
  • Completion gate:
    • make lint

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

Summary by CodeRabbit

  • Bug Fixes

    • Prevented unsafe propagation of WHERE-derived predicates into join filters to preserve LEFT/outer join semantics and correct results.

  • Behavior

    • Simplified pushed predicates on some join build-sides and coprocessor selections, yielding more accurate and stable plan trees.

  • Tests

    • Added a regression test ensuring LEFT JOINs are not incorrectly transformed by certain WHERE patterns.

@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed sig/planner SIG: Planner size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 16, 2026
@coderabbitai

coderabbitai Bot commented Apr 16, 2026 •
edited
Loading

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Tighten constant-propagation for outer joins so WHERE-derived predicates contribute to new join filters only if they are fully from the preserved (outer) schema. Add a regression test for the COALESCE+LEFT JOIN case and update several expected plan_tree outputs to reflect removed or simplified pushed predicates.

Changes

Cohort / File(s) Summary
Constant propagation logic
pkg/expression/constant_propagation.go		 Guard deriveConds to skip conditions not entirely from the preserved outer schema when filterConds is true; adjust propagateColumnEQ to derive WHERE-derived join filters only after safe filtering.
Regression test
pkg/planner/core/casetest/rule/rule_outer2inner_test.go		 Add a TestOuter2Inner case reproducing Issue #66833 (COALESCE predicate with LEFT JOIN), asserting plan remains left outer join and the query returns an empty set.
Integration test outputs
tests/integrationtest/r/executor/merge_join.result, tests/integrationtest/r/planner/core/integration.result, tests/integrationtest/r/planner/core/rule_outer2inner.result		 Update expected EXPLAIN/plan_tree outputs: remove or simplify pushed predicates (drop some pushed Selection predicates or the Selection layer on build-side TableReader) to match guarded predicate propagation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

ok-to-test

Suggested reviewers

  • qw4990
  • guo-shaoge

Poem

🐰 I hopped through predicates, soft and spry,
I kept outer rows where NULLs might lie.
COALESCE now whispers, joins behave true,
A carrot for tests, a tidy plan view. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description follows the template with Problem Summary, What Changed sections, and properly formatted Check List with passing test details and completion gate (make lint).
Linked Issues check ✅ Passed The code changes directly address issue #66833 by restricting outer-join filterConds derivation to preserved-side predicates only, and adding the specific regression test for LEFT JOIN + WHERE COALESCE(...) case.
Out of Scope Changes check ✅ Passed All changes are tightly scoped to fixing the outer-join constant propagation bug: implementation fix, unit test, and necessary test result updates are all directly related to the issue.
Title check ✅ Passed The title clearly and specifically identifies the main change: guarding mixed outer-join filter propagation in the expression and planner modules to fix a bug with outer-join semantics.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

coderabbitai[bot]
coderabbitai Bot reviewed Apr 16, 2026
View reviewed changes

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
pkg/expression/constant_propagation.go (1)

867-869: Mark rejected mixed-side filters as visited.

This guard is independent of the current outerCol = innerCol pair, so leaving the predicate unvisited makes deriveConds re-check the same skipped WHERE item for every equivalent-column pair.

Suggested change 
 		if filterConds && !ExprFromSchema(cond, s.outerSchema) {
+			visited[k+offset] = true
 			continue
 		}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/expression/constant_propagation.go` around lines 867 - 869, The loop
skipping mixed-side filters currently does "if filterConds &&
!ExprFromSchema(cond, s.outerSchema) { continue }" but doesn't mark the
predicate as visited, so deriveConds will revisit it for each equivalent-column
pair; fix by marking the condition as visited before continuing (use the same
visit-tracking mechanism deriveConds uses—e.g., call the visitor helper such as
s.markVisited(cond) or add cond to s.visited/set) so the predicate is skipped in
subsequent iterations.
🤖 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/expression/constant_propagation.go`:
- Around line 867-869: The loop skipping mixed-side filters currently does "if
filterConds && !ExprFromSchema(cond, s.outerSchema) { continue }" but doesn't
mark the predicate as visited, so deriveConds will revisit it for each
equivalent-column pair; fix by marking the condition as visited before
continuing (use the same visit-tracking mechanism deriveConds uses—e.g., call
the visitor helper such as s.markVisited(cond) or add cond to s.visited/set) so
the predicate is skipped in subsequent iterations.
ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4f263ff4-a3a4-474f-8d4d-b47608ad6765

📥 Commits

Reviewing files that changed from the base of the PR and between 7762bc6 and cb20175.

📒 Files selected for processing (2)
  • pkg/expression/constant_propagation.go
  • pkg/planner/core/casetest/rule/rule_outer2inner_test.go

@codecov

codecov Bot commented Apr 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.6063%. Comparing base (7762bc6) to head (3edb52a).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files 
@@               Coverage Diff                @@
##             master     #67804        +/-   ##
================================================
+ Coverage   77.6037%   77.6063%   +0.0025%     
================================================
  Files          1982       1966        -16     
  Lines        548713     551334      +2621     
================================================
+ Hits         425822     427870      +2048     
- Misses       122081     123444      +1363     
+ Partials        810         20       -790
Flag Coverage Δ
integration 41.0808% <100.0000%> (+6.7411%) ⬆️
unit 76.8378% <100.0000%> (+0.4876%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 61.5065% <ø> (ø)
parser ∅ <ø> (∅)
br 50.5038% <ø> (-9.9226%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hawkingrei

hawkingrei commented Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@ti-chi-bot ti-chi-bot Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 16, 2026
@hawkingrei hawkingrei added the AI-Correction Bugfix by AI label Apr 16, 2026
@hawkingrei hawkingrei changed the title expression, planner: guard mixed outer-join filter propagation expression, planner: guard mixed outer-join filter propagation | tidb-test=pr/2729 Apr 16, 2026
@hawkingrei

hawkingrei commented Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 17, 2026
@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 17, 2026
@ti-chi-bot

ti-chi-bot Bot commented Apr 17, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-04-17 03:29:16.119464338 +0000 UTC m=+1704561.324824385: ☑️ agreed by terry1purcell.
  • 2026-04-17 03:52:16.74017374 +0000 UTC m=+1705941.945533787: ☑️ agreed by AilinKid.

windtalker
windtalker approved these changes Apr 17, 2026
View reviewed changes

@windtalker windtalker 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.

lgtm

@ti-chi-bot

ti-chi-bot Bot commented Apr 17, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, terry1purcell, windtalker

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the approved label Apr 17, 2026
@hawkingrei

hawkingrei commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

/retest

2 similar comments
@hawkingrei

hawkingrei commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/test unit-test

@tiprow

tiprow Bot commented Apr 18, 2026

Copy link
Copy Markdown

@hawkingrei: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow

/test tidb_parser_test

Use /test all to run all jobs.

Details

In response to this:

/test unit-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/retest

9 similar comments
@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

hawkingrei commented Apr 19, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@ti-chi-bot
ti-chi-bot Bot merged commit a10a43a into pingcap:master Apr 19, 2026
35 checks passed
premal pushed a commit to premal/tidb that referenced this pull request Apr 30, 2026
@hawkingrei
expression, planner: guard mixed outer-join filter propagation (pingc…
6382a4f 
…ap#67804)

close pingcap#66833
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@windtalker windtalker windtalker approved these changes
@AilinKid AilinKid AilinKid approved these changes
@terry1purcell terry1purcell terry1purcell approved these changes

Assignees

No one assigned

Labels

AI-Correction Bugfix by AI approved lgtm release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect result with LEFT/RIGHT JOIN and COALESCE predicate

4 participants

@hawkingrei @windtalker @AilinKid @terry1purcell