planner: handle double NOT in outer join #65942

hawkingrei · 2026-01-31T12:43:48Z

What problem does this PR solve?

Issue Number: close #65937

Problem Summary:

Double NOT in outer join ON conditions stayed as other cond and could lead to cartesian-like joins.

What changed and how does it work?

Normalize outer join OtherConditions by applying PushDownNot when UnaryNot is present.
Add predicate pushdown cases for double NOT left/right/semi joins and record plan + result.
Add integration test case in planner/core/issuetest to validate checksum parity.
Document optimizer/test experience in docs and update skills guidance.

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.

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

codecov · 2026-01-31T13:19:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.5746%. Comparing base (97c2efc) to head (7a76b40).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #65942        +/-   ##
================================================
- Coverage   77.7249%   77.5746%   -0.1504%     
================================================
  Files          2001       1923        -78     
  Lines        545885     533722     -12163     
================================================
- Hits         424289     414033     -10256     
+ Misses       119934     119683       -251     
+ Partials       1662          6      -1656

Flag	Coverage Δ
integration	`41.4756% <100.0000%> (-6.7111%)`	⬇️
unit	`76.7033% <100.0000%> (+0.3171%)`	⬆️

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

Components	Coverage Δ
dumpling	`56.7974% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`48.8756% <ø> (-12.1080%)`	⬇️

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

qw4990 · 2026-02-02T04:46:02Z

/test check-dev2

tiprow · 2026-02-02T04:46:19Z

@qw4990: 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 check-dev2

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.

Copilot

Pull request overview

This PR fixes a bug where double NOT in outer join ON conditions (LEFT JOIN t2 ON NOT NOT (t0.k0 = t2.k0)) were not normalized, causing them to remain as "other conditions" and potentially leading to cartesian-like joins with incorrect results.

Changes:

Added normalization step for outer join OtherConditions to apply PushDownNot when UnaryNot is present
Enhanced PushDownNot to recognize and simplify double-NOT patterns for logical expressions
Added comprehensive test cases for double-NOT in left/right/semi joins with both plan and result validation
Added documentation to guide future optimizer development

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/planner/core/operator/logicalop/logical_join.go	Added normalizeJoinConditionsForOuterJoin() to apply PushDownNot on outer join OtherConditions when UnaryNot is present
pkg/expression/util.go	Enhanced pushNotAcrossExpr() to detect and simplify double-NOT patterns (NOT NOT expr) for logical expressions
pkg/expression/util_test.go	Added test cases for double-NOT simplification including comparison operations and multiple NOT levels
pkg/planner/core/casetest/rule/rule_predicate_pushdown_test.go	Added runPredicatePushdownTestDataWithResult() to validate both plans and execution results
pkg/planner/core/casetest/rule/testdata/*.json	Added test cases for double-NOT in various join types and updated schema to include Result field
docs/note/planner/rule/optimizer_ai_notes.md	Added documentation about the fix with background, implementation choices, and testing guidance
AGENTS.md	Added reference to optimizer notes documentation
.github/skills/tidb-test-guidelines/*	Updated test guidance for predicate pushdown cases

pkg/planner/core/operator/logicalop/logical_join.go

pkg/expression/util.go

AilinKid

rest is good

qw4990

LGTM, but please address @AilinKid 's comment before merging.

ti-chi-bot · 2026-02-02T08:02:01Z

[LGTM Timeline notifier]

Timeline:

2026-02-02 06:54:18.948962868 +0000 UTC m=+77130.050361588: ☑️ agreed by qw4990.
2026-02-02 08:02:00.268270506 +0000 UTC m=+81191.369669226: ☑️ agreed by AilinKid.

hawkingrei · 2026-02-02T08:05:57Z

/retest

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

hawkingrei · 2026-02-02T15:05:31Z

/retest

fixdb · 2026-02-02T18:29:18Z

@windtalker please take a look

hawkingrei · 2026-02-03T03:10:17Z

/retest

windtalker · 2026-02-03T05:39:57Z

pkg/planner/core/operator/logicalop/logical_join.go

+		return
+	}
+	exprCtx := p.SCtx().GetExprCtx()
+	for i := range p.OtherConditions {


Looks like if only one of p.OtherConditions contains unary not, all the expression in p.OtherConditions will call expression.PushDownNot, it seems not necessary to me.

I have refactored it.

windtalker · 2026-02-03T05:40:26Z

/hold

windtalker

lgtm

ti-chi-bot · 2026-02-03T06:36:35Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [AilinKid,XuHuaiyu,qw4990,windtalker]
~~pkg/expression/OWNERS~~ [XuHuaiyu,windtalker]
~~pkg/planner/OWNERS~~ [AilinKid,qw4990]

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

windtalker · 2026-02-03T06:36:40Z

/hold cancel

hawkingrei · 2026-02-03T07:45:12Z

/retest

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 31, 2026

hawkingrei force-pushed the fix_double_not_outer_join branch from 539d0fa to 645443d Compare January 31, 2026 12:52

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 31, 2026

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Jan 31, 2026

hawkingrei added the AI-Testing label Feb 2, 2026

qw4990 added the type/bugfix This PR fixes a bug. label Feb 2, 2026

hawkingrei added the AI-Correction Bugfix by AI label Feb 2, 2026

hawkingrei requested a review from Copilot February 2, 2026 05:08

Copilot started reviewing on behalf of hawkingrei February 2, 2026 05:09 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

pkg/planner/core/operator/logicalop/logical_join.go Show resolved Hide resolved

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 2, 2026

AilinKid reviewed Feb 2, 2026

View reviewed changes

pkg/expression/util.go Outdated Show resolved Hide resolved

AilinKid reviewed Feb 2, 2026

View reviewed changes

qw4990 approved these changes Feb 2, 2026

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 2, 2026

AilinKid approved these changes Feb 2, 2026

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 2, 2026

hawkingrei requested a review from Copilot February 2, 2026 08:49

Copilot started reviewing on behalf of hawkingrei February 2, 2026 08:49 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

AilinKid approved these changes Feb 2, 2026

View reviewed changes

fixdb changed the title *: handle double NOT in outer join planner: handle double NOT in outer join Feb 2, 2026

XuHuaiyu approved these changes Feb 3, 2026

View reviewed changes

ti-chi-bot bot added the approved label Feb 3, 2026

hawkingrei added 6 commits February 3, 2026 12:07

planner: handle double NOT in outer join

d318736

expression: extend PushDownNot tests

0cb6d1b

tests: add integration case for double NOT join

6fa4ae1

docs: clarify planner rule notes

8535480

docs: refine notes guidance

c4a6076

expression: simplify PushDownNot comment

00c2562

windtalker reviewed Feb 3, 2026

View reviewed changes

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2026

planner: skip PushDownNot when no outer NOT

7a76b40

hawkingrei force-pushed the fix_double_not_outer_join branch from 5ed4824 to 7a76b40 Compare February 3, 2026 06:22

windtalker approved these changes Feb 3, 2026

View reviewed changes

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2026

ti-chi-bot bot merged commit 6c97ced into master Feb 3, 2026
30 checks passed

ti-chi-bot bot deleted the fix_double_not_outer_join branch February 3, 2026 13:27

planner: handle double NOT in outer join #65942

planner: handle double NOT in outer join #65942

Conversation

hawkingrei commented Jan 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

codecov bot commented Jan 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

qw4990 commented Feb 2, 2026

Uh oh!

tiprow bot commented Feb 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

AilinKid left a comment

Choose a reason for hiding this comment

Uh oh!

qw4990 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Feb 2, 2026

[LGTM Timeline notifier]

Uh oh!

hawkingrei commented Feb 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

hawkingrei commented Feb 2, 2026

Uh oh!

fixdb commented Feb 2, 2026

Uh oh!

hawkingrei commented Feb 3, 2026

Uh oh!

windtalker Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkingrei Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

windtalker commented Feb 3, 2026

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Feb 3, 2026

Uh oh!

windtalker commented Feb 3, 2026

Uh oh!

hawkingrei commented Feb 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hawkingrei commented Jan 31, 2026 •

edited

Loading

codecov bot commented Jan 31, 2026 •

edited

Loading

windtalker Feb 3, 2026 •

edited

Loading