planner: fix ErrViewInvalid caused by view constant folding#67123
Conversation
ti-chi-bot
Bot
added
the
release-note
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
sig/planner
|
Hi @expxiaoli. 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. |
📝 WalkthroughWalkthroughModify the planner to ignore truncate errors during constant predicate folding when expanding views by setting a per-build flag and using a wrapped expression context; add regression tests covering INSERT ... ON DUPLICATE KEY UPDATE with a view and STRICT_TRANS_TABLES mode. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PlanBuilder
participant ViewExpander
participant ExprCtx
participant ExprEvaluator
Client->>PlanBuilder: Build plan for INSERT ... ON DUPLICATE KEY UPDATE
PlanBuilder->>PlanBuilder: set ignoreTruncateErrForViewPredicateFolding = true
PlanBuilder->>ViewExpander: BuildDataSourceFromView()
ViewExpander->>ExprCtx: b.ctx.GetExprCtx()
Note right of ExprCtx: wrap with CtxWithHandleTruncateErrLevel(LevelIgnore)
ViewExpander->>ExprEvaluator: EvalBool(constant CNF items) using foldEvalCtx
ExprEvaluator-->>ViewExpander: evaluation result (ignore truncate errors)
ViewExpander-->>PlanBuilder: expanded view plan
PlanBuilder->>PlanBuilder: restore ignoreTruncateErrForViewPredicateFolding
PlanBuilder-->>Client: final plan
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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/executor/insert.test (1)
1794-1798: Optional: make setup idempotent for interrupted local reruns.Consider defensive
DROP ... IF EXISTSbeforeCREATEso reruns don't fail early if artifacts remain from a prior aborted run.♻️ Proposed tweak
SET `@old_sql_mode_65832` = @@sql_mode; SET sql_mode = 'STRICT_TRANS_TABLES'; +DROP VIEW IF EXISTS v_65832; +DROP TABLE IF EXISTS t_65832_0, t_65832_1; CREATE TABLE t_65832_0(c0 BLOB(304)); CREATE TABLE t_65832_1(c0 CHAR DEFAULT '0'); CREATE VIEW v_65832(c0) AS SELECT 0.9699394901011086 FROM t_65832_0 INNER JOIN t_65832_0 AS t0_alias ON t_65832_0.c0 WHERE '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/t/executor/insert.test` around lines 1794 - 1798, Make the setup idempotent by adding defensive DROP statements before the CREATEs: run DROP VIEW IF EXISTS v_65832; DROP TABLE IF EXISTS t_65832_1; DROP TABLE IF EXISTS t_65832_0; then proceed with CREATE TABLE t_65832_0(...), CREATE TABLE t_65832_1(...), and CREATE VIEW v_65832(...) so interrupted or repeated test runs won’t fail due to leftover artifacts.
🤖 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/executor/insert.test`:
- Around line 1794-1798: Make the setup idempotent by adding defensive DROP
statements before the CREATEs: run DROP VIEW IF EXISTS v_65832; DROP TABLE IF
EXISTS t_65832_1; DROP TABLE IF EXISTS t_65832_0; then proceed with CREATE TABLE
t_65832_0(...), CREATE TABLE t_65832_1(...), and CREATE VIEW v_65832(...) so
interrupted or repeated test runs won’t fail due to leftover artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 03e1d29b-db50-42ae-86ab-688e0ce0d5a0
📒 Files selected for processing (3)
pkg/planner/core/logical_plan_builder.gotests/integrationtest/r/executor/insert.resulttests/integrationtest/t/executor/insert.test
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67123 +/- ##
================================================
+ Coverage 77.6906% 78.0217% +0.3311%
================================================
Files 2016 1945 -71
Lines 552163 543515 -8648
================================================
- Hits 428979 424060 -4919
+ Misses 121442 119007 -2435
+ Partials 1742 448 -1294
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest-required |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with 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 kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/logical_plan_builder.go (1)
5195-5201: Please add a planner-level regression test for this flag.The executor regression covers the user-visible symptom, but this state toggle now guards planner-only behavior during view expansion. A focused planner test around
BuildDataSourceFromView/buildSelectionwould make future refactors safer.Based on learnings: Applies to pkg/planner/** : For planner rule or logical/physical plan changes, perform targeted planner unit tests and update rule testdata when needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/logical_plan_builder.go` around lines 5195 - 5201, Add a planner-level regression test that exercises BuildDataSourceFromView (and/or buildSelection) to verify the planner-only toggle ignoreTruncateErrForViewPredicateFolding is applied during view expansion: set up a view with a folding-eligible constant predicate that would normally raise a truncate error, invoke the planner routine that expands the view, and assert that predicate folding proceeds without surfacing a truncate error while the outer statement semantics remain unchanged; also include a complementary case ensuring the flag is false outside the expansion. Locate references to ignoreTruncateErrForViewPredicateFolding, BuildDataSourceFromView, and buildSelection when adding the test.
🤖 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/core/logical_plan_builder.go`:
- Around line 5195-5201: Add a planner-level regression test that exercises
BuildDataSourceFromView (and/or buildSelection) to verify the planner-only
toggle ignoreTruncateErrForViewPredicateFolding is applied during view
expansion: set up a view with a folding-eligible constant predicate that would
normally raise a truncate error, invoke the planner routine that expands the
view, and assert that predicate folding proceeds without surfacing a truncate
error while the outer statement semantics remain unchanged; also include a
complementary case ensuring the flag is false outside the expansion. Locate
references to ignoreTruncateErrForViewPredicateFolding, BuildDataSourceFromView,
and buildSelection when adding the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2df80cd3-d4d1-4a67-a215-402f29669cfa
📒 Files selected for processing (2)
pkg/planner/core/logical_plan_builder.gopkg/planner/core/planbuilder.go
|
/retest-required |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with 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 kubernetes-sigs/prow repository. |
|
@pantheon-bot review |
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hawkingrei, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
3 participants
What problem does this PR solve?
Issue Number: close #65832
Problem Summary:
When a view containing a constant predicate such as
WHERE ''is referenced from anINSERT ... ON DUPLICATE KEY UPDATEsubquery, TiDB may incorrectly returnErrViewInvalidin strict SQL mode.The root cause is that planner-time constant folding in
buildSelectioninherits the outer DML statement's truncate handling. EvaluatingWHERE ''under strict DML can raiseErrTruncatedWrongVal, and this planner error is later wrapped asErrViewInvalidduring view expansion.What changed and how does it work?
This PR fixes the issue by relaxing truncate handling only for planner-time constant predicate folding in
buildSelection.Instead of mutating the global
StatementContextwhile expanding a view, the fix uses a temporary expression context when evaluating constant predicates. This keeps the change local to planner evaluation and avoids leaking semantics to the outer statement.A regression test is also added to cover the reported case in
executor/insert.Check List
Tests
Test commands:
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests