[pull] master from cube-js:master - #682
Merged
Merged
Conversation
…#11502) * fix(tesseract): parenthesize member SQL spliced into filter templates 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> * fix(tesseract): wrap a filter member whose SQL ends in a line comment 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> * test(tesseract): pin the filter operand contract in the crate `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> * test(tesseract): cover the filter member operand in the crate 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )