fix(tesseract): parenthesize member SQL spliced into filter templates - #11502
Conversation
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. A measure `sql: "{total} IS NOT NULL"` filtered by
`equals true` rendered `HAVING (sum(...) IS NOT NULL = CAST(? AS BOOLEAN))`,
which Trino and Athena reject. Aggregate-typed measures were safe by accident,
being wrapped in their own function call.
Where the member's top level is `AND`/`OR` the mis-parse stays valid SQL and
silently returns a different row set — in `WHERE` over a dimension as much as in
`HAVING` over a measure.
Render the member as one operand when `FilterSqlContext` is built, so every
operator receives it already pinned, and keep the field private so a new call
site cannot bypass it. Atomicity is decided by the existing expression scanner:
plain columns, aggregates, casts and CASE keep their shape, so the wrapping does
not spread through filters that never had the hazard.
An expression ending in a line comment gets its closing parenthesis on a line of
its own — on the same line the comment would swallow it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
rust
|
Claude finished @waralexrom's task in 3m 54s —— View job Review: sound fix, well-targeted — 1 medium, 2 low. No blocking issues.Full review
What the fix gets rightThe shape is the right one. Rendering the operand once in Reusing The The test file is stronger than most SQL-shape tests in this repo — real row-level assertions against inline Findings
On #3. Two smaller notes, neither worth acting on:
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 The Not verified: I did not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11502 +/- ##
==========================================
- Coverage 83.96% 79.52% -4.44%
==========================================
Files 257 484 +227
Lines 80919 99739 +18820
Branches 0 3672 +3672
==========================================
+ Hits 67940 79317 +11377
- Misses 12979 19874 +6895
- Partials 0 548 +548
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Deciding atomicity first left the line-comment guard unreachable for the case that needs it just as much: `sql: "amount -- note"` has no top-level operator, so it went in bare and the template appended its operator to the commented line — `amount -- note > $1`, where the predicate silently disappears. Ask the comment question first and let it force the wrapping regardless of atomicity, since the reason is the closing parenthesis rather than precedence. Gate the test file on the planner once at the describe level: a bare `return` per test reported as a pass under the legacy planner, so a regression that read the flag as false everywhere would have turned the file green instead of red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 2m 52s —— View job Re-review of
|
| # | 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_compoundtreats::as a top-level operator, so common shapes likesql: "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 confirmingpackages/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 leavingparenthesize.rsandconvert_tzout 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.
`as_operand` decides the comment question before the atomicity one, and that ordering is what keeps a trailing line comment from swallowing the closing parenthesis. Only the Postgres integration suite covered it, so swapping the two blocks back would compile and pass `cargo test`. The function is a pure `&str -> String`, so state the contract next to it: atomic stays bare, compound gets wrapped, and a trailing line comment forces the wrap with the parenthesis on a line of its own however atomic the expression is. Drop the data table from the integration test's header comment. It restated the inline `VALUES` twenty lines below, plus the per-row truth values derived from them — three copies of one fact, and the copy nobody runs is the one that rots. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 2m 1s —— View job Re-review of
|
| // An expression ending in a line comment swallows whatever the template | ||
| // appends on that line, so it needs the wrapping — and a line of its own | ||
| // for the closing parenthesis — however atomic it otherwise is. | ||
| if ends_in_line_comment(member_sql) { |
There was a problem hiding this comment.
@claude could you showcase an example of SQL? Why is this needed?
There was a problem hiding this comment.
There was a problem hiding this comment.
@waralexrom, about what comments is this code about?)
- name: test
sql: "amount + 1 -- this comment?"
type: number
such comments? ☝️
There was a problem hiding this comment.
Claude finished @ovr's task in 1m 1s —— View job
SQL examples
Sure — here's what actually changes on the wire. Every filter template splices the member's sql next to an operator of its own ({{ column }} = {{ value }}, {{ column }} > {{ value }}, …), so the member's own top-level operator ends up competing with the template's for the same operands.
1. The reported case — hard syntax error (Trino/Athena)
- name: total
sql: amount
type: sum
- name: total_is_set
sql: "{total} IS NOT NULL"
type: booleantotal_is_set equals true:
-- before
HAVING (sum("orders".amount) IS NOT NULL = CAST(? AS BOOLEAN))
-- Trino: mismatched input '=' — `IS NOT NULL` binds looser than `=`,
-- so `=` grabs only `NULL`, and `x IS NOT (NULL = ?)` is not a thing.
-- after
HAVING ((sum("orders".amount) IS NOT NULL) = CAST(? AS BOOLEAN))Same shape on Postgres with a comparison member (sql: "amount > 50", equals false) — comparisons are non-associative there, so amount > 50 = $1 is syntax error at or near "=".
2. The reason this is worth fixing beyond the error — silent wrong rows
This is the one I'd point at. With a top-level AND/OR, the mis-parse is valid SQL and just returns a different answer, on every dialect:
- name: big_and_flag
sql: "amount > 50 AND flag"
type: booleanbig_and_flag equals false:
-- before
WHERE (amount > 50 AND flag = CAST($1 AS BOOLEAN))
-- ^ parses as: amount > 50 AND (flag = false)
-- after
WHERE ((amount > 50 AND flag) = CAST($1 AS BOOLEAN))Over the test data (6 rows) that's 1 row before vs. 4 rows after — no error, no warning, just a wrong number in a dashboard. Covered by row-level assertions in filter-member-sql-parens.test.ts:139-181 (equals, notEquals, IN/NOT IN, set, notSet) rather than SQL-shape checks, precisely because a shape check proves nothing when the broken SQL still runs.
The repo already had one such shape encoded in an assertion — base-query.test.ts expected WHERE (1 = 1 = ?), which Postgres rejects; it now reads WHERE ((1 = 1) = ?). That was the only assertion in the whole repo that moved, which is the evidence that the wrapping isn't spreading.
3. Why the separate line-comment branch (this thread)
An expression ending in -- … needs the wrap for a different reason than precedence — the closing ) can't sit on the same line:
- name: amount_commented
sql: "amount -- as is" # atomic! no top-level operator at all
type: numberamount_commented gt 50:
-- before
WHERE (amount -- as is > $1) -- `> $1` is inside the comment; predicate is gone
-- naive wrap on one line
WHERE ((amount -- as is) > $1) -- still swallowed, now the `)` too
-- after
WHERE ((amount -- as is
) > $1)That's why ends_in_line_comment is checked before is_top_level_compound — amount is atomic, so the atomicity check alone would return it bare. Pinned in the crate at filter_sql_context.rs:290-298 and end-to-end at filter-member-sql-parens.test.ts:306-312.
4. What deliberately does not change
"orders".amount = $1 -- plain column: unchanged
sum("orders".amount) > $1 -- aggregate: unchangedAtomic members (plain columns, aggregates, CAST, CASE, already-parenthesized expressions) keep their exact shape, decided by the same is_top_level_compound scanner ParenthesizeSqlNode already uses for SqlCall arguments. Two negative tests guard that (filter-member-sql-parens.test.ts:317-335) so the diff can't quietly widen later.
One caveat worth stating explicitly, from my earlier passes: :: counts as a top-level operator, so a common shape like sql: "status::text" now renders (status::text) = $1. Semantically identical, but it means emitted SQL moves for more models than the reported bug alone — worth confirming packages/cubejs-testing-drivers (per-driver SQL snapshots) is green in CI.
· branch tesseract-parenthesize-member-sql-in-filters
The fix was only exercised from the JS side, behind a native rebuild and an env-gated Postgres suite. Two crate-level layers cover it where it lives. `tests/filter/member_operand.rs` pins the rendered filter SQL per operator over a compound member — equality, IN lists, nullability, comparison, LIKE, date range, and the reported model's calculated boolean in HAVING — plus the two members that must stay bare and the one ending in a line comment. No database, so it runs in every `cargo test`. `tests/integration/filter_member_operand.rs` checks rows instead, because the dangerous form of the mis-parse is valid SQL over a different row set and the emitted text cannot tell it from the intended reading. The AND member returns 5 rows where the mis-parse silently returns none, and the OR member under `notSet` returns the one customer whose expression is NULL where the mis-parse returns the two it degenerates to. Both row sets were derived independently against the seed before the snapshots were accepted. The AND member carries parentheses around its second operand on purpose: without them the mis-parse chains two comparisons, which Postgres rejects outright, and the test would prove the error case the comparison member already covers instead of the silent one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 2m 54s —— View job Re-review of
|
| Test | Correct reading | Mis-parse | Snapshot |
|---|---|---|---|
equals_false_on_and_dimension |
amount > 100 AND (status='completed') false for orders 1,3,5,7,9 → 5 |
amount>100 AND ((status='completed')=false) → big-but-not-completed → none |
5 ✓ |
not_set_on_or_dimension |
city='New York' OR name LIKE 'Alice%' is NULL only for Charlie Brown (city NULL, name no match) → customer 3 |
city='NY' OR ((name LIKE 'Alice%') IS NULL); right side never NULL since name NOT NULL → customers 1, 4 |
3 ✓ |
equals_true_on_comparison_dimension |
amount > 100 → orders 2,4,6,8 → 4 |
Postgres rejects amount > 100 = $1 outright |
4 ✓ |
equals_on_calculated_boolean_measure |
sum(amount) IS NOT NULL true for all statuses |
PG puts IS above comparison, so this one parses the intended way — shape guard only, as the comment says |
cancelled/completed/pending ✓ |
The commit message's note about is_big_completed carrying its own parentheses around (status = 'completed') is the detail that makes the first test earn its keep: without them the mis-parse chains two comparisons and Postgres errors, which would prove the loud failure the third test already covers. Deliberately arranging the fixture so the mis-parse stays valid SQL is the right instinct — that's the case where a text snapshot proves nothing.
One thing I checked because it's easy to get wrong: the shared common/integration_basic.yaml gained three members and is read by 11 other integration modules. Nothing else in the crate references is_ny_or_alice, is_big_completed, is_big, or total_amount_is_set, and no test in that directory enumerates a cube's members, so no existing snapshot moves. MockSchema::from_yaml_file reads by path, so the new filter_operand.yaml needs no registration.
Findings
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Low | filter_operand.yaml:32 |
The big dimension is unused — orders.big[^_] matches nothing in the crate, so the "Top-level comparison" comment describes a case only the measure-side test covers (inline) |
No prior thread of mine is still open — all three were resolved on earlier passes and the current diff keeps them addressed. Two notes carried forward, unchanged and neither an objection:
- Emitted-SQL blast radius.
is_top_level_compoundtreats::as a top-level operator, sosql: "status::text"now renders(status::text) = $1. Semantically identical, but wider than the reported bug. Still worth confirmingpackages/cubejs-testing-drivers(per-driver SQL snapshots) is green in CI rather than reasoning about it. - Unterminated
/*.sql: "amount + 1 /* note"→(amount + 1 /* note), unhandled — that input is not valid SQL standalone either, so the asymmetry is justified. Same for leavingparenthesize.rsandconvert_tzout of scope.
Also worth stating: the JS suite (filter-member-sql-parens.test.ts, 26 tests) now overlaps the crate tests substantially. That's defensible — it's the only layer that exercises the JS→native boundary — but if it starts costing CI time, the crate tests are the ones that should stay.
Not verified: cargo test -p cubesqlplanner did not run in this session — the sandbox declined the command. Every expectation above was traced by hand against the fixture, the seed data, and the template output; the pass counts in the description are taken as reported.
| # Top-level comparison — a second one next to it is a syntax error on | ||
| # Postgres and Trino alike. | ||
| - name: big |
There was a problem hiding this comment.
Nit: big is not referenced by any test — grepping orders.big[^_] across the crate returns nothing, so this fixture member and its comment describe a case the crate does not actually exercise. The comparison shape is covered, but only on the measure side (total_over_150, test_equals_comparison_measure) and in the integration fixture (orders.is_big).
Either drop it, or add the two-line dimension counterpart so the comment stays true:
#[test]
fn test_equals_comparison_dimension() {
let result = build(indoc! {"
filters:
- dimension: orders.big
operator: equals
values:
- \"true\"
"});
assert_filter(&result, r#"(("orders".amount > 50) = $_0_$::boolean)"#, &["true"]);
}Not blocking.
3 participants
Summary
A filter template places its own operator next to the member's rendered SQL (
{{ column }} = {{ value }},{{ column }} IS NOT NULL, …). A member whosesqlis 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):
measure_2 equals truerenderedHAVING (sum("orders".amount) IS NOT NULL = CAST(? AS BOOLEAN)), which Trino rejects withmismatched 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 inWHERE), and where the member's top level isAND/ORthe 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
FilterSqlContextrenders the member as a single operand at construction time, so every filter operator receives it already pinned. The field is now private behindmember_sql()so a new call site cannot bypass the wrapping.sql_expression_scanner::is_top_level_compound— the same scannerParenthesizeSqlNodealready uses forSqlCallarguments. Plain columns, aggregates, casts,CASEand 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.tsrequiredWHERE (1 = 1 = ?), i.e. it had encoded invalid SQL.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 > $1does 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 = cis a syntax error there (comparisons are non-associative), so a membersql: "amount > 50"underequals/notEqualshard-fails — inWHERE, and inHAVINGoversum(...). Closest analogue of the reported Trino error.AND/ORdiverges silently: row-level assertions cover equals / notEquals / IN / NOT IN / set / notSet against real data.x IS NOT NULL = FALSEhappens 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.filter-member-sql-parens(Tesseract)getEnv('nativeSqlPlanner')dist/test/integration/postgres(Tesseract)dist/test/unit(Tesseract)cargo test -p cubesqlplannercargo test -p cubesqlplanner --features integration-postgresNote for reviewers
The same trailing-line-comment shape exists in
sql_nodes/parenthesize.rsandsql_templates/plan.rs::convert_tz. Left untouched to keep this diff scoped — happy to fold in if preferred.