fix(tesseract): parenthesize member SQL spliced into filter templates

[waralexrom]

Summary

A filter template places its own operator next to the member's rendered SQL ({{ column }} = {{ value }}, {{ column }} IS NOT NULL, …). A member whose sql is a bare expression then re-associates: when its own top-level operator binds weaker than the template's, that operator captures only the tail of the member expression.

Reported on Athena/Trino (CORE-726):

- name: measure_1
  sql: amount
  type: sum
- name: measure_2
  sql: "{measure_1} IS NOT NULL"
  type: boolean

measure_2 equals true rendered HAVING (sum("orders".amount) IS NOT NULL = CAST(? AS BOOLEAN)), which Trino rejects with mismatched input '='. Aggregate-typed measures were safe by accident, being wrapped in their own function call.

Two aggravating factors: it is not measure-only (a dimension sql: "amount IS NOT NULL" breaks the same way in WHERE), and where the member's top level is AND/OR the mis-parse stays valid SQL that silently returns a different row set — no error at all.

Tesseract only, per the ticket; the legacy planner is left as is.

Changes

• FilterSqlContext renders the member as a single operand at construction time, so every filter operator receives it already pinned. The field is now private behind member_sql() so a new call site cannot bypass the wrapping.
• Atomicity is decided by the existing sql_expression_scanner::is_top_level_compound — the same scanner ParenthesizeSqlNode already uses for SqlCall arguments. Plain columns, aggregates, casts, CASE and already-parenthesized expressions keep their shape, so the wrapping does not spread through the filters that never had the hazard. Across the whole repo exactly one existing assertion changed: base-query.test.ts required WHERE (1 = 1 = ?), i.e. it had encoded invalid SQL.
• New sql_expression_scanner::ends_in_line_comment: an expression ending in -- … gets its closing parenthesis on a line of its own, since on the same line the comment would swallow it. (Not a regression — such a member was already fatal in filters, amount + 1 -- note > $1 does not parse either — but the wrapping is the natural place to close it.)

Testing

New packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts — 26 tests, 22 of which failed before the fix.

Postgres works as a full-fidelity polygon because it exhibits both failure modes:

• a > b = c is a syntax error there (comparisons are non-associative), so a member sql: "amount > 50" under equals/notEquals hard-fails — in WHERE, and in HAVING over sum(...). Closest analogue of the reported Trino error.
• A member with a top-level AND/OR diverges silently: row-level assertions cover equals / notEquals / IN / NOT IN / set / notSet against real data.
• x IS NOT NULL = FALSE happens to parse the intended way on Postgres, so the ticket's literal shape is pinned on the emitted SQL, with a comment explaining why.
• gt/gte/lt/lte, the LIKE family and the date operators cannot be made to diverge on Postgres (its precedence agrees), so those assert the whole rendered predicate — which also pins the operator and wildcard shape, not just the parentheses.
• Two guards that the wrapping stays off atomic members (plain column dimension, aggregate measure), so the diff cannot quietly spread.

Suite	Result
filter-member-sql-parens (Tesseract)	26/26 pass (22 failed before the fix)
same file under legacy planner	skips cleanly via getEnv('nativeSqlPlanner')
dist/test/integration/postgres (Tesseract)	46 suites, 543 passed, 21 skipped
dist/test/unit (Tesseract)	36 suites, 716 passed
cargo test -p cubesqlplanner	1193 passed
cargo test -p cubesqlplanner --features integration-postgres	1193 passed

Note for reviewers

The same trailing-line-comment shape exists in sql_nodes/parenthesize.rs and sql_templates/plan.rs::convert_tz. Left untouched to keep this diff scoped — happy to fold in if preferred.

[claude]

Claude finished @waralexrom's task in 3m 54s —— View job

---

Review: sound fix, well-targeted — 1 medium, 2 low. No blocking issues.

Full review

• Gather context (read commit, diff, surrounding code)
• Review sql_expression_scanner.rs changes
• Review filter_sql_context.rs and all operator call sites
• Review tests
• Post review

What the fix gets right

The shape is the right one. Rendering the operand once in FilterSqlContext::new and making member_sql private behind an accessor means the invariant is established at the single point where the context is built, and a future operator cannot re-introduce the bug by reaching for the raw field. I verified all 12 call sites migrated to member_sql() and that no path in physical_plan/filter/ still touches the field directly.

Reusing is_top_level_compound rather than adding a second atomicity notion is also correct — the same predicate already governs ParenthesizeSqlNode, so a member SQL is now classified identically whether it is spliced into an SqlCall argument or a filter template. Spot-checking the classifier against realistic member SQL, the atomic set holds where it matters: window functions (sum(x) OVER (…)), CAST/EXTRACT/CASE, AT TIME ZONE, interval literals and already-parenthesized expressions all keep their shape, so the diff genuinely does not spread into filters that never had the hazard.

The open_line_comment state machine is correct for the property it claims. skip_to_line_end assigns rather than or-assigns, so "the last line comment wins", and since next_token calls skip_trivia before its EOF check, a trailing comment is always consumed before ends_in_line_comment reads the flag. a -- x\n/* y → false, a /* x */ -- y → true, '-- not a comment' → false; all three behave.

The test file is stronger than most SQL-shape tests in this repo — real row-level assertions against inline VALUES data for the AND/OR cases (where the bug is silent, so shape assertions alone would prove nothing), plus two negative guards that the wrapping stays off atomic members. That last pair is the part that keeps this diff from quietly widening later.

Findings

#	Severity	Where	Issue
1	Medium	filter_sql_context.rs:55-66	ends_in_line_comment is checked only on the compound branch, so an atomic member ending in -- … still swallows the template's operator (inline)
2	Low	filter-member-sql-parens.test.ts:146	26× repeated if (!getEnv(…)) return; reports as pass, not skip, under the legacy planner (inline)
3	Low	—	Emitted-SQL blast radius wider than the reported matrix; see below

On #3. is_top_level_compound treats :: as a top-level operator, so a very common real-world member shape — sql: "status::text", sql: "amount::numeric" — now renders as (status::text) = $1 where it previously rendered status::text = $1. Semantically identical, and arguably more correct, but it means the diff changes emitted SQL for a much larger set of models than the reported bug covers. The test matrix in the description covers cubesqlplanner and cubejs-schema-compiler, both of which I would expect to catch this; it does not mention packages/cubejs-testing-drivers, which carries per-driver SQL snapshots. Worth confirming that suite is green in CI before merge rather than reasoning about it — it is the one place a broad shape change like this would surface.

Two smaller notes, neither worth acting on:

• as_operand tokenizes the input twice on the compound path (is_top_level_compound, then ends_in_line_comment). Once per filter per query, on strings the length of a member's SQL — not a real cost, and the clarity of two named predicates is worth more than fusing them.
• The unterminated-/* analogue (sql: "amount + 1 /* note" → (amount + 1 /* note)) is not handled, but unlike a trailing -- that input is not valid SQL standalone either, so the asymmetry is justified. Agreed with leaving parenthesize.rs and convert_tz out of scope; they are the same shape but a separate change.

Legacy planner divergence is called out in the description and matches the repo's stated direction (Tesseract is the default, legacy is deprecated), so the same model producing broken SQL under CUBEJS_TESSERACT_SQL_PLANNER=false seems acceptable — flagging only so the decision is explicit rather than implicit.

The base-query.test.ts change is a genuine improvement, not a concession: the old expectation WHERE (1 = 1 = ?) had encoded SQL that Postgres itself rejects. That exactly one assertion in the repo moved is good evidence the classifier is tight.

Not verified: I did not run cargo test or the Jest suites in this session — the pass/fail counts in the description are taken as reported.

· branch `tesseract-parenthesize-member-sql-in-filters`

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.95%. Comparing base (259188d) to head (c260acd).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11502      +/-   ##
==========================================
- Coverage   83.96%   83.95%   -0.01%     
==========================================
  Files         257      257              
  Lines       80919    80919              
==========================================
- Hits        67940    67935       -5     
- Misses      12979    12984       +5     

Flag	Coverage Δ
cubesql	83.95% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @waralexrom's task in 2m 52s —— View job

---

Re-review of c260acd: both prior findings resolved. 1 low (non-blocking), nothing else outstanding.

Full review

• Gather context (read new commit, current file state)
• Re-check the line-comment finding from the prior review
• Re-check the test-gating finding
• Verify no call site bypasses member_sql()
• Resolve stale threads / post review

What the follow-up commit changed

c260acd addresses both findings from the previous pass, and does it the way the shape of the problem suggests rather than by patching around it.

Finding #1 (medium) — fixed. as_operand now asks the comment question first (filter_sql_context.rs:55-66):

if ends_in_line_comment(member_sql) { return format!("({}\n)", member_sql); }
if !is_top_level_compound(member_sql) { return member_sql.to_string(); }
format!("({})", member_sql)

This is the right ordering, and the comment above it now states the right reason — the wrap is forced by the closing parenthesis needing a line, not by precedence, so atomicity is irrelevant to that branch. The reported case (sql: "amount -- as is" under gt) now renders (amount -- as is\n) > $1 instead of the predicate-swallowing amount -- as is > $1. Covered by a new atomic expression test under a member SQL ending in a line comment describe, with a row-level assertion (idRows(1, 3, 5), matching amount > 50 over the inline data) rather than a shape-only check.

Finding #2 (low) — fixed. The 26 if (!getEnv('nativeSqlPlanner')) return; guards are gone, replaced by a single module-scope read plus (tesseract ? describe : describe.skip). I checked that this is safe here: the planner flag is not mutated by test/global-setup.ts (which only sets TZ and two MySQL/MSSQL timezone vars), so a module-scope getEnv reads the same value a per-test call would. Under the legacy planner the file now reports as skipped, which was the point. Note this is the first describe.skip gate in test/integration/postgres/ — every other file uses the per-test guard — so it diverges from local style, but in the direction that makes the runner output honest.

Call sites. Re-grepped: all 21 reads go through ctx.member_sql(), and the three construction sites in typed_filter.rs all use FilterSqlContext::new. No struct literal remains — and since the field is private, one could not be added from outside the module anyway. FilterSqlContext::new("", …) for the FILTER_PARAMS-callback path still yields "" (both predicates return false on the empty string, and the scanner has an explicit ends_in_line_comment("") test).

Finding

#	Severity	Where	Issue
1	Low	filter_sql_context.rs:55-66	The guard ordering is the load-bearing detail of the fix, and no Rust-level test pins it — only the env-gated Postgres integration suite (inline)

Two notes carried over from the prior pass, neither changed by this commit and neither an objection:

• Emitted-SQL blast radius. is_top_level_compound treats :: as a top-level operator, so common shapes like sql: "status::text" now render (status::text) = $1. Semantically identical, but the set of models whose emitted SQL moves is much wider than the reported bug. Still worth confirming packages/cubejs-testing-drivers (per-driver SQL snapshots) is green in CI rather than reasoning about it — that is the one suite where a broad shape change surfaces.
• Unterminated /*. sql: "amount + 1 /* note" → (amount + 1 /* note), unhandled. Unlike a trailing --, that input is not valid SQL standalone either, so the asymmetry is justified. Same for leaving parenthesize.rs and convert_tz out of scope.

Resolved both of my prior threads, since the current diff addresses them.

Not verified: cargo test and the Jest suites did not run in this session — the sandbox declined the commands. The pass counts in the description, and the CI result for the driver-snapshot suite, are taken as reported.

· branch `tesseract-parenthesize-member-sql-in-filters`

[claude]

The ordering here is now the load-bearing detail of the whole fix — comment question before atomicity question — and nothing in the Rust crate pins it. The only coverage is filter-member-sql-parens.test.ts, which needs a live Postgres and is gated on getEnv('nativeSqlPlanner'), so a future refactor that swapped these two blocks back would compile, pass cargo test, and only surface in the integration suite.

as_operand is a pure &str -> String, so a #[cfg(test)] mod tests in this file is nearly free and states the contract locally:

#[test]
fn operand_wrapping() {
    assert_eq!(FilterSqlContext::as_operand("amount"), "amount");
    assert_eq!(FilterSqlContext::as_operand("amount > 50"), "(amount > 50)");
    // Atomic, but a trailing line comment still forces the wrap.
    assert_eq!(FilterSqlContext::as_operand("amount -- as is"), "(amount -- as is\n)");
    assert_eq!(FilterSqlContext::as_operand(""), "");
}

Non-blocking.

Fix this →