planner: simplify NOT NOT in WHERE predicates for left-join right-side pruning#69939
planner: simplify NOT NOT in WHERE predicates for left-join right-side pruning#69939sijie-Z wants to merge 3 commits into
Conversation
…e pruning pushNotAcrossExpr had logic that was right for NOT NOT on comparison ops like EQ. The UnaryNot case recursively processes the child with the complement of . For NOT NOT (a=1): 1. outer UnaryNot: not=false -> recurse on wrapWithIsTrue(child) with not=true 2. inner UnaryNot: not=true -> recurse on inner child with not=false 3. EQ with not=false: unchanged (no not to apply) At step 3, the comparison is EQ, not=false, so it returns unchanged. At step 2, !changed && !not -> we return (false changed). Bug was here. Since was false at level 1 (!not, the changed got lost), the bug returned the full NOT NOT expression unchanged. Trivially, NOT NOT X is logically X (with is_true/null semantics preserved). The fix: when we strip the outer NOT (not=false case) and the recursion didn't modify the child, return childExpr (the simplified inner) anyway with changed=true. This shortcuts the double-negation in one step. Closes pingcap#69927 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ti-chi-bot
Bot
added
release-note
|
Hi @sijie-Z. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
ti-chi-bot
Bot
added
the
first-time-contributor
|
Welcome @sijie-Z! |
|
sijie-Z seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughThe change updates NOT pushdown to return a recursively simplified child expression, adds coverage for seven nested NOT operators, and documents the resulting double-negation planner fix. ChangesDouble-negation simplification
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/expression/notnot_test.go (1)
11-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the existing
TestPushDownNotinstead of adding duplicate coverage.
pkg/expression/util_test.goalready verifies thatNOT NOT (a=1)simplifies to(a=1). Moving this assertion into that existing test keeps the change smaller and avoids a new duplicate test scaffold.🤖 Prompt for 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. In `@pkg/expression/notnot_test.go` around lines 11 - 25, Remove the standalone TestPushDownNotDoubleNegation test and move its NOT NOT simplification setup and assertion into the existing TestPushDownNot in util_test.go. Preserve the assertion that PushDownNot returns an expression equal to the original equality expression, while avoiding duplicate test scaffolding.Source: Coding guidelines
pkg/expression/util.go (1)
1088-1089: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant conditional.
Line 1089 now returns the same value as the unconditional return on Line 1091, so
!changed && !notno longer controls behavior. Collapse this to a singlereturn childExpr, trueto avoid misleading future readers.Proposed simplification
- if !changed && !not { - return childExpr, true - } return childExpr, true🤖 Prompt for 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. In `@pkg/expression/util.go` around lines 1088 - 1089, In the surrounding expression simplification logic, remove the redundant conditional guarding the return and keep a single unconditional return of childExpr, true. Update the code near the visible changed and not checks without altering other behavior.
🤖 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 `@pkg/expression/notnot_test.go`:
- Line 1: Add the standard TiDB copyright and Apache 2.0 license header at the
top of the new notnot test file, before the package declaration, matching the
header format used by existing TiDB test files.
- Around line 3-9: Add the missing imports for the packages that provide
mock.NewContext and errors.RedactLogDisable in notnot_test.go, preserving the
existing test references so the new test compiles.
---
Nitpick comments:
In `@pkg/expression/notnot_test.go`:
- Around line 11-25: Remove the standalone TestPushDownNotDoubleNegation test
and move its NOT NOT simplification setup and assertion into the existing
TestPushDownNot in util_test.go. Preserve the assertion that PushDownNot returns
an expression equal to the original equality expression, while avoiding
duplicate test scaffolding.
In `@pkg/expression/util.go`:
- Around line 1088-1089: In the surrounding expression simplification logic,
remove the redundant conditional guarding the return and keep a single
unconditional return of childExpr, true. Update the code near the visible
changed and not checks without altering other behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67293e68-f537-49eb-aa44-62b3e55a77ed
📒 Files selected for processing (2)
pkg/expression/notnot_test.gopkg/expression/util.go
| @@ -0,0 +1,25 @@ | |||
| package expression | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the standard TiDB license header.
This is a new Go file without the required copyright and Apache 2.0 license header. Copy the standard header from an existing TiDB test file.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 1-1: : mkdir /home/jailuser/.cache/go/go-build1556602990/b8779/: no space left on device
(typecheck)
🤖 Prompt for 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.
In `@pkg/expression/notnot_test.go` at line 1, Add the standard TiDB copyright and
Apache 2.0 license header at the top of the new notnot test file, before the
package declaration, matching the header format used by existing TiDB test
files.
Source: Coding guidelines
ti-chi-bot
Bot
added
size/M
…implify conditional
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/expression/util_test.go`:
- Around line 331-341: Update the regression test in the NOT-chain section to
construct an equality expression for (a=1) before applying the seven UnaryNot
wrappers. Change the expected result to the corresponding inequality predicate
(a!=1), while preserving the existing PushDownNot assertion structure.
In `@pr_body.md`:
- Line 19: Update the checked unit-test entry in the PR checklist to reference
pkg/expression/util_test.go and the actual TestPushDownNot test name, replacing
the incorrect notnot_test.go and TestPushDownNotDoubleNegation references.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bca91f20-b708-4772-a6c2-a94749ae5440
📒 Files selected for processing (3)
pkg/expression/util.gopkg/expression/util_test.gopr_body.md
💤 Files with no reviewable changes (1)
- pkg/expression/util.go
|
|
||
| // Regression: NOT NOT NOT (a=1) should simplify to a!=1 | ||
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, col) | ||
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | ||
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | ||
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | ||
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | ||
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | ||
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) // 7 NOTs | ||
| ret = PushDownNot(ctx, notFunc) | ||
| require.True(t, ret.Equal(ctx, newFunctionWithMockCtx(ast.UnaryNot, newFunctionWithMockCtx(ast.IsTruthWithNull, col)))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the documented (a=1) regression, not just a column.
The comment says this covers seven NOTs around (a=1), but the constructed chain wraps col directly. As written, the test does not exercise the comparison predicate from the PR objective. Build eqFunc first, wrap that expression seven times, and assert the corresponding a != 1 result.
Proposed fix
- // Regression: NOT NOT NOT (a=1) should simplify to a!=1
- notFunc = newFunctionWithMockCtx(ast.UnaryNot, col)
+ // Regression: seven NOTs around (a=1) should simplify to a!=1
+ eqFunc = newFunctionWithMockCtx(ast.EQ, col, NewOne())
+ notFunc = newFunctionWithMockCtx(ast.UnaryNot, eqFunc)
...
- require.True(t, ret.Equal(ctx, newFunctionWithMockCtx(ast.UnaryNot, newFunctionWithMockCtx(ast.IsTruthWithNull, col))))
+ require.True(t, ret.Equal(ctx, newFunctionWithMockCtx(ast.NE, col, NewOne())))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Regression: NOT NOT NOT (a=1) should simplify to a!=1 | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, col) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) // 7 NOTs | |
| ret = PushDownNot(ctx, notFunc) | |
| require.True(t, ret.Equal(ctx, newFunctionWithMockCtx(ast.UnaryNot, newFunctionWithMockCtx(ast.IsTruthWithNull, col)))) | |
| // Regression: seven NOTs around (a=1) should simplify to a!=1 | |
| eqFunc = newFunctionWithMockCtx(ast.EQ, col, NewOne()) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, eqFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) | |
| notFunc = newFunctionWithMockCtx(ast.UnaryNot, notFunc) // 7 NOTs | |
| ret = PushDownNot(ctx, notFunc) | |
| require.True(t, ret.Equal(ctx, newFunctionWithMockCtx(ast.NE, col, NewOne()))) |
🤖 Prompt for 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.
In `@pkg/expression/util_test.go` around lines 331 - 341, Update the regression
test in the NOT-chain section to construct an equality expression for (a=1)
before applying the seven UnaryNot wrappers. Change the expected result to the
corresponding inequality predicate (a!=1), while preserving the existing
PushDownNot assertion structure.
|
|
||
| Tests | ||
|
|
||
| - [x] Unit test — added TestPushDownNotDoubleNegation in pkg/expression/notnot_test.go |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the unit-test file and test name.
The checklist references pkg/expression/notnot_test.go and TestPushDownNotDoubleNegation, but the supplied change is in pkg/expression/util_test.go within TestPushDownNot. Update this entry so the PR documentation accurately identifies the regression coverage.
🤖 Prompt for 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.
In `@pr_body.md` at line 19, Update the checked unit-test entry in the PR
checklist to reference pkg/expression/util_test.go and the actual
TestPushDownNot test name, replacing the incorrect notnot_test.go and
TestPushDownNotDoubleNegation references.
1 participant
What problem does this PR solve?
Issue Number: close #69927
Problem Summary:
NOT NOT a.c4 = 50 is logically equivalent to a.c4 = 50, but TiDB does not simplify the double negation before constraint derivation and left-join pruning. This causes a ~7.5x slowdown: the right table is scanned unnecessarily when the WHERE clause already contradicts the join condition.
What changed and how does it work?
In pushNotAcrossExpr (pkg/expression/util.go), when the UnaryNot case strips the outer NOT (not=false) and recurses into the inner child, the early return path now returns childExpr with changed=true instead of returning expr with changed=false. This allows the NOT NOT to be stripped in a single step rather than requiring the caller context to provide the
notflag.This is safe because NOT NOT X is logically X (with is_true_with_null semantics already handled by wrapWithIsTrue at the inner UnaryNot level).
Check List
Tests
Side effects
Documentation
Release note
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
NOTpredicates, including double negation.Tests
NOTexpressions.