planner: support more null-reject cases in plan cache - #66992
planner: support more null-reject cases in plan cache#66992Reminiscent wants to merge 3 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/L
|
Hi @Reminiscent. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
|
/ok-to-test |
📝 WalkthroughWalkthroughImplements plan-context-aware, conservative null-rejection checks used during join conversion and constraint inference, refactors related APIs to accept PlanContext, adds util helpers for symbolic/parameter-aware null-rejection, and extends tests to validate plan-cache behavior across many prepared-statement scenarios. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Command failed 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 (1)
tests/integrationtest/t/planner/core/plan_cache.test (1)
265-269: Use a different non-NULL binding on the second<=>execution.Lines 268-269 currently execute with
@v = 1twice, so this block can still pass if the cached plan accidentally bakes in the first bound value. Switching the second run to another non-NULL value makes the cache-reuse check meaningful end-to-end, then re-record the matching section intests/integrationtest/r/planner/core/plan_cache.result.Suggested change
prepare stmt from 'select t1.a as t1_a, t2.a as t2_a from t1 left join t2 on t1.a = t2.b where t2.a <=> ? order by t1.a'; set `@v` = 1; execute stmt using `@v`; -set `@v` = 1; +set `@v` = 2; execute stmt using `@v`; select @@last_plan_from_cache;🤖 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 265 - 269, The test currently binds the same non-NULL value twice which can mask a bug where a cached plan bakes in the first bound value; update the second execution to use a different non-NULL binding (e.g., change the second "set `@v` = 1" to "set `@v` = 2") before the second "execute stmt using `@v`" so the plan-reuse behavior is meaningfully tested, then re-record the expected output in tests/integrationtest/r/planner/core/plan_cache.result to reflect the new value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integrationtest/t/planner/core/plan_cache.test`:
- Around line 265-269: The test currently binds the same non-NULL value twice
which can mask a bug where a cached plan bakes in the first bound value; update
the second execution to use a different non-NULL binding (e.g., change the
second "set `@v` = 1" to "set `@v` = 2") before the second "execute stmt using `@v`"
so the plan-reuse behavior is meaningfully tested, then re-record the expected
output in tests/integrationtest/r/planner/core/plan_cache.result to reflect the
new value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 02277bca-2b41-445f-9a4d-2526fed4c676
📒 Files selected for processing (5)
pkg/expression/constant_fold.gopkg/expression/expression.gopkg/expression/expression_test.gotests/integrationtest/r/planner/core/plan_cache.resulttests/integrationtest/t/planner/core/plan_cache.test
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit bcbf953 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## master #66992 +/- ##
================================================
- Coverage 77.7132% 77.1646% -0.5486%
================================================
Files 2013 1932 -81
Lines 551161 541235 -9926
================================================
- Hits 428325 417642 -10683
- Misses 121105 123395 +2290
+ Partials 1731 198 -1533
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
715052a to
085e763
Compare
ti-chi-bot
Bot
removed
the
size/L
|
[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 |
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/planner/core/casetest/index/index_test.go (1)
397-409: Seed data and assert results here, not just the access path.With an empty table, this only proves the planner chose/cached a plan. A cached partial-index plan that incorrectly drops
a IS NULLrows would still pass. Add at least one(NULL, 123)row and one non-NULL row, then verify theNULLrow stays excluded across the cache hit.Suggested test hardening
tk.MustExec("drop table if exists t") tk.MustExec("create table t(a int, b int, index idx3(b) where a is not null)") + tk.MustExec("insert into t values (null, 123), (2, 123)") tk.MustExec("prepare stmt2 from 'select * from t use index(idx3) where b = ? and a + ? > 1'") tk.MustExec("set `@b` = 123") tk.MustExec("set `@c` = 0") - tk.MustExec("execute stmt2 using `@b`, `@c`") - tk.MustExec("execute stmt2 using `@b`, `@c`") + tk.MustQuery("execute stmt2 using `@b`, `@c`").Check(testkit.Rows("2 123")) + tk.MustQuery("execute stmt2 using `@b`, `@c`").Check(testkit.Rows("2 123")) tkProcess = tk.Session().ShowProcess() ps[0] = tkProcess tk.Session().SetSessionManager(&testkit.MockSessionManager{PS: ps}) tk.MustQuery(fmt.Sprintf("explain for connection %d", tkProcess.ID)).CheckContain("idx3") - tk.MustExec("execute stmt2 using `@b`, `@c`") + tk.MustQuery("execute stmt2 using `@b`, `@c`").Check(testkit.Rows("2 123")) tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("1"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/index/index_test.go` around lines 397 - 409, The test currently only checks that the partial-index plan (index idx3 on table t) was chosen/cached for prepared statement stmt2; strengthen it by inserting seed rows into t (e.g., one row with (NULL, 123) and one row with (1, 123) or similar), run execute stmt2 using `@b`, `@c` before and after the cache hit, and assert the query results explicitly (use tk.MustQuery(...).Check or equivalent) to verify the NULL row is not returned and the non-NULL row is returned both pre- and post-cache; keep the existing checks for explain/last_plan_from_cache around the executes to ensure you still validate caching behavior for stmt2 and idx3.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/operator/logicalop/logical_join.go`:
- Around line 330-340: The ConvertOuterToInnerJoin implementation calls
util.IsNullRejected directly, causing different null-reject semantics than
simplifyOuterJoin; update ConvertOuterToInnerJoin to call the new isNullRejected
wrapper instead (so it performs
expression.PushDownNot(ctx.GetNullRejectCheckExprCtx(), expr) and falls back to
isNullRejectedSpecially when needed) and pass the same ctx, schema and expr
parameters; replace direct util.IsNullRejected(...) usages in
ConvertOuterToInnerJoin with isNullRejected(ctx, schema, expr) to ensure
consistent behavior.
In `@pkg/planner/util/null_misc.go`:
- Around line 180-205: In isNullRejectedConservativeLeaf, short-circuit by
calling exprAlwaysNullForNullReject(expr, probe) at the top and return true if
it reports the expr always yields NULL for the given probe; this must happen
before the ScalarFunction type assertion so non-scalar leaves (e.g. raw column
operands discovered by LogicAnd) are correctly recognized as NULL-producing.
Keep the existing constant and scalar-function handling afterwards; the change
is just an early return using exprAlwaysNullForNullReject to avoid dropping
non-scalar leaves prematurely.
---
Nitpick comments:
In `@pkg/planner/core/casetest/index/index_test.go`:
- Around line 397-409: The test currently only checks that the partial-index
plan (index idx3 on table t) was chosen/cached for prepared statement stmt2;
strengthen it by inserting seed rows into t (e.g., one row with (NULL, 123) and
one row with (1, 123) or similar), run execute stmt2 using `@b`, `@c` before and
after the cache hit, and assert the query results explicitly (use
tk.MustQuery(...).Check or equivalent) to verify the NULL row is not returned
and the non-NULL row is returned both pre- and post-cache; keep the existing
checks for explain/last_plan_from_cache around the executes to ensure you still
validate caching behavior for stmt2 and idx3.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d6ea580-3193-49a0-bfa0-477400d414d9
📒 Files selected for processing (7)
pkg/planner/core/casetest/index/index_test.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/partidx/BUILD.bazelpkg/planner/core/partidx/check_constraint.gopkg/planner/util/null_misc.gotests/integrationtest/r/planner/core/plan_cache.resulttests/integrationtest/t/planner/core/plan_cache.test
085e763 to
bcbf953
Compare
ti-chi-bot
Bot
added
size/XXL
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/planner/util/null_misc.go (2)
244-244: Redundant fallback toexprAlwaysNullForNullReject.Line 244 returns
exprAlwaysNullForNullReject(expr, probe), but this case was already checked at line 218 at the function entry. If we reach line 244,exprAlwaysNullForNullRejectalready returned false for this expression. This final return will always be false for ScalarFunctions not in the whitelist.Consider returning
falsedirectly for clarity:Suggested change
if _, ok := expression.CompareOpMap[sf.FuncName.L]; ok { args := sf.GetArgs() return len(args) >= 2 && (exprAlwaysNullForNullReject(args[0], probe) || exprAlwaysNullForNullReject(args[1], probe)) } - return exprAlwaysNullForNullReject(expr, probe) + return false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/util/null_misc.go` at line 244, The final return currently calls exprAlwaysNullForNullReject(expr, probe) again for ScalarFunctions not in the whitelist, but that check was already performed at the function entry (via exprAlwaysNullForNullReject), so replace the redundant exprAlwaysNullForNullReject call with a direct return false in the default/else branch handling ScalarFunctions (referencing expr and probe); ensure the code returns false for non-whitelisted ScalarFunctions instead of re-invoking exprAlwaysNullForNullReject.
25-37: Consider documenting the whitelist selection criteria.The
nullRejectPlanCacheStrictFuncswhitelist includes arithmetic operations that propagate NULL. This is a conservative and correct set. Consider adding a brief comment explaining why these specific functions are included (all return NULL if any operand is NULL) to help future maintainers understand the selection criteria.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/util/null_misc.go` around lines 25 - 37, Update the comment for nullRejectPlanCacheStrictFuncs to explain the selection criteria: state that this whitelist contains arithmetic operators (referenced as ast.Abs, ast.Div, ast.IntDiv, ast.Minus, ast.Mod, ast.Mul, ast.Plus, ast.UnaryMinus, ast.UnaryPlus) whose semantics guarantee NULL result if any input operand is NULL, and note that this is intentionally conservative for plan-cache rejection logic; keep the map entries unchanged and add a short sentence describing that rationale so future maintainers understand why these specific functions were chosen.pkg/planner/util/path_test.go (1)
148-177: Consider adding negative test cases.The test only verifies cases where
IsNullRejectedByInnerColumnreturnstrue. For better coverage and to guard against false positives, consider adding cases that should returnfalse:
- A predicate like
col <=> 1(NullEQ - should not be null-rejecting)- A predicate like
IS NULL(col)(should not be null-rejecting)- A predicate referencing a different column
Example negative test case
{ name: "null-safe equal should not reject null", expr: expression.NewFunctionInternal( sctx.GetExprCtx(), ast.NullEQ, types.NewFieldType(mysql.TypeTiny), col, one, ), expected: false, // Add expected field to struct },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/util/path_test.go` around lines 148 - 177, Add negative test cases to the existing table-driven tests for IsNullRejectedByInnerColumn: extend the test struct to include an expected bool, add entries using expression.NewFunctionInternal for predicates that should NOT reject NULL (e.g. ast.NullEQ with col and one, ast.IsNull with col, and a predicate referencing a different column), and assert require.False(same call) or compare to tt.expected using sctx.GetPlanCtx(), col, one/zero as appropriate; keep existing positive cases and change the loop to check require.Equal(t, tt.expected, util.IsNullRejectedByInnerColumn(sctx.GetPlanCtx(), col, tt.expr), tt.name) so both true and false expectations are validated.
🤖 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.go`:
- Line 244: The final return currently calls exprAlwaysNullForNullReject(expr,
probe) again for ScalarFunctions not in the whitelist, but that check was
already performed at the function entry (via exprAlwaysNullForNullReject), so
replace the redundant exprAlwaysNullForNullReject call with a direct return
false in the default/else branch handling ScalarFunctions (referencing expr and
probe); ensure the code returns false for non-whitelisted ScalarFunctions
instead of re-invoking exprAlwaysNullForNullReject.
- Around line 25-37: Update the comment for nullRejectPlanCacheStrictFuncs to
explain the selection criteria: state that this whitelist contains arithmetic
operators (referenced as ast.Abs, ast.Div, ast.IntDiv, ast.Minus, ast.Mod,
ast.Mul, ast.Plus, ast.UnaryMinus, ast.UnaryPlus) whose semantics guarantee NULL
result if any input operand is NULL, and note that this is intentionally
conservative for plan-cache rejection logic; keep the map entries unchanged and
add a short sentence describing that rationale so future maintainers understand
why these specific functions were chosen.
In `@pkg/planner/util/path_test.go`:
- Around line 148-177: Add negative test cases to the existing table-driven
tests for IsNullRejectedByInnerColumn: extend the test struct to include an
expected bool, add entries using expression.NewFunctionInternal for predicates
that should NOT reject NULL (e.g. ast.NullEQ with col and one, ast.IsNull with
col, and a predicate referencing a different column), and assert
require.False(same call) or compare to tt.expected using sctx.GetPlanCtx(), col,
one/zero as appropriate; keep existing positive cases and change the loop to
check require.Equal(t, tt.expected,
util.IsNullRejectedByInnerColumn(sctx.GetPlanCtx(), col, tt.expr), tt.name) so
both true and false expectations are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9071691e-5b97-4fa5-aea7-5a76b1b36aa8
📒 Files selected for processing (11)
pkg/planner/core/casetest/index/index_test.gopkg/planner/core/casetest/rule/testdata/outer2inner_in.jsonpkg/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/BUILD.bazelpkg/planner/core/partidx/check_constraint.gopkg/planner/util/null_misc.gopkg/planner/util/path_test.gotests/integrationtest/r/planner/core/plan_cache.result
💤 Files with no reviewable changes (1)
- pkg/planner/core/operator/logicalop/BUILD.bazel
|
@Reminiscent: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@Reminiscent: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: TBD
Problem Summary:
TBD
What changed and how does it work?
TBD
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
Tests
Bug Fixes
Refactor