rebenitez1802 opened a new pull request, #43864: URL: https://github.com/apache/superset/pull/43864
### SUMMARY A calculated column's `expression` becomes SQL via `literal_column(expr)` 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. [#39793](https://github.com/apache/superset/pull/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> = value` mis-parsed: ``` state = 'CA' OR state = 'NY' = 1 -> state = 'CA' OR (state = 'NY' = 1) ``` changing which groups the top-N prequery selects and the join membership whenever a boolean/`OR` calculated column is used as a series dimension. **Fix:** 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 (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_parenthesized` guard 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: ``` ... ON state = 'CA' OR state = 'NY' = is_ca_or_ny__ ... ``` After: ``` ... ON (state = 'CA' OR state = 'NY') = is_ca_or_ny__ ... ``` ### 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 legacy `COUNT(DISTINCT ...)`: ``` pytest tests/unit_tests/models/helpers_test.py tests/unit_tests/connectors/sqla/models_test.py ``` ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
