Skip to content

planner: fix null-reject proof for WEEK and YEARWEEK#68817

Merged
ti-chi-bot[bot] merged 3 commits into
masterfrom
fix-null-misc-builtin-classification
May 31, 2026
Merged

planner: fix null-reject proof for WEEK and YEARWEEK#68817
ti-chi-bot[bot] merged 3 commits into
masterfrom
fix-null-misc-builtin-classification

Conversation

@winoros
Copy link
Copy Markdown
Member

@winoros winoros commented May 31, 2026

What problem does this PR solve?

Issue Number: close #68816

Problem Summary:

WEEK(date, mode) and YEARWEEK(date, mode) were classified as whole-function NULL-preserving builtins for null-reject proof. This is unsafe because TiDB/MySQL treats a NULL mode argument as mode 0, so predicates such as WEEK('2024-01-08', nullable_mode) >= 0 are not null-rejecting on nullable_mode.

What changed and how does it work?

  • Remove WEEK and YEARWEEK from the whole-function NULL-preserving builtin table.
  • Add a narrow proof rule that keeps the valid behavior for their date argument: if the date argument is proven NULL, the function result is NULL.
  • Remove unreachable NULL-preserving table entries for builtins that are not registered as scalar functions.
  • Extend the registry snapshot test to reject future NULL-preserving table entries that are not registered builtins, except for the internal scalar CAST.

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 test:

go test -count=1 -tags=intest,deadlock ./pkg/planner/util -run 'Test(IsNullRejectedProofModes|NullRejectBuiltinRegistrySnapshot)'

Other checks:

make lint
git diff --check origin/master..HEAD

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.

Fix a potential wrong result caused by unsafe null-reject proof for WEEK and YEARWEEK with a NULL mode argument.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected null-handling for WEEK and YEARWEEK date functions and refined which built-ins are treated as null-preserving to improve query accuracy.
  • Tests

    • Expanded test coverage for null-rejection behavior, added registry presence checks for built-ins, and added datetime-focused cases to validate WEEK/YEARWEEK handling.

@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner labels May 31, 2026
@pantheon-ai
Copy link
Copy Markdown

pantheon-ai Bot commented May 31, 2026

@winoros I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details.

⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.

ℹ️ Learn more details on Pantheon AI.

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 31, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 31, 2026

Review Change Stack

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: 147cc68a-8aef-4a1a-a9d9-3ee22b60c5a1

📥 Commits

Reviewing files that changed from the base of the PR and between 43a335f and 9ee6612.

📒 Files selected for processing (3)
  • pkg/planner/util/null_misc.go
  • pkg/planner/util/null_misc_builtins.go
  • pkg/planner/util/null_misc_test.go
💤 Files with no reviewable changes (1)
  • pkg/planner/util/null_misc_builtins.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/planner/util/null_misc.go
  • pkg/planner/util/null_misc_test.go

📝 Walkthrough

Walkthrough

The PR stops classifying WEEK and YEARWEEK as fully NULL-preserving, adds targeted proof logic that treats only the date argument as NULL-preserving, and expands tests and registry checks to validate the corrected null-rejection behavior.

Changes

WEEK/YEARWEEK null-handling fix

Layer / File(s) Summary
Builtin NULL-preserving classification updates
pkg/planner/util/null_misc_builtins.go
Removes ast.UnaryPlus and ast.Soundex from nullRejectNullPreservingFunctions; excludes WEEK(date, mode) and YEARWEEK(date, mode) from blanket null-preserving classification.
Special-case null-rejection proof for WEEK/YEARWEEK
pkg/planner/util/null_misc.go
Adds switch cases in proveNullRejectedScalarFunc for ast.Week and ast.YearWeek that only treat the date argument as NULL-preserving; returns nonTrue+mustNull if the date is mustNull, otherwise yields no proof.
Extended tests for WEEK/YEARWEEK and builtin registry
pkg/planner/util/null_misc_test.go
Validates registry membership for nullRejectNullPreservingFunctions entries (except ast.Cast); adds innerDate datetime column, WEEK/YEARWEEK expression builders and table-driven assertions, and a newNullRejectDatetimeColumn helper.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 The WEEK and YEARWEEK once took a fall,
Mode's NULL was blamed for dropping a row;
Now date-first proof keeps left-joins whole,
Tests sing the fix and registry rows glow. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% 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 Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'planner: fix null-reject proof for WEEK and YEARWEEK' directly summarizes the main change, which is fixing the null-reject proof behavior for these two functions.
Linked Issues check ✅ Passed All code changes directly address the objectives in issue #68816: removing WEEK/YEARWEEK from NULL-preserving builtins table, adding targeted null-proof rule for date argument only, and extending registry validation.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the WEEK/YEARWEEK null-reject proof issue; removal of UnaryPlus and Soundex from the builtins map is a cleanup of unreachable entries.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-null-misc-builtin-classification

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.

❤️ Share

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

@winoros winoros force-pushed the fix-null-misc-builtin-classification branch from 43a335f to 9ee6612 Compare May 31, 2026 10:27
@codecov
Copy link
Copy Markdown

codecov Bot commented May 31, 2026

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.3535%. Comparing base (7727603) to head (9ee6612).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #68817        +/-   ##
================================================
- Coverage   76.3106%   75.3535%   -0.9572%     
================================================
  Files          2041       2023        -18     
  Lines        563469     567153      +3684     
================================================
- Hits         429987     427370      -2617     
- Misses       132566     139752      +7186     
+ Partials        916         31       -885     
Flag Coverage Δ
integration 41.3156% <0.0000%> (+1.5371%) ⬆️

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

Components Coverage Δ
dumpling 60.4610% <ø> (ø)
parser ∅ <ø> (∅)
br 49.8023% <ø> (-13.0287%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels May 31, 2026
@ti-chi-bot ti-chi-bot Bot added the lgtm label May 31, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 31, 2026

[APPROVALNOTIFIER] This PR is APPROVED

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

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 removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label May 31, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 31, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-05-31 11:11:33.850762382 +0000 UTC m=+94394.921079762: ☑️ agreed by hawkingrei.
  • 2026-05-31 13:51:50.370433934 +0000 UTC m=+104011.440751314: ☑️ agreed by qw4990.

@ti-chi-bot ti-chi-bot Bot merged commit 50bad68 into master May 31, 2026
36 checks passed
@ti-chi-bot ti-chi-bot Bot deleted the fix-null-misc-builtin-classification branch May 31, 2026 13:57
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 31, 2026

@winoros: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/check_dev_2 9ee6612 link unknown /test check-dev2

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

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

Labels

approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

planner: nullable WEEK/YEARWEEK mode can be misclassified as null-rejecting

3 participants