planner: fix null-reject based FD not-null inference

[hawkingrei]

What problem does this PR solve?

Issue Number: ref #68026

Problem Summary:

ExtractNotNullFromConds was using a whole-schema null-reject proof for each predicate and then
marking every referenced column as not null. For predicates like c = 1 or d = 1, the condition
is null-rejected for the output row, but that does not prove both c and d are individually not
null. That over-strengthened FD derivation and let ONLY_FULL_GROUP_BY accept invalid queries.

What changed and how does it work?

• change ExtractNotNullFromConds to check null rejection per referenced column instead of once
  against the whole schema
• add a planner regression in pkg/planner/funcdep/extract_fd_test.go
• add an integration regression in tests/integrationtest/t/executor/aggregate.test

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

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Fix ONLY_FULL_GROUP_BY false acceptance caused by over-aggressive not-null inference in FD extraction.

Summary by CodeRabbit

• Bug Fixes

  • Improved validation of GROUP BY clauses under ONLY_FULL_GROUP_BY SQL mode to properly reject queries selecting non-aggregated, non-grouped columns that lack functional dependency.
  • Enhanced null-rejection evaluation in functional dependency analysis for more accurate query constraint handling.

• Tests

  • Added test coverage for edge cases involving GROUP BY validation with complex WHERE predicates.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request fixes FD extraction logic by evaluating null-rejection per column rather than whole-schema basis. This prevents incorrect functional dependency inference when WHERE conditions contain OR clauses. Tests validate the fix with new negative test cases for GROUP BY queries with disjunctive WHERE predicates.

Changes

Cohort / File(s)	Summary
FD Extraction Core Logic pkg/planner/util/funcdep_misc.go	Modified ExtractNotNullFromConds to call IsNullRejected with a schema scoped to each individual column, rather than the full plan schema, ensuring null-rejection is checked per-column basis.
FD Extraction Tests pkg/planner/funcdep/extract_fd_test.go	Extended test harness with err field; adds new test scenario that validates rejection of GROUP BY queries selecting non-aggregated columns when functional dependency cannot be proven under OR predicates with nullable unique constraints.
Integration Tests tests/integrationtest/t/executor/aggregate.test, tests/integrationtest/r/executor/aggregate.result	Adds negative test case verifying Error 1055 is raised when selecting non-grouped column under ONLY_FULL_GROUP_BY mode with OR-based WHERE clause and nullable unique key.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• planner: prove null-reject for deferred expressions #67789: Also modifies ExtractNotNullFromConds in pkg/planner/util/funcdep_misc.go with adjustments to IsNullRejected invocation semantics.

Suggested labels

sig/planner, size/L, approved, lgtm, AI-Correction

Suggested reviewers

• AilinKid
• winoros
• qw4990

Poem

🐰 A rabbit hops through FDs per-column wise,
No more whole-schema null-reject guise,
Each column checked with surgical care,
OR conditions now treated fair,
Functional deps now true and bright! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.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
Linked Issues check	✅ Passed	All code changes directly address issue #68026 requirements: ExtractNotNullFromConds modified for per-column null-rejection checks instead of whole-schema checks, with regression tests covering the reported case where (b,c) should not be upgraded to strict FD.
Out of Scope Changes check	✅ Passed	All changes are in-scope: modifications to ExtractNotNullFromConds in util/funcdep_misc.go, planner regression test in extract_fd_test.go, and integration tests in aggregate test files directly address the null-rejection inference bug without introducing unrelated changes.
Title check	✅ Passed	The PR title directly and accurately captures the main fix: correcting null-rejection based functional dependency not-null inference in the planner, which is the core change across all modified files.
Description check	✅ Passed	The pull request description comprehensively addresses the issue, clearly explains the problem, details the changes made, and provides proper issue linkage and test coverage checkboxes.

_{✏️ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

pkg/planner/util/funcdep_misc.go (1)

44-48: Correctly fixes the over-approximation; minor redundant work per column.

The per-column null-rejection check resolves the bug for predicates like c = 1 OR d = 1 — neither c nor d is null-rejected individually under a single-column schema, so neither gets marked, which is the intended behavior.

Minor inefficiency: IsNullRejected calls expression.PushDownNot on the predicate on every invocation (see pkg/planner/util/null_misc.go:99), so for a condition referencing K columns, the same PushDownNot is recomputed K times. You could push-down-not once per condition before the inner loop, then call proveNullRejected directly. Not a correctness issue; only worth doing if profiling shows overhead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/util/funcdep_misc.go` around lines 44 - 48, The loop in
funcdep_misc.go repeatedly calls IsNullRejected which internally runs
expression.PushDownNot for the same condition multiple times; to avoid redundant
work, compute pushed := expression.PushDownNot(condition) once before iterating
cols and then call proveNullRejected(p.SCtx(), pushed,
expression.NewSchema(col)) (the internal helper used by IsNullRejected) for each
col, inserting int(col.UniqueID) when true; keep behavior unchanged otherwise.

🤖 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/funcdep_misc.go`:
- Around line 44-48: The loop in funcdep_misc.go repeatedly calls IsNullRejected
which internally runs expression.PushDownNot for the same condition multiple
times; to avoid redundant work, compute pushed :=
expression.PushDownNot(condition) once before iterating cols and then call
proveNullRejected(p.SCtx(), pushed, expression.NewSchema(col)) (the internal
helper used by IsNullRejected) for each col, inserting int(col.UniqueID) when
true; keep behavior unchanged otherwise.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 66e0deb0-221c-4698-8b5c-f9786e5f574f

📥 Commits

Reviewing files that changed from the base of the PR and between 6f2fe56 and d534bc4.

📒 Files selected for processing (4)

• pkg/planner/funcdep/extract_fd_test.go
• pkg/planner/util/funcdep_misc.go
• tests/integrationtest/r/executor/aggregate.result
• tests/integrationtest/t/executor/aggregate.test

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.0215%. Comparing base (bc991bf) to head (d534bc4).
⚠️ Report is 38 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #68050        +/-   ##
================================================
- Coverage   77.7722%   77.0215%   -0.7507%     
================================================
  Files          1990       1972        -18     
  Lines        551424     556914      +5490     
================================================
+ Hits         428855     428944        +89     
- Misses       121649     127648      +5999     
+ Partials        920        322       -598     

Flag	Coverage Δ
integration	42.0481% <100.0000%> (+2.2463%)	⬆️

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

Components	Coverage Δ
dumpling	60.4888% <ø> (ø)
parser	∅ <ø> (∅)
br	50.0597% <ø> (-13.0338%)	⬇️

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

[hawkingrei]

/retest

[hawkingrei]

/retest

[hawkingrei]

/retest

[hawkingrei]

/retest

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, 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:

• OWNERS [AilinKid,qw4990]
• 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

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2026-05-07 02:33:23.668119347 +0000 UTC m=+321476.541469319: ☑️ agreed by qw4990.
• 2026-05-07 10:59:07.194466642 +0000 UTC m=+351820.067816614: ☑️ agreed by AilinKid.