rusackas commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3624410628
##########
superset/models/helpers.py:
##########
@@ -3962,7 +3996,10 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
direction = sa.asc if ascending else sa.desc
qry = qry.order_by(direction(col))
- if row_limit:
+ # A GROUPING SETS query computes a bounded set of rollup levels;
applying
+ # a row LIMIT is both unnecessary and unparseable by some SQL parsers
+ # (LIMIT after a GROUPING SETS GROUP BY), so skip it for that path.
+ if row_limit and not use_grouping_sets:
qry = qry.limit(row_limit)
if row_offset:
qry = qry.offset(row_offset)
Review Comment:
This is intentional. GROUPING SETS computes a bounded set of rollup levels,
so limiting it risks cutting off subtotal/grand-total rows, and some engines
can't even parse LIMIT after a GROUPING SETS GROUP BY. Added a note to the Cell
limit control's description so users know it doesn't apply to non-additive
rollups.
##########
superset/common/query_context_processor.py:
##########
@@ -252,9 +256,78 @@ 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. The native path applies `row_offset` exactly once,
+ # to the combined multi-level result (see the unconditional
+ # `qry.offset()` call in models/helpers.py). Applying the same
+ # offset to each per-level sub-query independently would apply it
+ # once per level instead of once overall, and can silently drop
+ # low-row-count levels (e.g. the single grand-total row) entirely.
+ # Zero it here and apply it once after concatenation instead.
+ sub_query.row_limit = None
+ sub_query.row_offset = 0
+ result = self._qc_datasource.get_query_result(sub_query)
Review Comment:
Matches the native path on purpose, so the fallback behaves the same as
native GROUPING SETS regardless of engine support, rather than silently
truncating rollup levels on engines without it. Same reasoning as the
helpers.py thread. Documented the limitation on the Cell limit control instead
of enforcing it here.
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts:
##########
@@ -17,18 +17,27 @@
* under the License.
*/
-import { TimeGranularity } from '@superset-ui/core';
+import {
+ QueryFormColumn,
+ QueryFormMetric,
+ QueryObject,
+ TimeGranularity,
+} from '@superset-ui/core';
import buildQuery from '../../src/plugin/buildQuery';
import { PivotTableQueryFormData } from '../../src/types';
+// buildQuery attaches `grouping_sets` (one entry per rollup level) to the
+// query object; it is not part of the base QueryObject type, so narrow to a
+// local type instead of casting through `any`.
+type GroupingSetsQuery = QueryObject & { grouping_sets: QueryFormColumn[][] };
Review Comment:
Good catch, `grouping_sets` is column labels, not `QueryFormColumn[][]`.
Fixed the test's type to `string[][]` and dropped the unused import.
--
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]
