planner: validate NAME_CONST arguments like MySQL#68439
Conversation
ti-chi-bot
Bot
added
release-note
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCentralize NAME_CONST second-argument validation in the planner via helper functions that enforce accepted AST forms (reject boolean pseudo-literals, allow unary-wrapped values and PI() with zero args) and add an integration test verifying accepted and rejected argument patterns and planner errors. ChangesNAME_CONST Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/planner/core/preprocess.go (1)
762-769: ⚡ Quick winAdd doc comment explaining unary value validation rules.
The function validates unary-wrapped values for NAME_CONST but lacks documentation. Add a comment explaining:
- Why only ValueExpr is accepted inside unary operations
- Why boolean pseudo-literals are rejected even when wrapped
📝 Suggested doc comment
+// isValidNameConstUnaryValue validates unary operations wrapping the second NAME_CONST argument. +// MySQL accepts unary operations only when they wrap ordinary literals (not boolean pseudo-literals +// or function calls). For example, -5 is accepted, but -TRUE and -PI() are rejected. func isValidNameConstUnaryValue(arg *ast.UnaryOperationExpr) bool {As per coding guidelines, comments should explain non-obvious intent and SQL/compatibility contracts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/preprocess.go` around lines 762 - 769, Add a doc comment above isValidNameConstUnaryValue that explains the validation rules: clarify that the function only accepts an ast.UnaryOperationExpr whose inner expression is a *driver.ValueExpr (so only literal/constant values are allowed, not nested expressions or identifiers), and state that boolean pseudo-literals are explicitly rejected by checking mysql.HasIsBooleanFlag(v.Type.GetFlag()) even when wrapped in a unary operation to preserve SQL compatibility and prevent treating boolean-like tokens as numeric/string NAME_CONST values; reference the function name isValidNameConstUnaryValue, the parameter type ast.UnaryOperationExpr, the inner type *driver.ValueExpr, and the flag check mysql.HasIsBooleanFlag(v.Type.GetFlag()) in the comment.pkg/expression/integration_test/integration_test.go (1)
1571-1583: ⚡ Quick winConsider adding a test case for non-PI() function rejection.
The test verifies that
PI()is accepted as a special-case zero-argument function, but doesn't verify that other functions are rejected. According to the PR objectives, PI() is specifically mentioned as a "MySQL-compatible special case" and other functions should continue to be rejected.Adding a test for a non-PI() function (e.g.,
NOW(),UUID(), orRAND()) would confirm that the validation specifically allows only PI() rather than all zero-argument functions.Suggested addition
tk.MustGetErrMsg("select /* issue:59459 */ name_const('bool1', true)", "[planner:1210]Incorrect arguments to NAME_CONST") tk.MustGetErrMsg("select /* issue:59459 */ name_const('bool2', false)", "[planner:1210]Incorrect arguments to NAME_CONST") tk.MustQuery("select /* issue:59459 */ name_const('pi', pi())").Check(testkit.Rows("3.141592653589793")) + tk.MustGetErrMsg("select name_const('now', now())", "[planner:1210]Incorrect arguments to NAME_CONST") tk.MustQuery("select name_const('neg', -1), name_const('negd', -1.0), name_const('str', 'x'), name_const('nil', null)").Check(testkit.Rows("-1 -1.0 x <nil>")) tk.MustGetErrMsg("select name_const('expr', 1+1)", "[planner:1210]Incorrect arguments to NAME_CONST")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/expression/integration_test/integration_test.go` around lines 1571 - 1583, Add a test in TestNameConstBuiltin to assert that only PI() is allowed as a zero-argument function for name_const and that other zero-arg functions are rejected; specifically add a MustGetErrMsg call using name_const('bad', NOW()) (or RAND()/UUID()) expecting "[planner:1210]Incorrect arguments to NAME_CONST" to ensure name_const accepts PI() but not other zero-arg functions. Locate the test in the integration_test.go TestNameConstBuiltin function where existing name_const checks are and add the new assertion next to the PI() and other error cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/preprocess.go`:
- Around line 748-760: The function isValidNameConstValue implements
MySQL-specific NAME_CONST validation but lacks a doc comment explaining the
compatibility contract; add a clear doc comment above isValidNameConstValue that
states: this function enforces MySQL NAME_CONST rules (accept only literal
values, unary expressions validated by isValidNameConstUnaryValue, and the
special-case PI() with zero args), explicitly note that driver.ValueExprs with
boolean pseudo-literals are rejected by checking
mysql.HasIsBooleanFlag(v.Type.GetFlag()), and describe the overall validation
contract so callers understand why driver.ValueExpr, ast.UnaryOperationExpr, and
ast.FuncCallExpr (with ast.PI and zero args) are treated specially.
---
Nitpick comments:
In `@pkg/expression/integration_test/integration_test.go`:
- Around line 1571-1583: Add a test in TestNameConstBuiltin to assert that only
PI() is allowed as a zero-argument function for name_const and that other
zero-arg functions are rejected; specifically add a MustGetErrMsg call using
name_const('bad', NOW()) (or RAND()/UUID()) expecting "[planner:1210]Incorrect
arguments to NAME_CONST" to ensure name_const accepts PI() but not other
zero-arg functions. Locate the test in the integration_test.go
TestNameConstBuiltin function where existing name_const checks are and add the
new assertion next to the PI() and other error cases.
In `@pkg/planner/core/preprocess.go`:
- Around line 762-769: Add a doc comment above isValidNameConstUnaryValue that
explains the validation rules: clarify that the function only accepts an
ast.UnaryOperationExpr whose inner expression is a *driver.ValueExpr (so only
literal/constant values are allowed, not nested expressions or identifiers), and
state that boolean pseudo-literals are explicitly rejected by checking
mysql.HasIsBooleanFlag(v.Type.GetFlag()) even when wrapped in a unary operation
to preserve SQL compatibility and prevent treating boolean-like tokens as
numeric/string NAME_CONST values; reference the function name
isValidNameConstUnaryValue, the parameter type ast.UnaryOperationExpr, the inner
type *driver.ValueExpr, and the flag check
mysql.HasIsBooleanFlag(v.Type.GetFlag()) in the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75160f2a-4ecd-43e8-aaea-5c82633476d3
📒 Files selected for processing (2)
pkg/expression/integration_test/integration_test.gopkg/planner/core/preprocess.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68439 +/- ##
================================================
- Coverage 77.2762% 76.4903% -0.7860%
================================================
Files 2010 1992 -18
Lines 555477 557631 +2154
================================================
- Hits 429252 426534 -2718
- Misses 125305 131053 +5748
+ Partials 920 44 -876
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qw4990 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 |
[LGTM Timeline notifier]Timeline:
|
2 participants
What problem does this PR solve?
Issue Number: close #59459
Problem Summary:
NAME_CONST()acceptedTRUE/FALSEpseudo-literals but rejectedPI(), which is incompatible with MySQL. MySQL returns incorrect-arguments errors for the boolean pseudo-literals and acceptsNAME_CONST('pi', PI()).What changed and how does it work?
This PR keeps the fix local to
NAME_CONSTpreprocessing validation:TRUE/FALSEpseudo-literals by checking the parser value expression's boolean flag;PI()as a zero-argument function value;The expression evaluation signatures are unchanged. The owner path is
pkg/planner/core/preprocess.go, whereNAME_CONSTalready validates constant-like arguments before expression building.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Tests
Summary by CodeRabbit
Bug Fixes
Tests