codeant-ai-for-open-source[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3564138579
##########
superset/common/query_context_processor.py:
##########
@@ -252,9 +256,60 @@ 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 = [get_column_name(col) for col in query_object.columns]
+ label_to_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(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:** The fallback reuses the original query object (including
`row_limit`) for every rollup level. That applies the limit independently per
level, which can truncate subtotal/grand-total rows and produce different
results than the native GROUPING SETS path (which explicitly skips per-level
limiting). Clear `row_limit` on each fallback subquery and, if needed, enforce
any final cap only once on the concatenated result. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Pivot/pivot-table rollup totals incorrect on non-supporting engines.
⚠️ Grouping-sets rollup results inconsistent across database backends.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a pivot/table Chart Data payload with a grouping_sets rollup on
a dataset
backed by an engine without native GROUPING SETS support
(SqliteEngineSpec.supports_grouping_sets is False, verified in
tests/unit_tests/db_engine_specs/test_grouping_sets_capability.py:39). Use a
payload
similar to TestGroupingSetsRollup in
tests/integration_tests/non_additive_totals_tests.py:230-249, but targeting
such an engine
and with a finite row_limit (for example 100).
2. The ChartData API parses this payload via
ChartDataQueryContextSchema().load(payload)
as in tests/integration_tests/non_additive_totals_tests.py:42-45, producing
a QueryContext
whose first QueryObject has columns ["gender", "state"], a non-additive
metric,
grouping_sets levels, and row_limit set to the requested value.
3. When QueryContext.get_query_result(query_context.queries[0]) is called
(tests/integration_tests/non_additive_totals_tests.py:299-2), it delegates to
QueryContextProcessor.get_query_result(query_object) in
superset/common/query_context_processor.py:33-49. Because
datasource.db_engine_spec.supports_grouping_sets is False,
_supports_grouping_sets()
(lines 51-55) returns False and _grouping_sets_fallback(query_object) (lines
57-92) is
invoked.
4. Inside _grouping_sets_fallback, the loop over levels at
superset/common/query_context_processor.py:73-80 creates sub_query =
copy.copy(query_object), leaves sub_query.row_limit unchanged, and calls
self._qc_datasource.get_query_result(sub_query) for each level. This applies
the same
row_limit independently to every level, truncating each per-level DataFrame
before
concatenation at line 91. The combined result.df therefore omits rows that
would be
present if the GROUPING SETS path fetched all rows once and applied any
limit only on the
final combined result, yielding incorrect subtotals/grand totals on engines
using the
fallback.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3a51977983a74a1c9ef6c6c365e0c1df&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=3a51977983a74a1c9ef6c6c365e0c1df&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:** 294:298
**Comment:**
*Logic Error: The fallback reuses the original query object (including
`row_limit`) for every rollup level. That applies the limit independently per
level, which can truncate subtotal/grand-total rows and produce different
results than the native GROUPING SETS path (which explicitly skips per-level
limiting). Clear `row_limit` on each fallback subquery and, if needed, enforce
any final cap only once on the concatenated result.
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=6478106458fb985c103eac66d2af066b06ec84f9e2d6fe063e2891882902af20&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=6478106458fb985c103eac66d2af066b06ec84f9e2d6fe063e2891882902af20&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]
