Repository navigation
#473 — refactor raw_scan masking to the sqlparser tokenizer (evaluation → adopt) - #475
Conversation
…e hand-rolled SQL lexer)
Reimplement raw_scan's SQL-region masking via sqlparser::tokenizer::Tokenizer
(GenericDialect — the same dialect cte_engine parses with, so raw & compiled
never diverge on SQL lexing). Two passes: (1) Jinja masking via the existing
offset-preserving render::find_close/find_expr_close helpers (sqlparser does not
understand Jinja); (2) tokenize the Jinja-masked text and blank the byte span of
every string/comment/dollar-quote token, with the tokenizer's own escape
handling. Fail-closed on a TokenizerError (blank the whole text — nothing leaks).
Deletes the hand-rolled SQL string/comment/dollar-quote machinery — scan_sql_quoted
(CC 14), scan_dollar_quote (CC 12), the SQL arms of classify_opener (CC 16→5),
scan_line_comment, scan_block_comment, quote_has_string_prefix. The subtle SQL
string-escape surface (doubled-quote, prefixed-backslash E'…'/U&'…') now lives in
the tokenizer, not in this module.
Soundness preserved (over-mask-only, never under-mask): a quoted identifier
(Token::Word{quote_style:Some}) is blanked too — both for paren-balance soundness
(an interior `)` is not a structural paren) and behavior-parity with the old
masker (quoted-ident names are already honestly omitted at the fill layer). A
line comment's terminating `\n` is preserved so emitted line/col stays faithful
to the true raw source (byte offsets are the anchor; the `\n` carries no name).
Goldens byte-identical (examples/ unchanged). 62 raw_scan tests (was 57: +5
pinning tokenizer-error fail-closed, paren-in-quoted-ident, bare-Word-live,
dialect-parity). Full nextest + bdd + fmt + clippy --locked + deny + crap4rs
green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
…cs to match masking behavior The mask_regions module doc + lexical-region table still described quoted identifiers as 'left live', contradicting is_maskable_token (which blanks them, w.quote_style.is_some()) and its own detailed soundness rationale. A doc asserting the inverse of the code on a soundness-load-bearing decision is a correctness hazard; align the docs with the (sound, over-masking) behavior. Doc-only — goldens + gates unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 18 minutes and 7 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthrough
ChangesTwo-pass masking refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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)
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 |
📄 Rendered report previewAll golden examples regenerated cleanly. 🟡 Golden examplesCommitted to
🐶 Live dogfood previewThis PR doesn't touch 🧭 Explore previewThe two-page 🟡 Golden exploreThe committed
🐶 Live exploreThis PR doesn't touch ▶ Open ↗ opens the report or explorer in your browser in one The Pages preview may take ~1 min to update after this comment Alternative: GitHub CLI# gh CLI >= 2.63 extracts into ./report-preview-playground/.
gh run download 28003299556 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the SQL masking logic in src/adapters/raw_scan.rs into a robust two-pass process: a Jinja-masking pass followed by a SQL-tokenization pass using sqlparser's tokenizer. This replaces several hand-rolled scanners, improving consistency and correctness. The review feedback recommends adding a defensive guard to ensure start < end before calling blank to prevent potential panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Per gemini review on PR #475: guard blank() against an inverted (from >= to) span. The upper bound was already clamped (to.min(len)); the inverted case was unreachable today (byte_of clamps to len + token spans are ordered + the line-comment end-=1 is guarded by end > start) but would panic if any future caller passed an inverted span. The masker must fail closed, never crash the render on a manifest-derived span, so make blank() total. Pinned by blank_is_total_on_malformed_spans. Behavior on valid spans unchanged (goldens byte-stable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
…ingle_char_names) The blank totality test used 5 single-char bindings (a..e), tripping clippy::many_single_char_names under -D warnings. Rename to valid/clamped/ empty/inverted/past_end. Test-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
1 participant
Evaluation outcome for #473: ADOPT. Reimplements
raw_scan's SQL-region lexical masking onsqlparser's tokenizer (the same engine the compiled side already parses with), deleting the hand-rolled SQL string/comment/dollar-quote lexer. Behavior-preserving: all goldens byte-identical.What changed
mask_regionsis now two passes: pass 1mask_jinja(unchanged — sqlparser doesn't understand Jinja; reusesrender::find_close/find_expr_closeso the raw-span path and the zone path agree on Jinja boundaries on the same model), pass 2mask_sql_tokens—Tokenizer::new(&GenericDialect{}, …).tokenize_with_location()then blank the byte span of every string / comment / dollar-quote / quoted-identifier token via the now-sharedcte_engine::ByteIndex::byte_of.scan_sql_quoted(CC 14, the doubled-quote + prefixed-backslash escape logic),scan_dollar_quote(CC 12), the line/block comment scanners,quote_has_string_prefix, and the SQL arms ofclassify_opener(which now only dispatches Jinja).mask_regionsCC 16 → 10.GenericDialect).Soundness (the load-bearing property)
The masker may only ever OVER-mask (a name omitted → honest "no anchor") and must never UNDER-mask (a string/comment interior name leaks as a live span → a false anchor). Preserved on every axis:
U&'…\…') → blank the entire text = maximal over-mask (tokenizer_error_fails_closed_blanks_everything).Word{quote_style:Some}, e.g.")") are blanked, not left live — two honest-direction reasons: (a) paren-balance soundness (a)insideselect ")" as xis not a structural paren; leaving it live truncated the CTE body span — a false span, now pinned byparen_inside_quoted_identifier_does_not_truncate_cte_span); (b) behavior-parity (such names are already honestly omitted at the fill layer). An unquotedWordstays live (the onlyWordcarrying a matchable name).\npreserved — the tokenizer'sSingleLineCommentspan includes its terminating newline; blanking it shifted downstream line/col (byte offsets stayed correct). Caught mid-build via 4 churned goldens, diagnosed not blindly regenerated, fixed to match the old scanner exactly → goldens back to byte-identical.Verification
E'/U&'/N'/X', Postgres$func$-body nesting, mismatched dollar tags, CRLF/CR/U+2028 line endings, comment-glued names, placeholders, backtick identifiers. Verdict: sound; fail-closed verified.git diff --exit-code -- examples/clean — independently re-verified).fmt --check,clippy --all-targets --locked -D warnings, fullnextest,crap4rs,cargo deny,cargo doc -D warnings.Notes for review
balanced_closewas kept — it's the structural paren-matcher for CTE-body extent (operates over the masked text, not a region scanner); deleting it would breakcte_raw_span. Still sound because quoted-identifier interiors are now masked, so only live parens survive.mask_regionsmodule doc that contradictedis_maskable_token's (sound) behavior.is_maskable_tokenis exhaustive over sqlparser 0.62 but usesmatches!with an implicit wildcard, so a future dep bump adding a string variant could silently under-mask — tracked to add a compile-time exhaustiveness guard.Settles the masker before S2 (#470) inherits it.
Closes #473.
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests