planner: replace outer-join null-reject evaluation with structural proof | tidb-test=pr/2724 - #67129
Merged
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note-none
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
sig/planner
📝 WalkthroughWalkthroughAdds a proof-based null-rejection framework for outer-join simplification, a builtin function registry accessor, migrates join logic to use the new proof API, refines partition-constraint checks, and expands unit and integration tests exercising null-handling, IN/IS/COALESCE semantics, and plan-cache interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Planner
participant LogicalJoin
participant NullProof as util.IsNullRejected
participant Builtins as BuiltinRegistry
Client->>Planner: build/optimize query with outer join
Planner->>LogicalJoin: evaluate join predicate / push-down
LogicalJoin->>NullProof: IsNullRejected(ctx, innerSchema, predicate, ...)
NullProof->>Builtins: query function classifications (preserve/reject tests)
Builtins-->>NullProof: classification results
NullProof-->>LogicalJoin: proof result (nonTrue/mustNull)
LogicalJoin-->>Planner: decide outer->inner conversion or keep left join
Planner-->>Client: return plan / execute
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/expression/builtin_registry.go (1)
19-20: Make the doc comment describe the returned value.This API returns a sorted snapshot of builtin function names, not the registry itself.
As per coding guidelines, keep exported-symbol doc comments, and prefer semantic constraints over name restatement.✏️ Suggested wording
-// RegisteredBuiltinFunctionNames returns the sorted builtin function registry. +// RegisteredBuiltinFunctionNames returns a sorted snapshot of registered builtin function names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/expression/builtin_registry.go` around lines 19 - 20, The doc comment for RegisteredBuiltinFunctionNames should describe the returned value rather than restate the function name; update the comment above the RegisteredBuiltinFunctionNames function to say it returns a sorted snapshot (slice) of builtin function names (e.g., "RegisteredBuiltinFunctionNames returns a sorted snapshot of builtin function names.") so the comment documents the semantic constraint of the return value.tests/integrationtest/t/planner/core/plan_cache.test (1)
1560-1589: Resettidb_enable_prepared_plan_cacheafter this case.Line 1560 forces the session switch on, but nothing restores it. That makes later plan-cache cases depend on this block's side effect if the default or surrounding setup changes.
🧪 Minimal isolation fix
set `@v`=2; execute stmt67048b using `@v`; select @@last_plan_from_cache; +set tidb_enable_prepared_plan_cache=DEFAULT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/t/planner/core/plan_cache.test` around lines 1560 - 1589, This test turns on the session variable tidb_enable_prepared_plan_cache but never restores it; wrap the case so you save the original tidb_enable_prepared_plan_cache value, set it to 1 for the prepared-plan checks (the block that prepares stmt67048a and stmt67048b and executes them), then restore the saved value at the end (use a defer/teardown or explicit reset) so subsequent tests are not affected.docs/design/2026-03-16-null-reject-proof-framework.md (1)
3-5: Fill in the TBD placeholders before merging.The author, discussion PR, and tracking issue fields are still TBD. These should be populated to maintain proper documentation provenance.
📝 Proposed fix
# Design: Structural Null-Reject Proof under Three-Valued Logic -- Author(s): TBD -- Discussion PR: TBD -- Tracking Issue: TBD +- Author(s): winoros +- Discussion PR: https://github.com/pingcap/tidb/pull/67129 +- Tracking Issue: `#60737`, `#60080`, `#61327`, `#59162`, `#60081`, `#66825`, `#58793`, `#67048`, `#60370`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/2026-03-16-null-reject-proof-framework.md` around lines 3 - 5, Populate the TBD placeholders in the document header by replacing "Author(s): TBD", "Discussion PR: TBD", and "Tracking Issue: TBD" with the actual author name(s), the PR number or link for the discussion, and the tracking issue number or link respectively; update the fields Author(s), Discussion PR, and Tracking Issue at the top of docs/design/2026-03-16-null-reject-proof-framework.md so the metadata is complete before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/design/2026-03-16-null-reject-proof-framework.md`:
- Around line 3-5: Populate the TBD placeholders in the document header by
replacing "Author(s): TBD", "Discussion PR: TBD", and "Tracking Issue: TBD" with
the actual author name(s), the PR number or link for the discussion, and the
tracking issue number or link respectively; update the fields Author(s),
Discussion PR, and Tracking Issue at the top of
docs/design/2026-03-16-null-reject-proof-framework.md so the metadata is
complete before merging.
In `@pkg/expression/builtin_registry.go`:
- Around line 19-20: The doc comment for RegisteredBuiltinFunctionNames should
describe the returned value rather than restate the function name; update the
comment above the RegisteredBuiltinFunctionNames function to say it returns a
sorted snapshot (slice) of builtin function names (e.g.,
"RegisteredBuiltinFunctionNames returns a sorted snapshot of builtin function
names.") so the comment documents the semantic constraint of the return value.
In `@tests/integrationtest/t/planner/core/plan_cache.test`:
- Around line 1560-1589: This test turns on the session variable
tidb_enable_prepared_plan_cache but never restores it; wrap the case so you save
the original tidb_enable_prepared_plan_cache value, set it to 1 for the
prepared-plan checks (the block that prepares stmt67048a and stmt67048b and
executes them), then restore the saved value at the end (use a defer/teardown or
explicit reset) so subsequent tests are not affected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b19fcf1-6da1-4369-b276-c89929edc36d
📒 Files selected for processing (16)
docs/design/2026-03-16-null-reject-proof-framework.mdpkg/expression/BUILD.bazelpkg/expression/builtin_registry.gopkg/planner/core/casetest/rule/rule_outer2inner_test.gopkg/planner/core/casetest/rule/testdata/outer2inner_out.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_xut.jsonpkg/planner/core/operator/logicalop/BUILD.bazelpkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/partidx/check_constraint.gopkg/planner/util/BUILD.bazelpkg/planner/util/null_misc.gopkg/planner/util/null_misc_test.gotests/integrationtest/r/planner/core/plan_cache.resulttests/integrationtest/r/planner/core/rule_outer2inner.resulttests/integrationtest/t/planner/core/plan_cache.testtests/integrationtest/t/planner/core/rule_outer2inner.test
💤 Files with no reviewable changes (1)
- pkg/planner/core/operator/logicalop/BUILD.bazel
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67129 +/- ##
================================================
- Coverage 77.5871% 77.4678% -0.1194%
================================================
Files 1981 1966 -15
Lines 547950 551914 +3964
================================================
+ Hits 425139 427556 +2417
- Misses 122001 124344 +2343
+ Partials 810 14 -796
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
guo-shaoge
self-requested a review
March 19, 2026 09:53
# Conflicts: # pkg/planner/core/casetest/rule/testdata/outer2inner_out.json # pkg/planner/core/casetest/rule/testdata/outer2inner_xut.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/util/null_misc_test.go (1)
168-182: Consider also validatingnullRejectNullPreservingFunctionskeys.The test validates that
nullRejectRejectNullTestskeys are present in the builtin registry, but does not perform the same check fornullRejectNullPreservingFunctions. If a null-preserving function name is misspelled or removed from the registry, it would silently fail to match during proof evaluation.♻️ Suggested enhancement
for name := range nullRejectRejectNullTests { require.Contains(t, names, name) } + + for name := range nullRejectNullPreservingFunctions { + require.Contains(t, names, name) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/util/null_misc_test.go` around lines 168 - 182, TestNullRejectBuiltinRegistrySnapshot currently verifies that keys from nullRejectRejectNullTests exist in the registered builtin names but misses validating nullRejectNullPreservingFunctions; update the test (TestNullRejectBuiltinRegistrySnapshot) to also iterate over the keys of nullRejectNullPreservingFunctions and assert each name is present in expression.RegisteredBuiltinFunctionNames() (use require.Contains like the existing loop) so misspelled or removed null-preserving functions are caught.
🤖 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/util/null_misc_test.go`:
- Around line 168-182: TestNullRejectBuiltinRegistrySnapshot currently verifies
that keys from nullRejectRejectNullTests exist in the registered builtin names
but misses validating nullRejectNullPreservingFunctions; update the test
(TestNullRejectBuiltinRegistrySnapshot) to also iterate over the keys of
nullRejectNullPreservingFunctions and assert each name is present in
expression.RegisteredBuiltinFunctionNames() (use require.Contains like the
existing loop) so misspelled or removed null-preserving functions are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 531226e1-3368-442e-9b8c-4c4946bb489b
📒 Files selected for processing (6)
docs/design/2026-03-16-null-reject-proof-framework.mdpkg/expression/builtin_registry.gopkg/planner/util/null_misc.gopkg/planner/util/null_misc_test.gotests/integrationtest/r/planner/core/plan_cache.resulttests/integrationtest/t/planner/core/plan_cache.test
✅ Files skipped from review due to trivial changes (2)
- pkg/expression/builtin_registry.go
- tests/integrationtest/t/planner/core/plan_cache.test
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/planner/util/null_misc.go
Reminiscent
reviewed
Mar 24, 2026
|
|
||
| // nullRejectNullPreservingFunctions lists builtins that return NULL when any | ||
| // argument is NULL. See the file-level comment for how this drives the proof. | ||
| var nullRejectNullPreservingFunctions = map[string]struct{}{ |
Contributor
There was a problem hiding this comment.
How do we determine whether a function should belong to this list?
For example, in PostgreSQL each function has a dedicated property (proisstrict) to indicate whether it is strict (i.e., returns NULL when any input is NULL).
I’m asking this because when I had Codex review this PR, it pointed out that ast.ConvertTz, ast.RegexpLike, ast.RegexpInStr, ast.RegexpSubstr, and ast.RegexpReplace do not seem to belong in nullRejectNullPreservingFunctions.
Member
Author
There was a problem hiding this comment.
I think the best way to do it is to have agents conduct a multi-round check.
I'll check these functions later.
Reminiscent
reviewed
Mar 24, 2026
| // `innerSchema` is replaced with SQL NULL. | ||
| func IsNullRejected(ctx base.PlanContext, innerSchema *expression.Schema, predicate expression.Expression, | ||
| skipPlanCacheCheck bool) bool { | ||
| _ = skipPlanCacheCheck // kept for API compatibility; the new proof does not use EvaluateExprWithNull |
Contributor
There was a problem hiding this comment.
Should we update the API directly here?
Do we need to maintain backward compatibility, or should that be handled separately later?
Reminiscent
approved these changes
Mar 24, 2026
qw4990
reviewed
Mar 26, 2026
qw4990
reviewed
Mar 26, 2026
qw4990
reviewed
Mar 26, 2026
qw4990
reviewed
Mar 26, 2026
This was referenced Apr 16, 2026
This was referenced Apr 21, 2026
premal
pushed a commit
to premal/tidb
that referenced
this pull request
Apr 30, 2026
…oof | tidb-test=pr/2724 (pingcap#67129)
13 tasks
This was referenced May 14, 2026
Closed
Member
Author
|
/cherry-pick release-8.5 |
Member
|
@winoros: new pull request created to branch DetailsIn response to this:
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 ti-community-infra/tichi repository. |
13 tasks
This was referenced Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #60080, close #61327, close #59162, close #60081, close #66825, close #58793, close #67048, close #60370, close #66824, close #67330, close #67373
Problem Summary:
What changed and how does it work?
Rewrite the current null-reject judging function with a well-designed algorithm.
You can view the doc for details.
The algorithm provides a reliable way to perform the check and address issues from the old buggy version.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation
Tests