fix(sqla): parenthesize calculated column expressions in SELECT/GROUP BY/ORDER BY and series-limit queries - #43864
Conversation
Code Review Agent Run #3bac2bActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
793a47c to
41f67dd
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| # Parenthesize calculated-column expressions so a bare boolean | ||
| # operator (e.g. OR) inside the expression cannot leak into the | ||
| # surrounding operator precedence (e.g. COUNT(DISTINCT ...)). | ||
| col = Grouping(literal_column(expression, type_=type_)) |
There was a problem hiding this comment.
Suggestion: get_timestamp_expression still emits calculated temporal expressions without this grouping, so time-series queries can retain the same boolean precedence bug. [incomplete implementation]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 1240:1240
**Comment:**
*Incomplete Implementation: `get_timestamp_expression` still emits calculated temporal expressions without this grouping, so time-series queries can retain the same boolean precedence bug.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The Would you like me to implement this fix for superset/connectors/sqla/models.py |
41f67dd to
acc2573
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43864 +/- ##
==========================================
- Coverage 79.45% 79.43% -0.03%
==========================================
Files 2895 2895
Lines 168167 167990 -177
Branches 38995 38895 -100
==========================================
- Hits 133624 133442 -182
- Misses 32044 32051 +7
+ Partials 2499 2497 -2
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:
|
… BY/ORDER BY and series-limit queries A calculated column's expression is turned into SQL via literal_column() with no parentheses. When the expression contains a low-precedence boolean operator (e.g. `state = 'CA' OR state = 'NY'`), the bare OR leaks into the surrounding operator's precedence. PR #39793 fixed this for WHERE/HAVING filters only; the same column used as a dimension / GROUP BY / ORDER BY / metric, and -- the real correctness bug -- the series-limit (top-N) prequery predicate and JOIN ON, were still emitted unparenthesized, so `<calc> = value` mis-parsed as `state = 'CA' OR (state = 'NY' = value)`, changing top-N group membership and join predicates. Wrap the expression branch of both column converters (ExploreMixin.convert_tbl_column_to_sqla_col and TableColumn.get_sqla_col) in Grouping(...); every downstream clause is built from the converter output, so one wrap at the source parenthesizes SELECT, GROUP BY, ORDER BY, both _get_top_groups, the JOIN ON, and COUNT(DISTINCT ...) at once. Physical columns are left untouched. Narrow the existing WHERE/HAVING filter wrap to adhoc expressions only (registered calculated columns are now parenthesized by the converter) and add an _is_parenthesized guard so converter-wrapped columns are not double-wrapped. Adhoc SQL-expression columns referenced by label are parenthesized too, matching inline adhoc columns. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
acc2573 to
f61a60c
Compare
Code Review Agent Run #79727aActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
A calculated column's
expressionbecomes SQL vialiteral_column(expr)with no parentheses. When the expression contains a low-precedence boolean operator (e.g.state = 'CA' OR state = 'NY'), the bareORleaks into the surrounding operator's precedence.#39793 fixed this for WHERE/HAVING filters only. The same column used as a dimension / GROUP BY / ORDER BY / metric was still emitted unparenthesized, and — the real correctness bug — so were the series-limit (top-N) prequery predicate and the series-limit JOIN ON, where
<calc col> = valuemis-parsed:changing which groups the top-N prequery selects and the join membership whenever a boolean/
ORcalculated column is used as a series dimension.Fix: wrap the expression branch of both column converters (
ExploreMixin.convert_tbl_column_to_sqla_colandTableColumn.get_sqla_col) inGrouping(...). Every downstream clause (SELECT, GROUP BY, ORDER BY, both_get_top_groups, the JOIN ON,COUNT(DISTINCT ...)) is built from the converter output, so one wrap at the source parenthesizes them all. Physical (non-expression) columns are left untouched.The existing WHERE/HAVING filter wrap is narrowed to adhoc expressions only (registered calculated columns are now parenthesized by the converter) with an
_is_parenthesizedguard that prevents a redundant((...))double-wrap for adhoc columns that reference a saved calculated column. Adhoc SQL-expression columns referenced by label are now parenthesized too, matching inline adhoc columns. SQL Lab virtual datasets (Query) inherit the same converter and are covered.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend query generation). Example — a boolean calculated column
is_ca_or_ny = state = 'CA' OR state = 'NY'used as a series (top-N) dimension:Before:
After:
TESTING INSTRUCTIONS
New unit tests cover SELECT / GROUP BY / ORDER BY (alias variants), the series-limit JOIN ON and top-N prequery predicate (correctness regressions that fail without this change), adhoc calc-column-reference filters, adhoc-by-label filters, SQL Lab
Query, and legacyCOUNT(DISTINCT ...):ADDITIONAL INFORMATION