Copilot commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3623679829
##########
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:
`row_limit` is intentionally skipped for `GROUPING SETS` queries, which
makes the Pivot Table “Cell limit” control ineffective for non-additive metrics
and can significantly increase result size (and runtime/memory) compared to the
previous leaf-only query. It also changes semantics vs additive fast-path where
`row_limit` still applies.
Consider either (a) documenting/validating that `row_limit` is ignored when
`grouping_sets` is used, or (b) implementing a leaf-only limiting strategy
(likely requires splitting into per-level queries when a limit/offset is
present, or another approach that preserves totals rows).
##########
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:
The fallback explicitly clears `row_limit` for every per-level subquery,
which (combined with the native path skipping LIMIT) means the Pivot Table
“Cell limit” won’t constrain non-additive rollup queries on engines without
native GROUPING SETS either. This can lead to unexpectedly large query results.
If the intent is to ignore limits for rollups, it’d be helpful to
enforce/document this at the API/form-data layer; otherwise consider
preserving/applying an equivalent limit behavior across levels.
##########
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:
`grouping_sets` is serialized to the backend as `list[list[str]]` (column
*labels*), not as `QueryFormColumn[][]`. Using `QueryFormColumn[][]` here makes
the test type misleading and can hide future shape/type regressions.
--
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]
