codeant-ai-for-open-source[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3557779929
##########
superset/common/query_context_processor.py:
##########
@@ -252,9 +254,54 @@ def get_query_result(self, query_object: QueryObject) ->
QueryResult:
This method delegates to the datasource's get_query_result method,
which handles query execution, normalization, time offsets, and
post-processing.
+
+ When the query requests rollup ``grouping_sets`` but the engine does
not
+ support native ``GROUPING SETS``, fall back to one query per level and
+ concatenate the results with ``GROUPING()``-equivalent markers, so the
+ combined result matches the shape the native path produces (SIP.md,
+ phase 3b). Engines that support it run the single native query.
"""
+ if query_object.grouping_sets and not self._supports_grouping_sets():
+ return self._grouping_sets_fallback(query_object)
return self._qc_datasource.get_query_result(query_object)
+ def _supports_grouping_sets(self) -> bool:
+ engine_spec = getattr(self._qc_datasource, "db_engine_spec", None)
+ return bool(engine_spec and engine_spec.supports_grouping_sets)
+
+ def _grouping_sets_fallback(self, query_object: QueryObject) ->
QueryResult:
+ """
+ Emulate a GROUPING SETS query on engines without native support: run
one
+ query per rollup level and concatenate, tagging each level's rows with
+ the same per-column markers the native path emits.
+ """
+ levels = query_object.grouping_sets
+ all_labels = get_column_names_from_columns(query_object.columns)
+ label_to_column = dict(zip(all_labels, query_object.columns,
strict=False))
Review Comment:
**Suggestion:** The fallback builds `label_to_column` by zipping a filtered
label list with the unfiltered `query_object.columns` list, which misaligns
entries whenever any non-physical/adhoc column is present. That can map a label
to the wrong column object and generate incorrect grouping SQL. Build the
mapping from the original columns and their actual labels in the same pass
(without dropping entries first) so each label always points to its correct
column definition. [incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Grouping_sets fallback groups by wrong columns on SQLite.
- ⚠️ Pivot/Table non-additive totals incorrect on fallback engines.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Build a Chart Data payload similar to
`TestGroupingSetsRollup.test_grouping_sets_returns_all_levels` in
`tests/integration_tests/non_additive_totals_tests.py:12-32`, but set
`queries[0]["columns"]` to include an adhoc SQL column dict first and a
physical column
second (e.g. `columns = [{"expressionType": "SQL", "sqlExpression": "...",
"label":
"adhoc_col"}, "state"]`), and define `grouping_sets` levels that reference
both labels
(e.g. `[["adhoc_col", "state"], ["adhoc_col"], []]`).
2. Send this payload to `ChartDataQueryContextSchema().load(...)` (see
`superset/charts/schemas.py:32-42` and
`tests/integration_tests/non_additive_totals_tests.py:81-84`), which
constructs a
`QueryContext` and its first `QueryObject` via `QueryObjectFactory.create`
(`superset/common/query_object_factory.py:52-95`). The resulting
`QueryObject.columns`
list preserves the adhoc + physical ordering
(`superset/common/query_object.py:114-162`).
3. Run the query against a datasource whose
`db_engine_spec.supports_grouping_sets` is
false (e.g. SQLite, referenced in the docstring at
`tests/integration_tests/non_additive_totals_tests.py:4-10`).
`QueryContext.get_query_result()`
(`superset/common/query_context.py:134-135`) delegates
to `QueryContextProcessor.get_query_result()`
(`superset/common/query_context_processor.py:250-266`), which detects
`query_object.grouping_sets` and calls `_grouping_sets_fallback()` when
native grouping
sets are unsupported.
4. In `_grouping_sets_fallback()`
(`superset/common/query_context_processor.py:272-281`),
`get_column_names_from_columns(query_object.columns)` from
`superset/utils/core.py:1699-1707` drops adhoc columns, producing a shorter
`all_labels`
list. `dict(zip(all_labels, query_object.columns, strict=False))` then pairs
each physical
label with the first N entries of the full `columns` list, so when an adhoc
appears before
a later physical column the label maps to the wrong `Column` object.
Subsequent per-level
subqueries built from `label_to_column` group by mis-mapped columns,
yielding rollup
results and grouping markers that no longer correspond to the intended
dimensions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b5b0140dab1c4ec2a832d9b48829d409&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=b5b0140dab1c4ec2a832d9b48829d409&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/query_context_processor.py
**Line:** 279:280
**Comment:**
*Incorrect Variable Usage: The fallback builds `label_to_column` by
zipping a filtered label list with the unfiltered `query_object.columns` list,
which misaligns entries whenever any non-physical/adhoc column is present. That
can map a label to the wrong column object and generate incorrect grouping SQL.
Build the mapping from the original columns and their actual labels in the same
pass (without dropping entries first) so each label always points to its
correct column definition.
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%2F41184&comment_hash=27e913088b389ae6e646cd1e83646c7f18305447e930343c9fba97c4efcccbfb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=27e913088b389ae6e646cd1e83646c7f18305447e930343c9fba97c4efcccbfb&reaction=dislike'>👎</a>
##########
superset/common/query_context_processor.py:
##########
@@ -252,9 +254,54 @@ def get_query_result(self, query_object: QueryObject) ->
QueryResult:
This method delegates to the datasource's get_query_result method,
which handles query execution, normalization, time offsets, and
post-processing.
+
+ When the query requests rollup ``grouping_sets`` but the engine does
not
+ support native ``GROUPING SETS``, fall back to one query per level and
+ concatenate the results with ``GROUPING()``-equivalent markers, so the
+ combined result matches the shape the native path produces (SIP.md,
+ phase 3b). Engines that support it run the single native query.
"""
+ if query_object.grouping_sets and not self._supports_grouping_sets():
+ return self._grouping_sets_fallback(query_object)
return self._qc_datasource.get_query_result(query_object)
+ def _supports_grouping_sets(self) -> bool:
+ engine_spec = getattr(self._qc_datasource, "db_engine_spec", None)
+ return bool(engine_spec and engine_spec.supports_grouping_sets)
+
+ def _grouping_sets_fallback(self, query_object: QueryObject) ->
QueryResult:
+ """
+ Emulate a GROUPING SETS query on engines without native support: run
one
+ query per rollup level and concatenate, tagging each level's rows with
+ the same per-column markers the native path emits.
+ """
+ levels = query_object.grouping_sets
+ all_labels = get_column_names_from_columns(query_object.columns)
+ label_to_column = dict(zip(all_labels, query_object.columns,
strict=False))
+
+ frames: list[pd.DataFrame] = []
+ result: QueryResult | None = None
+ for level in levels:
+ level_labels = set(level)
+ sub_query = copy.copy(query_object)
+ sub_query.grouping_sets = []
+ sub_query.columns = [
+ label_to_column[label] for label in all_labels if label in
level_labels
+ ]
Review Comment:
**Suggestion:** This fallback only keeps columns found in `all_labels`, but
`all_labels` is built from physical columns only; adhoc groupby columns
(including pivot temporal BASE_AXIS columns sent as adhoc SQL columns) are
dropped from every fallback subquery. On engines without native grouping sets,
those dimensions are silently omitted, so subtotals/grand totals are computed
at the wrong granularity. Include adhoc-labeled groupby columns when selecting
each level so fallback results match native behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Adhoc groupby dimensions dropped from fallback rollup queries.
- ⚠️ Totals computed at coarser-than-intended granularity levels.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a chart payload for the `api/v1/chart/data` endpoint that uses
`grouping_sets` with both physical and adhoc groupby dimensions, e.g. for a
pivot-table-like query: `columns = [{"expressionType": "SQL",
"sqlExpression":
"DATE_TRUNC('month', dttm)", "label": "month"}, "state"]` and `grouping_sets
= [["month",
"state"], ["month"], []]`, following the grouping-sets schema documented in
`superset/charts/schemas.py:32-42`.
2. Load this payload via `ChartDataQueryContextSchema().load(payload)`
(patterned after
`_result_df` in
`tests/integration_tests/non_additive_totals_tests.py:78-85`), which
creates a `QueryContext` and `QueryObject`. The `QueryObject.columns` list
contains both
the adhoc SQL column and the physical `state` column
(`superset/common/query_object.py:114-162`).
3. Execute the query on a database engine where
`db_engine_spec.supports_grouping_sets` is
false (e.g. SQLite, noted in
`tests/integration_tests/non_additive_totals_tests.py:4-10`).
`QueryContext.get_query_result()`
(`superset/common/query_context.py:134-135`) invokes
`QueryContextProcessor.get_query_result()`
(`superset/common/query_context_processor.py:250-266`), which detects
`grouping_sets` and
routes to `_grouping_sets_fallback()` for non-supporting engines.
4. In `_grouping_sets_fallback()`
(`superset/common/query_context_processor.py:272-290`),
`all_labels = get_column_names_from_columns(query_object.columns)` calls the
helper
defined in `superset/utils/core.py:1699-1707`, which expressly ignores adhoc
columns. The
per-level column selection `sub_query.columns = [label_to_column[label] for
label in
all_labels if label in level_labels]` therefore only ever includes physical
labels from
`all_labels`: adhoc groupby dimensions present in
`query_object.grouping_sets` are never
added to `sub_query.columns`. On fallback engines, rollup queries run at a
reduced
grouping granularity (e.g. grouped only by `state` instead of `[month,
state]`), so
subtotals and grand totals for non-additive metrics are computed over the
wrong dimension
set compared to the native GROUPING SETS path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=33eb2f9e225647bbbcf800dafc731b9d&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=33eb2f9e225647bbbcf800dafc731b9d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/query_context_processor.py
**Line:** 288:290
**Comment:**
*Logic Error: This fallback only keeps columns found in `all_labels`,
but `all_labels` is built from physical columns only; adhoc groupby columns
(including pivot temporal BASE_AXIS columns sent as adhoc SQL columns) are
dropped from every fallback subquery. On engines without native grouping sets,
those dimensions are silently omitted, so subtotals/grand totals are computed
at the wrong granularity. Include adhoc-labeled groupby columns when selecting
each level so fallback results match native behavior.
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%2F41184&comment_hash=62efd414b0b8c10bb055ea396a7a9695fd4aa167766521755e9f8f5891017404&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=62efd414b0b8c10bb055ea396a7a9695fd4aa167766521755e9f8f5891017404&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: [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]
