Repository navigation
#31 — adapters: slice CTE raw_sql to preserve SQL comments - #46
Conversation
Empirical: tests/sqlparser_comment_preservation.rs probes sqlparser 0.62's `parse → Display::to_string()` roundtrip across five comment shapes (line/block, inside/outside CTE, mid-select). All five drop the comment. Decision: accept the v0.1 fidelity limit. README "Known v0.1 fidelity limits" section documents the loss alongside the existing body-only state-modified limit (cute-dbt#14). Inline `tracked: cute-dbt#45` pointers at the engine sites (cte_engine.rs build_nodes) link the v0.2 widening — span-based slicing of `compiled_code` to preserve comments. The `sqlparser_062_drops_sql_comments` test is the hard regression gate: if sqlparser ever starts preserving comments, that assertion fires and prompts revisiting the decision. Closes #31. Follow-up: cute-dbt#45 (v0.2 widening). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 53 minutes and 59 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. 📝 WalkthroughWalkthroughREADME and cte_engine documentation now describe a v0.1 fidelity limit: sqlparser 0.62 drops SQL comments when the parsed AST is converted back to a string. A new test module empirically validates this limitation by parsing CTEs with various comment shapes and verifying that comments do not survive the roundtrip. Changessqlparser comment-drop analysis and v0.1 documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 |
![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 documents existing fidelity limits in the README.md and cte_engine.rs, specifically regarding the stripping of SQL comments by the current parser. It also introduces a new test suite to verify this behavior. Feedback focuses on the lack of assertions in these new tests, which limits their effectiveness as regression gates, and suggests refactoring fragile logic in the summary test to improve maintainability.
| fn line_comment_outside_cte_body() { | ||
| let sql = "\ | ||
| -- top-of-file comment | ||
| SELECT 1"; | ||
| let rendered = roundtrip(sql); | ||
| let preserved = rendered.contains("top-of-file comment"); | ||
| println!("[line_comment_outside] preserved={preserved}\n--- rendered ---\n{rendered}\n---"); | ||
| // Assertion is documentary — we want the test to record what | ||
| // sqlparser actually does, not to enforce a behavior we don't | ||
| // control. The decision (keep / slice / accept-limit) is wired | ||
| // off the printed output captured by `cargo test -- --nocapture`. | ||
| } |
There was a problem hiding this comment.
These "documentary" tests currently lack assertions, meaning they will always pass in CI regardless of whether sqlparser changes its behavior. To fulfill the goal of being a "regression gate" and prompting a revisit when behavior changes, these tests should assert the current behavior (that comments are dropped). This applies to all individual shape tests in this file (lines 50-108). Explicitly asserting these invariants ensures robustness against future changes in the underlying parser logic.
References
- Prioritize explicit defensive guards that document code invariants over more concise idiomatic alternatives if the guard clarifies intent and ensures robustness against future changes in logic.
There was a problem hiding this comment.
Disposition: Fixed (83d9633)
Paraphrase: the per-shape documentary tests in this file print findings but lack assertions, so they always pass in CI and don't act as a regression gate.
Action taken: replaced the five print-only shape tests + the consolidated comment_survives_decision_record test with a single sqlparser_062_drops_every_tested_comment_shape test that iterates the same five shapes (line/block, inside/outside CTE, mid-expression) with explicit sentinels (TOP_SENTINEL, INSIDE_LINE_SENTINEL, INSIDE_BLOCK_SENTINEL, TRAILING_SENTINEL, MID_EXPR_SENTINEL) and asserts each is dropped. The original sqlparser_062_drops_sql_comments hard-assertion regression gate stays as the canonical single-case gate.
Verification: cargo test --test sqlparser_comment_preservation — 2 tests pass; the second covers all five comment shapes with hard assertions.
Note: the broader framing of this PR also pivoted in 83d9633 (was: accept v0.1 limit + document; now: ship comment-preserving span-based slicing). The empirical tests are therefore retrospective regression gates for the upstream behavior that motivated slicing, not load-bearing assertions for cute-dbt's user-facing behavior — that lives in cte_engine::tests::cte_slice_preserves_sql_comments.
| #[test] | ||
| fn comment_survives_decision_record() { | ||
| // This is the consolidated documentary test: if ANY of the four | ||
| // comment shapes above survive the roundtrip, the engine could be | ||
| // kept as-is for those shapes; if NONE survive, the v0.1 fidelity | ||
| // limit is real and the decision is between raw-text slicing and | ||
| // accepting the limit. | ||
| let shapes = [ | ||
| ("-- top-of-file", "-- top-of-file comment\nSELECT 1"), | ||
| ( | ||
| "-- inside CTE body", | ||
| "WITH stg AS (\n -- inside\n SELECT id FROM users\n)\nSELECT id FROM stg", | ||
| ), | ||
| ( | ||
| "/* inside CTE body */", | ||
| "WITH stg AS (\n /* inside */\n SELECT id FROM users\n)\nSELECT id FROM stg", | ||
| ), | ||
| ( | ||
| "-- trailing on SELECT", | ||
| "WITH stg AS (\n SELECT id FROM users -- trailing\n)\nSELECT id FROM stg", | ||
| ), | ||
| ("/* mid-select */", "SELECT id /* mid-select */ FROM users"), | ||
| ]; | ||
| let mut summary = String::new(); | ||
| let mut any_preserved = false; | ||
| for (label, sql) in shapes { | ||
| let rendered = roundtrip(sql); | ||
| let preserved = rendered.contains("inside") | ||
| || rendered.contains("trailing") | ||
| || rendered.contains("top-of-file") | ||
| || rendered.contains("mid-select"); | ||
| if preserved { | ||
| any_preserved = true; | ||
| } | ||
| summary.push_str(&format!("{label}: preserved={preserved}\n")); | ||
| } | ||
| println!( | ||
| "\n=== sqlparser 0.62 comment preservation summary ===\n{summary}any_preserved={any_preserved}\n" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The comment_survives_decision_record test provides a good summary but lacks an assertion to act as a regression gate. Additionally, the logic for checking preservation (lines 137-140) is hardcoded and fragile, as it relies on specific keywords being present in the shapes array. Consider refactoring this to assert !any_preserved and deriving the check from the shapes data to improve maintainability and ensure the test remains a robust guard for code invariants.
References
- Prioritize explicit defensive guards that document code invariants over more concise idiomatic alternatives if the guard clarifies intent and ensures robustness against future changes in logic.
There was a problem hiding this comment.
Disposition: Fixed (83d9633)
Paraphrase: comment_survives_decision_record lacked an assertion and its preservation check (lines 137-140) hardcoded the keyword tokens ("inside", "trailing", "top-of-file", "mid-select"), making the gate fragile relative to the shapes array.
Action taken: deleted comment_survives_decision_record entirely. The replacement sqlparser_062_drops_every_tested_comment_shape derives the preservation check from the data itself — each (label, sql, sentinel) tuple carries its own sentinel, and the assertion uses that sentinel directly. No more hardcoded keyword list; adding a new shape now requires changing the data array only, not separately maintaining a check predicate.
The full assertion shape:
for (label, sql, sentinel) in cases {
let rendered = roundtrip(sql);
assert!(
!rendered.contains(sentinel),
"[{label}] sentinel `{sentinel}` survived sqlparser's parse → Display roundtrip — \
revisit cute-dbt#31. Got:\n{rendered}"
);
}Verification: cargo test --test sqlparser_comment_preservation — passes; each shape's sentinel is checked individually with a labeled failure message that names the shape on regression.
Switch `cte_engine::build_nodes` from `sqlparser::Display::to_string()` to span-based slicing of the original `compiled_code`. dbt preserves `--` and `/* */` SQL comments through `dbt compile`, so the source material is intact; sqlparser's `Display` was the layer dropping them. Each CTE's slice covers the full `name AS (... body ...)` wrapper via `Cte::span()`; the terminal node slices everything after the last CTE close-paren. A `ByteIndex` converts sqlparser's 1-indexed (line, column) locations to byte offsets in a single pre-pass, handling UTF-8 codepoints correctly (matters when a SQL comment contains non-ASCII text). End positions are exclusive in practice — sqlparser reports `(Location(L, C), Location(L, C+1))` for a single-character `)`. Render-layer companion changes: - `strip_sql_comments` prefilter on `is_simple_from_select` and `extract_table_leaf_refs` — a `-- pulled from raw` line comment must not bump the FROM count past 1, which would mis-classify the import CTE as a transform. String-literal-aware (handles `''` escapes). - `is_simple_from_select` trims `(` from both ends — the new sliced raw_sql puts the body inside the `name AS (...)` wrapper. - `highlightSql` rewritten as a single-pass alternation regex. Block comments now styled (previously absent); keywords inside comments and strings no longer get keyword-wrapped (cute-dbt#40 CR-1 finding becomes a regression risk with the new sliced raw_sql). `tests/sqlparser_comment_preservation.rs` keeps the regression gate locking the upstream sqlparser behavior — if 0.62+ ever starts preserving comments, the slicing layer becomes belt-and-suspenders and the decision should be revisited. Closes #31, supersedes #45 (no longer needed — the v0.2 widening this issue tracked is now in v0.1). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cmbays
changed the title
…l gate `strip_sql_comments` was CC=34, CRAP=34.00 — past the crap4rs default threshold of 25 — because the four match arms each carried inner state-machine logic (string-literal escape, line-comment terminator, block-comment terminator). Split into three private helpers (`copy_string_literal`, `skip_line_comment`, `skip_block_comment`) so the outer function is a flat 4-arm dispatcher; each helper's CC is well under 10. Behavior unchanged — render-layer tests still cover the literal-escape / keyword-inside-comment / leaf-ref cases. Verified locally: crap4rs reports 0 functions above threshold, worst is now 16.4 (PASS, down from 34). For `tests/sqlparser_comment_preservation.rs`: replace the five print-only documentary tests + the fragile keyword-extracting `comment_survives_decision_record` with a single `sqlparser_062_drops_every_tested_comment_shape` that iterates the same five shapes with explicit sentinels (e.g. `INSIDE_BLOCK_SENTINEL`) and asserts each one is dropped. Addresses Gemini-review-1 (documentary tests without assertions) and Gemini-review-2 (fragile hardcoded-keyword check). The shape coverage is preserved; the gate is now load-bearing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`src/adapters/render.rs` carries a strict CRAP threshold of 15 (crap4rs.toml :47-54). After the strip_sql_comments split, `extract_table_leaf_refs` was the remaining offender at CC=16, 88% coverage, CRAP=16.44. Refactor splits the function into three logical helpers along its natural concerns: - `is_from_or_join_keyword(tok)` — case-folded keyword match with trailing-punctuation tolerance. - `extract_leaf_table_identifier(raw)` — strip prefix/quotes/punct, validate identifier shape, return lowercased leaf or None. - `is_identifier_char(c)` — predicate that pins the heuristic's identifier grammar (ASCII alphanumeric + underscore). The parent function is now a flat iterator pipeline (filter → filter_map → filter_map → collect), reducing it to CC=1 / CRAP=1.00. The new helpers each carry their own focused unit tests (12 added) bringing every refactored function to 100% coverage: - 4 tests on `is_from_or_join_keyword` (case-fold, trailing punct, substring rejection, empty) - 5 tests on `extract_leaf_table_identifier` (simple, schema-prefix, surrounding paren, empty-leaf, non-identifier chars) - 2 tests on `is_identifier_char` (alnum + underscore, punctuation, non-ASCII) - 1 regression for `extract_table_leaf_refs` with FROM at end-of-input Coverage also pushed on `index_in_scope_tests_by_model` (86.7% → 100%) via two defensive-path tests covering (a) an in-scope test id not in the manifest unit_tests map, and (b) a unit test whose target model is missing from the manifest. Both paths were previously exercised only by the happy-path build_payload tests. Verified locally with `crap4rs --config crap4rs.toml --coverage <lcov>`: 438 functions, 0 above threshold, worst is now `find_import_node_id` at CRAP 11.02 (CC=11, 94.7% cov) — well under strict-15. EXIT 0. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add a per-model "Model SQL" `<details>` section above the CTE DAG that surfaces the model's full raw Jinja source (`node.raw_code`) verbatim. Matches dbt-docs' model-level UX without splitting Jinja per CTE (that remains a v0.2 concern). Companion to PR 11/#46 which sliced the compiled SQL per CTE; together they give reviewers both the bird's-eye Jinja view and the per-CTE compiled view. - `Node::raw_code() -> Option<&str>` added; canonical constructor takes `raw_code: Option<String>` adjacent to `compiled_code`. - Wire DTO threads `raw_code` from `node.raw_code` (`#[serde(default)]`, tolerant of older manifests). - `ModelPayload::raw_sql: Option<String>` populated from `model.raw_code()`; empty string treated as `None` so the section hides for nodes with no model file (seeds, generic tests). - `<section class="model-sql">` rendered above the CTE DAG with the verbatim Claude Design hand-back markup + CSS (collapsed by default, 24rem default height, user-resizable, reuses `sql-copy` handler). - `highlightSql` alternation widened: `{# ... #}` now styles as `.sql-comment` (semantically a comment); `{% ... %}` joins the existing `{{ ... }}` arm under `.sql-jinja` so Jinja control flow doesn't render as SQL keywords. - Tests: 5 new (3 render payload, 2 manifest reader round-trip); golden + chrome snapshots re-baselined; example `jaffle-shop-report.html` regenerated. All gates green: 315 nextest, 27 BDD, fmt, clippy, crap4rs (worst 11.0, render.rs strict-15 holds), cargo deny. Closes #47 Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
Closes #31. Supersedes #45 (no longer needed — the v0.2 widening that issue tracked is now in v0.1).
dbt preserves
--and/* */SQL comments throughdbt compile(Jinja{# #}comments are stripped, SQL comments are passed straight through tocompiled_code). cute-dbt'scte_enginewas the layer dropping them — sqlparser 0.62'sDisplay::to_string()doesn't re-emit comments through the AST roundtrip. This PR slices each CTE'sraw_sqldirectly fromcompiled_codevia sqlparser'sSpannedtrait, recovering the user's authored SQL exactly (wrapper, indentation, casing, blank lines, comments) for the report's compiled-SQL drawer.Engine change —
src/adapters/cte_engine.rsparse_cte_graph(compiled_sql)plumbs the source string throughbuild_graphtobuild_nodes.Cte, slicecompiled_sqlovercte.span()— the union of alias + body + closing-paren spans. This is thename AS (...)wrapper plus the full body. Comments inside the body survive intact.closing_paren_tokento end-of-compiled_sql. Any comments between)and the finalSELECTare preserved.ByteIndexconverts sqlparser's 1-indexed(line, column)locations to byte offsets in a single pre-pass, UTF-8-codepoint-aware (matters for non-ASCII text in comments).(Location(L, C), Location(L, C+1))for the single-character), sosql[start..end]slices directly without a+char_lencorrection.slice_or_fallbackdefends against an empty/out-of-bounds span — never panics; falls back to the ASTto_string()(so the engine degrades gracefully if sqlparser ever stops populating spans).Render-layer companion changes —
src/adapters/render.rsThe new sliced
raw_sqlincludes the CTE wrapper AND any SQL comments authored in the body, so two heuristics needed updating:strip_sql_commentsprefilter onis_simple_from_selectandextract_table_leaf_refs. A-- pulled from raw.usersline comment must not bump the FROM count past 1 (which would mis-classify the import CTE as a transform), and a-- from comment_tableline must not produce a spuriouscomment_tableleaf ref. String-literal-aware:'a -- b'and'/* not a comment */'stay verbatim; doubled-quote escapes ('it''s') handled.is_simple_from_selecttrims(from both token ends —(select, the first token of an import CTE's body inside the new wrapper, would otherwise be invisible to the case-folded keyword match.Template change —
templates/report.htmlThe pre-existing
highlightSqlran sequential regex replaces, which wrapped keywords inside already-wrapped comment/string spans. With sliced raw_sql shipping commented SQL, that's a visible regression: afrom raw.usersreference inside a--comment would render withfromkeyword-styled, breaking the comment.Rewritten as a single-pass alternation regex: block comments, line comments, string literals (with
\'and''escapes), and Jinja tags are matched FIRST as opaque blobs; keyword recognition can't reach inside them. This also adds/* */block-comment styling (previously absent), and absorbs the CodeRabbit-1 keyword-in-literal finding from #40 — that fix was a regression risk with sliced raw_sql.README —
Known v0.1 fidelity limits/Compiled-SQL fidelityThe earlier "SQL comments stripped from drawer" limit subsection is gone — it's no longer a limit. New "Compiled-SQL fidelity" subsection documents what the drawer now shows:
compiled_codeexactly asdbt compileproduced it, with the slicing rationale and a link to cute-dbt#31.Tests
tests/sqlparser_comment_preservation.rs— kept and reframed.sqlparser_062_drops_sql_commentsis the hard regression gate; if upstream ever starts preserving comments throughDisplay, the slicing layer's failure mode shifts and the decision in cute-dbt#31 should be revisited.cte_engine::tests::cte_slice_preserves_sql_comments(new) — engine-level end-to-end: parse a SQL with--and/* */comments inside CTE bodies + a comment between CTEs and the final SELECT; assert all three survive intoraw_sql.cte_engine::tests::nodes_carry_their_raw_sql— tightened: asserts the new wrapper-plus-body shape (starts at alias, contains body, ends at)).render::tests::strip_sql_comments_*+is_simple_from_select_*+extract_table_leaf_refs_*(new, 8 tests) — string-literal awareness, doubled-quote escapes, keyword-inside-comment robustness, leaf-ref ignoring comment text.render_integration__rendered_chrome_jaffle_shop.snapre-accepted — the inlinecompiled_sqlJSON now carries the wrapper-plus-body shape and the user's authored formatting (lowercase, indentation).examples/jaffle-shop-report.htmlregenerated — byte-equality gate respected.Verification
cargo fmt --all -- --checkcleancargo clippy --all-targets --locked -- -D warningscleancargo nextest run --locked --no-fail-fast— 301 passed, 1 skipped (the#[ignore]-d headless test)cargo test --test headless_zero_egress -- --ignored— passes against the regenerated example reportcargo test --test bdd --locked— 5 features, 27 scenarios passcargo test --test resource_ref_lint --locked— passes against the regenerated example reportTest plan
examples/jaffle-shop-report.htmlin a browser, click any model's compiled-SQL drawer, confirm the SQL renders with user's original formatting🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Improvements