Skip to content

planner: prove null-reject for deferred expressions#67789

Merged
winoros merged 1 commit into
pingcap:masterfrom
winoros:null-reject-opt
Apr 17, 2026
Merged

planner: prove null-reject for deferred expressions#67789
winoros merged 1 commit into
pingcap:masterfrom
winoros:null-reject-opt

Conversation

@winoros
Copy link
Copy Markdown
Member

@winoros winoros commented Apr 15, 2026

What problem does this PR solve?

Issue Number: close #67788, close #65583

Problem Summary:

Null-reject proof skipped Constant expressions with DeferredExpr to avoid treating plan-cache-sensitive runtime values as static constants. That was conservative, but it also missed safe symbolic proof opportunities and left a now-obsolete plan-cache compatibility parameter on planner/util.IsNullRejected.

What changed and how does it work?

This PR lets null-reject proof inspect DeferredExpr symbolically without evaluating or nullify-folding its runtime value.

  • Add an internal allowNullifiedFold mode to proveNullRejected. Normal proof still uses the nullify-then-fold bridge. When descending into Constant.DeferredExpr, the proof keeps using structural null-reject rules but disables nullified folding.
  • Keep ParamMarker and DeferredExpr out of static constant folding candidates.
  • Remove the obsolete skipPlanCacheCheck argument from IsNullRejected and update its callers.
  • Add regression cases for symbolic deferred proof, placeholder NULL classification, and ensuring deferred expressions skip nullified folding.

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.

Unit tests:

go test -run TestIsNullRejectedProofModes -tags=intest,deadlock
./tools/check/failpoint-go-test.sh pkg/planner/core/operator/logicalop -run '^$'
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

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

None

Summary by CodeRabbit

  • Refactor

    • Unified null-rejection logic in the query planner, affecting when joins can be simplified to inner joins and when NOT NULL predicates are derived or propagated.
  • Tests

    • Expanded test coverage for deferred/runtime-dependent expressions and null-rejection proofing to validate the updated behavior.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Apr 15, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 15, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: cc2e6ac5-e95a-497d-8b8b-0e420f219d8b

📥 Commits

Reviewing files that changed from the base of the PR and between e301757 and d592612.

📒 Files selected for processing (6)
  • pkg/planner/core/operator/logicalop/logical_aggregation.go
  • pkg/planner/core/operator/logicalop/logical_join.go
  • pkg/planner/core/operator/logicalop/logical_projection.go
  • pkg/planner/util/funcdep_misc.go
  • pkg/planner/util/null_misc.go
  • pkg/planner/util/null_misc_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • pkg/planner/util/funcdep_misc.go
  • pkg/planner/core/operator/logicalop/logical_projection.go
  • pkg/planner/core/operator/logicalop/logical_aggregation.go

📝 Walkthrough

Walkthrough

Removes the skipPlanCacheCheck parameter from IsNullRejected, adds an allowNullifiedFold proof flag, threads it through recursive null-rejection proof logic (suppressing nullify-then-fold for plan-cache-sensitive/deferred constants), and updates planner call sites and tests to the simplified IsNullRejected(...) signature.

Changes

Cohort / File(s) Summary
Core null-rejection logic
pkg/planner/util/null_misc.go
Remove skipPlanCacheCheck from IsNullRejected signature; add allowNullifiedFold flag; thread flag through proveNullRejected and helpers; prevent nullify-then-fold for deferred/plan-cache-sensitive constants.
Operator call sites (FD & join logic)
pkg/planner/core/operator/logicalop/logical_aggregation.go, pkg/planner/core/operator/logicalop/logical_projection.go, pkg/planner/core/operator/logicalop/logical_join.go
Update all calls to util.IsNullRejected(...) to the new signature (drop trailing boolean), affecting NOT NULL derivation used in FD extraction and outer→inner join simplification and derived predicates.
Function dependency helper
pkg/planner/util/funcdep_misc.go
Call site updated to call IsNullRejected without the extra boolean in ExtractNotNullFromConds.
Tests
pkg/planner/util/null_misc_test.go
Add newNullRejectDeferredConst test helper; add deferred-constant test cases; update test calls to new IsNullRejected signature.

Sequence Diagram(s)

sequenceDiagram
    participant Op as Planner Operator (Proj/Agg/Join)
    participant IsNull as util.IsNullRejected
    participant Proof as proveNullRejected
    participant Fold as tryFoldNullifiedConstant

    Op->>IsNull: IsNullRejected(ctx, schema, expr)
    IsNull->>Proof: start proof (allowNullifiedFold = true)
    alt encounter Constant with DeferredExpr
        Proof->>Proof: descend into DeferredExpr (allowNullifiedFold = false)
        Proof->>Fold: do not attempt nullify-then-fold (blocked)
    else other expressions
        Proof->>Fold: may attempt nullify-then-fold
    end
    Proof-->>IsNull: result (null-rejected true/false)
    IsNull-->>Op: boolean result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

ok-to-test

Suggested reviewers

  • guo-shaoge
  • Reminiscent
  • qw4990
  • AilinKid

Poem

🐰 I nibble through proofs with careful hop,
Deferred crumbs I sniff but never chop,
I peek inside their wrapped-up core,
Say “null’s rejected” — then I hop some more,
A tiny trace, the planner leaps a crop.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.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
Title check ✅ Passed The PR title 'planner: prove null-reject for deferred expressions' clearly and concisely describes the main change: enabling null-reject proof for deferred expressions.
Description check ✅ Passed The PR description includes issue reference (#67788), problem summary, detailed explanation of changes, test coverage, and release notes section, matching the template structure.
Linked Issues check ✅ Passed The PR fully addresses issue #67788 objectives: allows symbolic null-reject proof for DeferredExpr, prevents runtime value folding, removes obsolete skipPlanCacheCheck parameter, and adds unit tests.
Out of Scope Changes check ✅ Passed All code changes are scoped to the null-reject proof enhancement: updating null_misc.go signature, propagating callers in aggregation/join/projection/funcdep files, and adding regression tests.

✏️ 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.

@ti-chi-bot ti-chi-bot Bot added the sig/planner SIG: Planner label Apr 15, 2026
@tiprow
Copy link
Copy Markdown

tiprow Bot commented Apr 15, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 15, 2026
@winoros winoros marked this pull request as ready for review April 15, 2026 14:04
@ti-chi-bot ti-chi-bot Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 15, 2026
@pantheon-ai
Copy link
Copy Markdown

pantheon-ai Bot commented Apr 15, 2026

Review Complete

Findings: 0 issues
Posted: 0
Duplicates/Skipped: 0

ℹ️ Learn more details on Pantheon AI.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.5719%. Comparing base (757952a) to head (d592612).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #67789        +/-   ##
================================================
- Coverage   77.6156%   77.5719%   -0.0437%     
================================================
  Files          1982       1966        -16     
  Lines        548909     551248      +2339     
================================================
+ Hits         426039     427614      +1575     
- Misses       122060     123632      +1572     
+ Partials        810          2       -808     
Flag Coverage Δ
integration 41.0339% <90.0000%> (+6.6941%) ⬆️
unit 76.7927% <100.0000%> (+0.4518%) ⬆️

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

Components Coverage Δ
dumpling 61.5065% <ø> (ø)
parser ∅ <ø> (∅)
br 50.4780% <ø> (-9.9508%) ⬇️
🚀 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.

@winoros
Copy link
Copy Markdown
Member Author

winoros commented Apr 15, 2026

/retest

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 16, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 17, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qw4990, Reminiscent

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 approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 17, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 17, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-04-16 10:40:47.154695073 +0000 UTC m=+1644052.360055130: ☑️ agreed by Reminiscent.
  • 2026-04-17 11:28:35.869594363 +0000 UTC m=+1733321.074954420: ☑️ agreed by qw4990.

@winoros
Copy link
Copy Markdown
Member Author

winoros commented Apr 17, 2026

/retest

1 similar comment
@winoros
Copy link
Copy Markdown
Member Author

winoros commented Apr 17, 2026

/retest

@winoros winoros merged commit eea8b1e into pingcap:master Apr 17, 2026
35 checks passed
@winoros winoros deleted the null-reject-opt branch April 17, 2026 14:05
Copy link
Copy Markdown

@pantheon-ai pantheon-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Code looks good. No issues found.

@winoros
Copy link
Copy Markdown
Member Author

winoros commented Jun 1, 2026

/cherry-pick release-8.5

@ti-chi-bot
Copy link
Copy Markdown
Member

@winoros: new pull request created to branch release-8.5: #68850.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherry-pick release-8.5

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.

winoros added a commit to ti-chi-bot/tidb that referenced this pull request Jun 1, 2026
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

4 participants