codeant-ai-for-open-source[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3562190150
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -383,6 +383,38 @@ const fmtNonString =
(x: string | number | null): string =>
typeof x === 'string' ? x : formatter(x as number);
+/*
+ * Passthrough "aggregator" for the multi-query pivot. Because the database
+ * already computed every rollup level (one query per level), each cell
receives
Review Comment:
**Suggestion:** The new comment states that values come from “one query per
level,” but this PR’s query path explicitly uses a single `GROUPING SETS` query
when supported, so the comment now contradicts real behavior and can mislead
future maintenance/debugging. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Comment misstates query model used by PivotTable rollups.
⚠️ Future maintenance may mis-optimize based on incorrect assumptions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the non-additive query path in
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:127-144`,
where the comment and code explicitly state that for non-additive metrics
the pivot uses
“a single GROUPING SETS query” and then `splitGroupingSetsResult()` to split
the combined
result into per-level frames.
2. Check the backend integration in
`superset/superset/common/grouping_sets.py:20-27` and
`superset/superset/common/query_context_processor.py:259-279`, which
documents that when a
datasource engine reports `supports_grouping_sets`, N per-rollup-level
queries are
collapsed into a single native `GROUPING SETS` query, and only when native
support is
absent does the processor fall back to “one query per rollup level” with
concatenated
results.
3. Compare this architecture with the comment on `cellValue` in
`superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:27-33`,
which describes it as a passthrough aggregator “for the multi-query pivot”
and asserts
that “the database already computed every rollup level (one query per
level),” treating
the multi-query fallback as the generic behavior.
4. Because `cellValue` is used by `PivotData` for all non-additive rollup
results
regardless of whether the backend executed a single `GROUPING SETS` query or
multiple
per-level queries (see `PivotData.constructor` at utilities.ts:53-103 and
`transformProps`
at transformProps.ts:127-144), the “one query per level” wording in the
comment is now
misleading and contradicts the supported single-query path, which can
confuse future
maintainers about the actual query execution model.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=451f89e004754b9d8b2edbc5f96e717d&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=451f89e004754b9d8b2edbc5f96e717d&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-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts
**Line:** 388:388
**Comment:**
*Comment Mismatch: The new comment states that values come from “one
query per level,” but this PR’s query path explicitly uses a single `GROUPING
SETS` query when supported, so the comment now contradicts real behavior and
can mislead future maintenance/debugging.
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=d5bcfae09a71fb8780b422161688367f40c419130a905972b5bd419d8c3a6c31&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=d5bcfae09a71fb8780b422161688367f40c419130a905972b5bd419d8c3a6c31&reaction=dislike'>👎</a>
##########
superset/db_engine_specs/presto.py:
##########
@@ -166,6 +166,7 @@ class PrestoBaseEngineSpec(BaseEngineSpec,
metaclass=ABCMeta):
supports_dynamic_schema = True
supports_catalog = supports_dynamic_catalog =
supports_cross_catalog_queries = True
+ supports_grouping_sets = True
Review Comment:
**Suggestion:** Setting this capability on `PrestoBaseEngineSpec` propagates
to all subclasses in the inheritance chain (including `HiveEngineSpec`,
`SparkEngineSpec`, and Databricks Hive), which can force the native GROUPING
SETS path on engines that may not fully support the same `GROUPING()` marker
semantics. That bypasses the fallback path and can cause runtime SQL failures
for those engines; set the flag on `PrestoEngineSpec`/`TrinoEngineSpec`
directly or explicitly override it to `False` on non-guaranteed descendants.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Pivot-table non-additive totals fail on Hive-like databases.
⚠️ Databricks Hive rollup queries may error on execution.
⚠️ SparkEngineSpec inherits flag; potential GROUPING SETS failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a database using Hive or a Hive-derived engine (e.g. Databricks
Hive or
Spark), whose spec classes are `HiveEngineSpec` at
`superset/db_engine_specs/hive.py:88`
(extends `PrestoEngineSpec`) and `DatabricksHiveEngineSpec` at
`superset/db_engine_specs/databricks.py:235` (extends `HiveEngineSpec`),
both inheriting
`PrestoBaseEngineSpec` in `superset/db_engine_specs/presto.py:162-169` where
`supports_grouping_sets = True` is set.
2. Create a Pivot Table chart with a non-additive metric (such as a saved
SQL metric, AVG,
or COUNT_DISTINCT) so the frontend `buildQuery` helper in
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts:85-96`
populates `grouping_sets` on the single query and sends it to the backend
`QueryContext`.
3. On the backend, the `QueryObject` with `grouping_sets` flows into
`QueryContextProcessor.get_query_result` in
`superset/common/query_context_processor.py:250-23`;
`_supports_grouping_sets()` reads
`self._qc_datasource.db_engine_spec.supports_grouping_sets`, which is `True`
for
`HiveEngineSpec`/`DatabricksHiveEngineSpec`/`SparkEngineSpec` via
`PrestoBaseEngineSpec.supports_grouping_sets` at
`superset/db_engine_specs/presto.py:169`,
so the fallback `_grouping_sets_fallback()` is skipped and native `GROUPING
SETS` SQL is
emitted.
4. The query builder in `superset/models/helpers.py:3564-3570` sees
`db_engine_spec.supports_grouping_sets` as `True`, appends `GROUPING()`
marker columns via
`grouping_id_column` and issues a single `GROUPING SETS` query. On engines
like
`HiveEngineSpec` (`superset/db_engine_specs/hive.py:88-134`) or
`DatabricksHiveEngineSpec`
(`superset/db_engine_specs/databricks.py:235-252`) that do not fully support
`GROUPING
SETS`/`GROUPING()` with the semantics assumed by
`superset/common/grouping_sets.py:48-79`,
this SQL can fail at runtime (syntax errors or unsupported function),
breaking Pivot Table
rollup queries instead of using the per-level fallback.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1348d6b183be400489ab101a09fa4028&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=1348d6b183be400489ab101a09fa4028&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/db_engine_specs/presto.py
**Line:** 169:169
**Comment:**
*Api Mismatch: Setting this capability on `PrestoBaseEngineSpec`
propagates to all subclasses in the inheritance chain (including
`HiveEngineSpec`, `SparkEngineSpec`, and Databricks Hive), which can force the
native GROUPING SETS path on engines that may not fully support the same
`GROUPING()` marker semantics. That bypasses the fallback path and can cause
runtime SQL failures for those engines; set the flag on
`PrestoEngineSpec`/`TrinoEngineSpec` directly or explicitly override it to
`False` on non-guaranteed descendants.
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=e110a706e9974575a22f844cbbf6bdeade3bd2df824f2b4f2011a4931934e67f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=e110a706e9974575a22f844cbbf6bdeade3bd2df824f2b4f2011a4931934e67f&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]
