fix(tesseract): parenthesize member SQL spliced into filter templates - #11502
fix(tesseract): parenthesize member SQL spliced into filter templates#11502waralexrom wants to merge 1 commit into
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}
| fn as_operand(member_sql: &str) -> String { | ||
| if !is_top_level_compound(member_sql) { | ||
| return member_sql.to_string(); | ||
| } | ||
| // An expression ending in a line comment would swallow the closing | ||
| // parenthesis, so that one gets a line of its own. | ||
| if ends_in_line_comment(member_sql) { | ||
| format!("({}\n)", member_sql) | ||
| } else { | ||
| format!("({})", member_sql) | ||
| } | ||
| } |
There was a problem hiding this comment.
The line-comment guard is unreachable for the atomic case, which is where it is also needed. A member whose SQL is atomic and ends in a line comment returns early on line 57 and the template's operator still lands on the commented line:
- name: amount_commented
sql: "amount -- note" # atomic: no top-level operator
type: numbergt renders amount -- note > $1, and everything after -- is swallowed — the predicate silently disappears (or the statement fails to parse, depending on what follows on that line). Same for sql: "sum(amount) -- note" on a measure. This is pre-existing rather than introduced here, but the PR already carries ends_in_line_comment, and closing it is a reorder: decide the comment question first and let it force the wrap.
| fn as_operand(member_sql: &str) -> String { | |
| if !is_top_level_compound(member_sql) { | |
| return member_sql.to_string(); | |
| } | |
| // An expression ending in a line comment would swallow the closing | |
| // parenthesis, so that one gets a line of its own. | |
| if ends_in_line_comment(member_sql) { | |
| format!("({}\n)", member_sql) | |
| } else { | |
| format!("({})", member_sql) | |
| } | |
| } | |
| fn as_operand(member_sql: &str) -> String { | |
| // An expression ending in a line comment would swallow whatever the | |
| // template appends on that line, so it needs the wrapping — and its | |
| // closing parenthesis on a line of its own — even when atomic. | |
| 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) | |
| } |
|
|
||
| describe('WHERE — dimension whose SQL is a top-level AND/OR', () => { | ||
| it('equals', async () => { | ||
| if (!getEnv('nativeSqlPlanner')) return; |
There was a problem hiding this comment.
Nit: the if (!getEnv('nativeSqlPlanner')) return; guard is repeated 26 times, and a bare return reports as a pass under the legacy planner rather than a skip — so a regression that made getEnv('nativeSqlPlanner') read false everywhere would turn this whole file green instead of red. Reading the flag once outside describe and gating with describe.skip makes the intent visible in the runner output and removes the repetition:
const tesseract = getEnv('nativeSqlPlanner');
(tesseract ? describe : describe.skip)('Filter member SQL parenthesization', () => { … });Not blocking — the per-test guard is the prevailing style in this directory.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11502 +/- ##
==========================================
- Coverage 83.96% 79.46% -4.50%
==========================================
Files 257 480 +223
Lines 80919 98861 +17942
Branches 0 3636 +3636
==========================================
+ Hits 67940 78564 +10624
- Misses 12979 19777 +6798
- Partials 0 520 +520
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:
|
1 participant
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.