codeant-ai-for-open-source[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3565632155
##########
superset/common/query_context_processor.py:
##########
@@ -252,9 +256,69 @@ 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: BaseEngineSpec | None = 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: list[list[str]] = query_object.grouping_sets
+ # Use the same label derivation as the native path (physical column
name
+ # or adhoc column label) so both column kinds are represented and each
+ # label maps back to its own column, in the same order as the source
+ # list.
+ all_labels: list[str] = [get_column_name(col) for col in
query_object.columns]
+ label_to_column: dict[str, Column] = dict(
+ zip(all_labels, query_object.columns, strict=True)
+ )
+
+ frames: list[pd.DataFrame] = []
+ result: QueryResult | None = None
+ for level in levels:
+ level_labels: set[str] = 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
+ ]
+ # A GROUPING SETS query computes a bounded set of rollup levels, so
+ # the native path never applies row_limit to it (see the
+ # `use_grouping_sets` check in models/helpers.py). Match that here:
+ # limiting each level's fallback sub-query independently would
+ # truncate subtotal/grand-total rows and diverge from the native
+ # result shape.
+ sub_query.row_limit = None
Review Comment:
**Suggestion:** The fallback path removes `row_limit` per level but leaves
`row_offset` untouched, so each per-level subquery applies the offset
independently. That diverges from native `GROUPING SETS` behavior (single
global offset) and can silently drop subtotal/grand-total rows on paginated
requests. Reset per-level `row_offset` and apply offset once after
concatenation (or disable offset for this fallback path) to keep results
consistent with native execution. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Grouping-sets rollup loses subtotal/grand total rows on pagination.
- ⚠️ Pivot and Table charts misrepresent non-additive totals when paginated.
- ⚠️ Engines without GROUPING SETS behave inconsistently versus supported
engines.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/tests/integration_tests/non_additive_totals_tests.py` inside
`TestGroupingSetsRollup.test_grouping_sets_returns_all_levels` (around lines
70-89),
extend the existing query payload by adding a non-zero offset, for example:
- set `payload["queries"][0]["row_offset"] = 1` alongside the existing
`row_limit` and
`grouping_sets` fields.
2. Observe that this test constructs a `QueryContext` via
`ChartDataQueryContextSchema().load(payload)` at lines 100-101 of the same
file:
- `query_context: QueryContext =
ChartDataQueryContextSchema().load(payload)`
- `df = query_context.get_query_result(query_context.queries[0]).df`
This calls `QueryContext.get_query_result` in
`superset/common/query_context.py:134-135`, which delegates directly to
`QueryContextProcessor.get_query_result`.
3. In `superset/common/query_context_processor.py`,
`QueryContextProcessor.get_query_result` (lines 252-268) checks for rollup
queries:
- If `query_object.grouping_sets` is non-empty and
`_supports_grouping_sets()` is
False, it calls `self._grouping_sets_fallback(query_object)` at line 267.
- `_supports_grouping_sets` (lines 270-274) looks up
`self._qc_datasource.db_engine_spec.supports_grouping_sets`.
For the birth_names test dataset, the engine spec inherits from
`BaseEngineSpec` where
`supports_grouping_sets = False` by default
(`superset/db_engine_specs/base.py:567`),
so the fallback path is taken for engines like SQLite that lack native
GROUPING SETS.
4. Inside `_grouping_sets_fallback` in
`superset/common/query_context_processor.py:282-61`, for each rollup `level`:
- A shallow copy of the original `QueryObject` is created: `sub_query =
copy.copy(query_object)` at line 37.
- `sub_query.grouping_sets` is cleared and `sub_query.columns` is
restricted to that
level (lines 38-41).
- Crucially, `sub_query.row_offset` is not changed; it keeps the original
non-zero
value set in the payload via `QueryObject.__init__`
(`superset/common/query_object.py:37-39`, where `self.row_offset =
row_offset or 0`).
- Only `sub_query.row_limit` is reset to `None` at line 48 (the PR line
under review),
and then `self._qc_datasource.get_query_result(sub_query)` is called at
line 49.
5. The datasource’s SQL construction in
`superset/models/helpers.py:3990-4016` shows how
pagination is applied:
- `use_grouping_sets` is computed earlier (lines 7-16 of the first
BulkRead snippet)
and is False for these per-level fallback queries because
`sub_query.grouping_sets` was
cleared.
- Row limit is only applied when `row_limit and not use_grouping_sets`
(lines 10-14),
but row offset is always applied when non-zero: `if row_offset: qry =
qry.offset(row_offset)` at line 15.
Therefore, every per-level fallback query runs with the same non-zero
`row_offset`
inherited from the original query object.
6. For rollup levels with very few rows (especially the grand-total level
`[]` which
produces exactly one row in `TestGroupingSetsRollup`), applying `OFFSET 1`
at the
sub-query level drops that entire level:
- The grand-total sub-query returns an empty `DataFrame` because all rows
have been
skipped before aggregation.
- `_grouping_sets_fallback` still tags each per-level frame with
`grouping_marker_label` columns (lines 50-54) and concatenates them into
`result.df`
via `pd.concat(frames, ignore_index=True)` at line 60, but the
grand-total and possibly
some subtotal frames contribute no rows.
7. Downstream, the test splits the combined result back into per-level
frames using
`split_grouping_sets_result` from `superset/common/grouping_sets.py:82-117`:
- Lines 107-116 of `non_additive_totals_tests.py` call `leaf, gender_sub,
grand =
split_grouping_sets_result(df, levels, ["gender", "state"])`.
- The test then asserts `assert len(grand) == 1` and checks that the
grand total ratio
is valid (lines 110-114).
Under the fallback with per-level offsets, `grand` becomes empty
(`len(grand) == 0`)
and these assertions fail, demonstrating that subtotal/grand-total rows
have been
silently dropped by the per-level row_offset.
8. Contrast this with the native GROUPING SETS path: for an engine where
`supports_grouping_sets = True`, such as Presto
(`superset/db_engine_specs/presto.py:919`)
or Postgres (`superset/db_engine_specs/postgres.py:281`), the same payload
builds a single
GROUPING SETS query in `models/helpers.py`:
- `use_grouping_sets` is True, so row limit is skipped for that query
(`if row_limit
and not use_grouping_sets`), but a single global `row_offset` is still
applied once at
the end (`if row_offset: qry = qry.offset(row_offset)`).
- The grand-total row remains present after offsetting, so
`split_grouping_sets_result`
still finds `len(grand) == 1`.
This shows that the fallback path’s per-level offset semantics are
inconsistent with
the native GROUPING SETS behavior and can drop rollup rows only when the
fallback is
used.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0f4d1905d6154bd7a71f79c2c879acc4&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=0f4d1905d6154bd7a71f79c2c879acc4&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:** 307:307
**Comment:**
*Logic Error: The fallback path removes `row_limit` per level but
leaves `row_offset` untouched, so each per-level subquery applies the offset
independently. That diverges from native `GROUPING SETS` behavior (single
global offset) and can silently drop subtotal/grand-total rows on paginated
requests. Reset per-level `row_offset` and apply offset once after
concatenation (or disable offset for this fallback path) to keep results
consistent with native execution.
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=e2fe2b2008ae46989420c80b9289fe06bb3a2233fb6d082979b8d3e318c50e8b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=e2fe2b2008ae46989420c80b9289fe06bb3a2233fb6d082979b8d3e318c50e8b&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]
