Repository navigation
parser: allow decimal values in SET_VAR hints - #68128
ti-chi-bot[bot] merged 2 commits into
Conversation
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds lexer state to accept numeric literals in SET_VAR value positions, introduces a ChangesSET_VAR decimal hint parsing
sequenceDiagram
participant Client
participant Lexer as hintScanner
participant Parser
participant Planner
participant SessionStore
Client->>Parser: send SQL with /*+ set_var(...0.3...) */
Parser->>Lexer: request next token stream
Lexer->>Lexer: track setVarValueState (name, '=', optional sign)
Lexer-->>Parser: emit `hintNumericLit("0.3")` when in value state
Parser->>Parser: reduce `Value` using hintNumericLit (apply sign if '-')
Parser->>Planner: produce hint AST with value "0.3" or "-0.3"
Planner->>SessionStore: apply hint for this query / create binding
SessionStore-->>Planner: confirm applied
Planner-->>Client: return result reflecting overridden variable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/parser/hintparser_test.go (1)
207-250: ⚡ Quick winAdd signed-decimal coverage for the new
SET_VARpath.The new parser path also adds
+/-handling for decimal literals, but these cases only exercise unsigned decimals. A small parser case forset_var(...=+0.3)andset_var(...=-0.3)would lock down the newAfterSignlexer state and catch regressions there.As per coding guidelines, "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
Also applies to: 341-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/hintparser_test.go` around lines 207 - 250, Add minimal test cases exercising signed decimals through the new AfterSign lexer state by extending the existing SET_VAR test row in hintparser_test.go: include two additional hint invocations in the input string, e.g. set_var(sel=+0.3) and set_var(sel=-0.3), and add corresponding entries to the expected output slice using ast.HintSetVar with VarName "sel" (or distinct names like "sel_plus"/"sel_minus") and Value "+0.3" and "-0.3" respectively; ensure you reference the same Test case that contains the other SET_VAR entries so the AfterSign path is covered without broad changes.
🤖 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/parser/hintparser_test.go`:
- Around line 207-250: Add minimal test cases exercising signed decimals through
the new AfterSign lexer state by extending the existing SET_VAR test row in
hintparser_test.go: include two additional hint invocations in the input string,
e.g. set_var(sel=+0.3) and set_var(sel=-0.3), and add corresponding entries to
the expected output slice using ast.HintSetVar with VarName "sel" (or distinct
names like "sel_plus"/"sel_minus") and Value "+0.3" and "-0.3" respectively;
ensure you reference the same Test case that contains the other SET_VAR entries
so the AfterSign path is covered without broad changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d0c5ee8-9004-408a-a54c-510034fcc75e
📒 Files selected for processing (5)
pkg/parser/hintparser.gopkg/parser/hintparser.ypkg/parser/hintparser_test.gopkg/parser/hintparserimpl.gopkg/planner/core/hint_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #68128 +/- ##
================================================
- Coverage 77.7015% 76.5343% -1.1673%
================================================
Files 1991 1992 +1
Lines 552249 559800 +7551
================================================
- Hits 429106 428439 -667
- Misses 122223 131153 +8930
+ Partials 920 208 -712
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/check-issue-triage-complete |
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 3
- Inline comments: 3
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (1)
- Decimal SET_VAR hints lack persisted-binding upgrade coverage (
pkg/parser/hintparser.y:310, pkg/parser/hintparserimpl.go:157, pkg/planner/core/hint_test.go:74)
🟡 [Minor] (1)
- Context-sensitive SET_VAR lexer state has no intent comment (
pkg/parser/hintparserimpl.go:40)
🧹 [Nit] (1)
- hintNumericLit name is broader than the token it represents (
pkg/parser/hintparser.y:54)
|
Addressed the CodeRabbit signed-decimal nit in 13f45cb as well. The existing SET_VAR parser test now covers both set_var(sel_plus=+0.3) and set_var(sel_minus=-0.3). |
13f45cb to
88ab574
Compare
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, qw4990, yudongusa 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 |
Conflict resolution keeps the hand-written recursive-descent parser and removes the goyacc infrastructure (parser.y, hintparser.y, hintparserimpl.go, generate_keyword/) that master still carried. Master's grammar changes since the merge base were ported into the hand-written parser: - LATERAL derived tables (pingcap#67076) - RETURNING clause for INSERT/UPDATE/DELETE (pingcap#65633) - INSERT ... VALUES ... AS row_alias [(cols)] (pingcap#67920) - MySQL dual passwords: RETAIN CURRENT / DISCARD OLD PASSWORD (pingcap#68028) - MariaDB syntax mode: SetMariaDB wiring, UUID type, system-versioned ROW START/END, BINLOG MONITOR privilege (pingcap#64625, pingcap#68392) - NDVRATE analyze option + whitespace-separated analyze options (pingcap#67608) - SHOW [RAW] IMPORT JOB(S) (pingcap#67521) - FLUSH STATS_DELTA with scoped object list (pingcap#67728) - EXPLAIN EXPLORE REPLAYER 'file' (pingcap#68927) - SET_VAR hints with decimal values (pingcap#68128) - LIKE ... ESCAPE explicit-escape semantics under NO_BACKSLASH_ESCAPES (pingcap#65973) - group_concat SEPARATOR charset/collation inference (pingcap#68362) - sum_int builtin token (pingcap#66085) Also fixed pre-existing hand-parser gaps surfaced by master's tests: SHOW INDEXES with WHERE, SHOW MASTER STATUS, SHOW PROFILE. ast/base.go merges master's NO_BACKSLASH_ESCAPES text transform (convertBinaryStringLiterals) into the branch's atomic textState machine, keeping the per-node sync.Once elimination. Validation: go test -tags=intest,deadlock ./... in pkg/parser (all 13 packages ok), TestHintSuite in pkg/planner/core, go build ./pkg/... ./cmd/..., make bazel_prepare. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What problem does this PR solve?
Issue Number: close #64251
Problem Summary:
SET_VARoptimizer hints reject decimal and floating-point values, so a hint like/*+ SET_VAR(tidb_default_string_match_selectivity=0.3) */cannot be parsed even though the target system variable accepts decimal values.What changed and how does it work?
Allow decimal and floating-point numeric literals only while parsing the value slot of
SET_VAR(... = value).The hint scanner now tracks a small
SET_VARvalue-position state and returns a hint-only numeric literal token for decimal values in that position. Other hint arguments still reject decimal literals, preserving the existing grammar boundary.Regenerate
pkg/parser/hintparser.goand add parser/planner coverage for decimalSET_VARvalues.Check List
Tests
Manual test details:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Bug Fixes
Tests