You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rebenitez1802 (via GitHub)" <gi...@apache.org> on 2026/09/04 13:22:13 UTC
[PR] fix(sqla): parenthesize calculated column expressions in SELECT/GROUP BY/ORDER BY and series-limit queries [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sqla): parenthesize calculated column expressions in SELECT/GROUP BY/ORDER BY and series-limit queries [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #43864:
URL: https://github.com/apache/superset/pull/43864
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sqla): parenthesize calculated column expressions in SELECT/GROUP BY/ORDER BY and series-limit queries [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43864:
URL: https://github.com/apache/superset/pull/43864#discussion_r3934431774
##########
superset/connectors/sqla/models.py:
##########
@@ -1234,7 +1234,10 @@ def get_sqla_col(
self.table.schema if self.table else None,
)
expression = self._validate_stored_expression(expression)
- col = literal_column(expression, type_=type_)
+ # 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_))
Review 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`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7636e6fc6f364fa68278475dfd0760c6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7636e6fc6f364fa68278475dfd0760c6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43864&comment_hash=406cbe9bea3bfda708df346d9fdcce857b38214a259c79d614af65abcd31e89b&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43864&comment_hash=406cbe9bea3bfda708df346d9fdcce857b38214a259c79d614af65abcd31e89b&reaction=dislike'>๐</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sqla): parenthesize calculated column expressions in SELECT/GROUP BY/ORDER BY and series-limit queries [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43864:
URL: https://github.com/apache/superset/pull/43864#issuecomment-5541972562
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43864?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `55.55556%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.22%. Comparing base ([`8d0637e`](https://app.codecov.io/gh/apache/superset/commit/8d0637efadf1e2e72547744ac3a6087322fb53d6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`acc2573`](https://app.codecov.io/gh/apache/superset/commit/acc2573788df1288ee7d0486ad272e37764d4dcd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 11 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43864?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/models/helpers.py](https://app.codecov.io/gh/apache/superset/pull/43864?src=pr&el=tree&filepath=superset%2Fmodels%2Fhelpers.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | 42.85% | [3 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/43864?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (8d0637e) and HEAD (acc2573). Click for more details.
>
> <details><summary>HEAD has 148 uploads less than BASE</summary>
>
>| Flag | BASE (8d0637e) | HEAD (acc2573) |
>|------|------|------|
>|python|103|3|
>|presto|17|1|
>|hive|17|1|
>|sqlite|17|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43864 +/- ##
===========================================
- Coverage 79.42% 66.22% -13.21%
===========================================
Files 2895 2895
Lines 167997 168002 +5
Branches 38903 38903
===========================================
- Hits 133434 111260 -22174
- Misses 32064 54469 +22405
+ Partials 2499 2273 -226
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43864/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/43864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.77% <22.22%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/43864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/43864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.66% <33.33%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.34% <55.55%> (-26.54%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.21% <55.55%> (-0.01%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43864?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sqla): parenthesize calculated column expressions in SELECT/GROUP BY/ORDER BY and series-limit queries [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43864:
URL: https://github.com/apache/superset/pull/43864#issuecomment-5541050096
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sqla): parenthesize calculated column expressions in SELECT/GROUP BY/ORDER BY and series-limit queries [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43864:
URL: https://github.com/apache/superset/pull/43864#issuecomment-5543489977
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sqla): parenthesize calculated column expressions in SELECT/GROUP BY/ORDER BY and series-limit queries [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43864:
URL: https://github.com/apache/superset/pull/43864#issuecomment-5541117800
<!-- Bito Reply -->
The flagged issue is correct. The `get_timestamp_expression` method in `superset/connectors/sqla/models.py` currently bypasses the `Grouping` wrapper applied to other calculated columns, which can lead to operator precedence bugs in time-series queries. To resolve this, you should wrap the expression returned by `get_timestamp_expression` in a `Grouping` object, similar to how other calculated columns are handled in the PR.
Would you like me to implement this fix for `get_timestamp_expression`? I can also check the rest of the PR comments if you would like to address other issues as well.
**superset/connectors/sqla/models.py**
```
from sqlalchemy.sql.elements import ColumnClause, Grouping, TextClause
# ... inside get_timestamp_expression ...
return Grouping(literal_column(expression, type_=type_))
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sqla): parenthesize calculated column expressions in SELECT/GROUP BY/ORDER BY and series-limit queries [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43864:
URL: https://github.com/apache/superset/pull/43864#issuecomment-5541071820
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | 793a47c4fc88f8ecbfca63b39d917db289ae4be3 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a9ac605019b330008246380 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-43864--superset-docs-preview.netlify.app](https://deploy-preview-43864--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcvBDcMgDADAXfwuihNaRLpBxzDgKEgUkOM8mqq7l9997gtd2rsrPAFuEIRq3IeFA9esfM0el6mTcNWdj3yxiVTiWUg5mSZmy0VZjnET99I-rzS2o5WiwwfOa7AWEf1yd9Yj_P4) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43864--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org