fix(parse): PARTIAL -> ERROR only when no assignment parsed (v1.0.6)#42
Conversation
v1.0.5's parse_set() downgraded PARTIAL to ERROR whenever the tokenizer flagged an error. That was the right call for top-level malformed input (`SET = 1`, `SET datestyle = ;`, bare `$word`) but too aggressive for multi-assignment SETs where one element is malformed alongside well-formed ones: SET sql_mode='TRADITIONAL', whatever = , autocommit=1 -- v1.0.5: ERROR (the `whatever =` triggered flag_error, -- poisoning the whole AST even though sql_mode and autocommit -- parsed cleanly). -- now: PARTIAL with VAR_ASSIGNMENT[sql_mode] + VAR_ASSIGNMENT[autocommit] -- in the AST; consumer can use the successful elements. Tighten the downgrade rule: only promote PARTIAL -> ERROR when the parse produced NO children at all. When the AST has at least one well-formed assignment, leave the status at PARTIAL so consumers can still see and use the successful elements. `SET = 1`, `SET datestyle = ;`, bare `$word`, `SET autocommit =`, `SET search_path = ,public` etc. are unaffected -- they all produce empty ASTs and still surface as ERROR. Validated end-to-end via ProxySQL's setparser_parsersql_test (270/270 passing), which exercises both the regex parser and the ParserSQL adapter on the same shared fixtures and was where the v1.0.5 regression surfaced.
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 47 minutes and 48 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
1 participant
Summary
Hot-fix for an over-aggressive PARTIAL -> ERROR downgrade introduced
in v1.0.5 (#40).
The regression
v1.0.5's `parse_set()` downgraded `PARTIAL` to `ERROR` whenever
`tokenizer_.has_error()` was set. That was the right call for
top-level malformed input (`SET = 1`, `SET datestyle = ;`, bare
`$word`), but too aggressive for multi-assignment SETs where one
element is malformed alongside well-formed ones:
```sql
SET sql_mode='TRADITIONAL', whatever = , autocommit=1
```
v1.0.5: `ERROR` (the `whatever =` triggered `flag_error()`,
poisoning the whole AST even though `sql_mode` and `autocommit`
parsed cleanly into the children).
The fix
Tighten the downgrade rule in `Parser::parse_set()`: only promote
`PARTIAL` -> `ERROR` when the parse produced no children at all.
The originally-targeted clearly-malformed cases still surface as
`ERROR` (all of them produce empty ASTs):
`SET = 1`, `SET datestyle = ;`, `SET autocommit =`,
`SET search_path = ,public`, `SET search_path TO`,
`SET search_path = $user`, bare `SET`, `SET GLOBAL`.
Validation gate
Per the lesson from PR #39 / #40 -> v1.0.5: this fix was validated
end-to-end through ProxySQL's actual consumer (`setparser_parsersql_test`)
before being committed here.
270/270 passing, was 10 failing on v1.0.5
`setparser_test3-t` (regex-parser variants, share the same
fixture file): all still passing (1 + 226 + 224)
Downstream
Tag as v1.0.6 after merge. ProxySQL's `deps/parsersql` bump
will be updated to 1.0.6 in the same proxysql commit (v1.0.5 was
tagged but never consumed downstream, so there's no in-flight
breakage).