Repository navigation
fix(explore): omit constant Custom SQL dimensions from GROUP BY - #45271
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45271 +/- ##
=======================================
Coverage 84.12% 84.13%
=======================================
Files 3056 3056
Lines 201418 201438 +20
Branches 47095 47098 +3
=======================================
+ Hits 169441 169471 +30
+ Misses 28729 28718 -11
- Partials 3248 3249 +1
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
| def test_all_constant_keys_with_a_metric_keep_group_by(table: SqlaTable) -> None: | ||
| select, sql = _compile(table, columns=[_adhoc("'Other'")]) | ||
| assert _group_keys(select) == ["'Other'"], sql |
There was a problem hiding this comment.
Suggestion: _compile only checks generated SQL, so this assertion passes with GROUP BY 'Other', which PostgreSQL rejects; charts with only a constant dimension still fail.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Incomplete implementation
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/models/groupby_literal_column_test.py
**Line:** 186:186
**Comment:**
*Incomplete Implementation: `_compile` only checks generated SQL, so this assertion passes with `GROUP BY 'Other'`, which PostgreSQL rejects; charts with only a constant dimension still fail.
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 fixThere was a problem hiding this comment.
Keeping the current approach: when every GROUP BY key is constant the keys are left unchanged on purpose, because omitting GROUP BY is only correct when the query aggregates, which is not known at that point (disclosed in the PR description). The tests compile the generated SQL for the PostgreSQL dialect like the neighbouring unit tests; a live PostgreSQL run is outside the unit-test layer.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not require omitting constant GROUP BY keys when aggregate context is unavailable; omitting GROUP BY is only correct when the query is known to aggregate.
Applied to:
tests/unit_tests/models/groupby_literal_column_test.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
CodeAnt PR Risk: Medium Risk
Assessed commit: |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Careful fix, the fails-closed is_constant_expression (including the SQLite TRUE/FALSE-as-identifier edge case) and restricting detection to raw literal_column/ColumnClause constructs rather than compiling arbitrary expressions are the right calls. Good test coverage compiling real SQL through sqlglot rather than just checking the Python-side expression tree.
CodeAnt's "every key is constant" point is the same limitation already disclosed in the PR body (GROUP BY is left alone there since dropping it depends on whether the query aggregates, which isn't known at that point) — not a new gap. LGTM!
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Read through this one and the approach holds up. Filtering at the four GROUP BY sites through one helper, and only looking at raw literal_column text, keeps the blast radius small.
Left two notes inline: a series-limit case where the chart still fails even though the main query has a real key, and negative numeric literals slipping past is_constant_expression. I didn't repeat the all-constant-keys limitation since it's disclosed and already discussed above. The red check-openapi-spec-drift check looks unrelated, nothing here touches the API.
| inner_time_filter = [_inner_filter] | ||
| subq = subq.where(and_(*(where_clause_and + inner_time_filter))) | ||
| subq = subq.group_by(*inner_groupby_exprs) | ||
| subq = subq.group_by(*self._groupby_clause_exprs(inner_groupby_exprs)) |
There was a problem hiding this comment.
The all-constant fallback bites here in a case that isn't the disclosed one. A line chart with a time x-axis, one dimension 'Other' and a series limit gets a fixed outer query (GROUP BY ds), but the inner subquery only groups by series columns, so it's back to GROUP BY 'Other' and Postgres still rejects the chart.
When every series column is constant there's exactly one series, so the limit can't cut anything. Could we skip the series-limit subquery (and the Others rebuild) in that case? A test with a non-constant x-axis plus a single constant series column would pin it.
There was a problem hiding this comment.
Thanks, this is a real gap and I looked at it closely. I prototyped skipping the series-limit pass when every series column is constant. It works for the plain case, but review kept finding queries where the result differs from today's behaviour: a separate selection window (including timezone-aware bounds that compare equal as instants but render to different SQL), a NULL series (NULL = NULL never joins), a time filter consumed by a dataset template, non-deterministic filters evaluated separately in the outer query and the selection, a templated constant that renders differently in the no-join prequery path, and an empty-string series on Oracle. Closing those needs more than a local condition, so I've left the series-limit path unchanged in this PR; the constants-only limitation stays documented in the description. Happy to take it on in a follow-up PR if you think it is worth pursuing.
| node = parsed.expressions[0].unnest() | ||
| if isinstance(node, exp.Boolean): | ||
| return dialect != Dialects.SQLITE | ||
| return isinstance(node, (exp.Literal, exp.Null)) |
There was a problem hiding this comment.
sqlglot parses -1 and -1.5 as Neg(Literal), so they come back non-constant and stay in GROUP BY. Postgres folds the minus into the constant, so a -1 sentinel dimension still fails (GROUP BY position -1 is not in select list), and -1.5 hits the same non-integer constant error.
is_number already treats a negated numeric literal as a number:
| return isinstance(node, (exp.Literal, exp.Null)) | |
| return node.is_number or isinstance(node, (exp.Literal, exp.Null)) |
Worth adding -1 and (-1.5) to the parametrized cases.
There was a problem hiding this comment.
Fixed in ef2a16b. is_constant_expression now returns node.is_number or isinstance(node, (exp.Literal, exp.Null)), so negated numeric literals such as -1 and (-1.5) are treated as constants and omitted from GROUP BY. Added -1, (-1.5) and SQLite -1 as constant cases and -region, -(1 + 1), - 'a' and -TRUE as non-constant cases to test_is_constant_expression, plus -1/(-1.5) to the model-level constant-dimension test.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
A Custom SQL dimension holding a bare string literal is emitted verbatim as a GROUP BY key, which PostgreSQL rejects with "non-integer constant in GROUP BY". Pin the compiled PostgreSQL GROUP BY so the constant is not a group key while the labelled column stays in SELECT. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
A Custom SQL dimension that is a bare constant (e.g. 'Other') was emitted
verbatim as a GROUP BY key. PostgreSQL rejects a non-integer constant there
("non-integer constant in GROUP BY"), and an integer constant is read as a
select-list ordinal.
Omit provably-constant keys (bare string/numeric/boolean literal or NULL,
parentheses stripped) from the GROUP BY of the main query, the series-limit
inner subquery and the "Others" rebuilds; they stay in SELECT under their
label. If every key is constant and the query aggregates, GROUP BY is
omitted; without an aggregate the keys are left unchanged. Expressions that
merely contain a constant (CASE, CAST, concatenation) remain keys.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… as identifiers on SQLite Only omit constant GROUP BY keys while a non-constant key remains. When every key is constant the keys are left unchanged, since whether GROUP BY can be dropped depends on the query aggregating, which is not known there. On SQLite TRUE and FALSE are identifiers that resolve to a same-named column, so they are not bare constants: dropping (TRUE) as a key would merge groups. Boolean literals stay constants on other engines. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
sqlglot parses a negated number such as `-1` as Neg(Literal), so it was not recognized as a bare constant and stayed a GROUP BY key. PostgreSQL rejects `GROUP BY -1` and `GROUP BY -1.5`, so treat a negated number as a constant. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ef2a16b to
0e34933
Compare
Two conflicts, both additive: - superset/datasets/schemas.py: keep FilterOperator (partition mapping preview) and master's is_valid_d3_format import (#45633). - superset/models/helpers.py: the series-limit subquery keeps the branch's inner-window partition mirrors in its WHERE and takes master's _groupby_clause_exprs for its GROUP BY (#45271). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SUMMARY
A chart dimension or X-axis defined as a Custom SQL constant (for example the string literal
'Other') is emitted as aGROUP BYkey as well as aSELECTcolumn. PostgreSQL and related engines reject a bare non-integer constant there, so the chart fails withnon-integer constant in GROUP BY; an integer constant is silently read as a select-list ordinal instead. The same literal works in theSELECTlist, and an expression such asCASE WHEN 1=1 THEN 'x' ENDhappens to work because it is not a bare constant.For example, a Bar Chart with X-axis Custom SQL
'Other', metricSUM(amount)and dimensionregionproduced:Root Cause
ExploreMixin.get_sqla_query(superset/models/helpers.py) turns every selected groupby expression into aGROUP BYkey, unconditionally. TheLabelwrapper added for the SELECT column does not change how the key is rendered inGROUP BY, so a bare constant is emitted verbatim. Grouping by a constant never changes the groups, so omitting it is result-preserving whenever another key remains. The pattern is the same for Custom SQL, calculated columns whose expression is a literal, raw-string groupby entries, and the series-limit inner subquery; there is no otherGROUP BYemitter for chart queries.Reproduced on PostgreSQL 16.2:
'Other',('Other'),1.5andNULLas aGROUP BYkey all fail, whileCASE …,CAST('Other' AS VARCHAR)and aliases succeed.The fix
superset/sql/parse.py: newis_constant_expression()next tohas_aggregate(). It isTrueonly for a single bare string/numeric literal,NULL, or boolean literal (parentheses stripped); anything that merely contains a constant (CASE,CAST, function calls,'a' || col) is not constant, and a parse error fails closed.TRUE/FALSEare not treated as constants on SQLite, where they resolve to a same-named column.superset/models/helpers.py: one helper,ExploreMixin._groupby_clause_exprs, filters provably-constant keys out of theGROUP BYat the four emission sites (main query, series-limit inner subquery, and the two "Others" rebuilds). The constant stays in theSELECTlist under its label. Queries without a constant key are unchanged.GROUP BY(Oracle forbids ordinals, and PostgreSQL matches metric expressions structurally), wrapping the literal inCAST(needs a per-dialect type and MSSQL still rejects it), and an engine-spec flag (a per-dialect list that cannot be verified).GROUP BYthere is only correct when the query aggregates. TheGROUPING SETSbranch is untouched.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: backend SQL generation change with no frontend diff. The regression tests below show the generated SQL before and after.
TESTING INSTRUCTIONS
pytest tests/unit_tests/models/groupby_literal_column_test.py tests/unit_tests/sql/parse_tests.py -k "constant or literal or group_by or is_constant"The new tests compile the query for the PostgreSQL dialect and assert no bare constant remains in
GROUP BYfor Custom SQL, calculated-column, raw-string and series-limit shapes; assertCASE/CAST/function expressions stay as keys; and run the generated SQL on in-memory SQLite to check that aTRUEcolumn still groups correctly. Before the change the primary test fails withbare constant in GROUP BY: ["'Other'"].Manual check on PostgreSQL: create a Bar Chart with X-axis > Custom SQL
'Other', a metric, and a dimension, then run it. It previously failed withnon-integer constant in GROUP BY.ADDITIONAL INFORMATION