codeant-ai-for-open-source[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3557744988
##########
superset/charts/schemas.py:
##########
@@ -1358,6 +1358,17 @@ class Meta: # pylint: disable=too-few-public-methods
load_default=False,
allow_none=True,
)
+ grouping_sets = fields.List(
Review Comment:
**Suggestion:** Add an explicit type annotation to this newly introduced
schema field attribute so the new code complies with the repository's type-hint
requirement. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is newly added Python code and the class attribute is introduced
without any type annotation, which matches the repository rule requiring type
hints on annotatable variables.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dad7a907f96844bcb969d01667475951&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=dad7a907f96844bcb969d01667475951&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/charts/schemas.py
**Line:** 1361:1361
**Comment:**
*Custom Rule: Add an explicit type annotation to this newly introduced
schema field attribute so the new code complies with the repository's type-hint
requirement.
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=ca1a3461756e5c59e901da112f4d4a9aaa2f74f2fa09037a7224b84870a95a2e&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=ca1a3461756e5c59e901da112f4d4a9aaa2f74f2fa09037a7224b84870a95a2e&reaction=dislike'>๐</a>
##########
superset/common/query_context_processor.py:
##########
@@ -252,9 +254,54 @@ 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 = getattr(self._qc_datasource, "db_engine_spec", None)
Review Comment:
**Suggestion:** Add an explicit type annotation for this local variable to
comply with the requirement for annotating relevant variables in new code.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is new Python code introducing a local variable whose type can be
annotated, and it lacks an explicit type hint. That matches the custom rule
requiring type hints on relevant variables in modified Python code.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=997647b0a4134885b8f8ba8ac6a3d7ce&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=997647b0a4134885b8f8ba8ac6a3d7ce&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:** 269:269
**Comment:**
*Custom Rule: Add an explicit type annotation for this local variable
to comply with the requirement for annotating relevant variables in new code.
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=561944dd7f2fdf1559f5aaccfdf501c01338bca9453b07972a4ec2660a453ba3&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=561944dd7f2fdf1559f5aaccfdf501c01338bca9453b07972a4ec2660a453ba3&reaction=dislike'>๐</a>
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -96,6 +96,7 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec):
supports_dynamic_schema = True
supports_catalog = supports_dynamic_catalog =
supports_cross_catalog_queries = True
+ supports_grouping_sets = True
Review Comment:
**Suggestion:** Add an explicit type annotation for this new class attribute
so the newly introduced variable is type-hinted. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The added class attribute is a boolean constant that can be explicitly
annotated, e.g. `supports_grouping_sets: bool = True`. Because the rule
requires type hints on new or modified Python code where relevant variables can
be annotated, this is a real violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=40128bd2bc22478c881db795e7673f35&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=40128bd2bc22478c881db795e7673f35&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/snowflake.py
**Line:** 99:99
**Comment:**
*Custom Rule: Add an explicit type annotation for this new class
attribute so the newly introduced variable is type-hinted.
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=1b022f9c89688ea147a3d349f7f8e9b9fc61e4e2762876b2b35516a5472902dc&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=1b022f9c89688ea147a3d349f7f8e9b9fc61e4e2762876b2b35516a5472902dc&reaction=dislike'>๐</a>
##########
superset/db_engine_specs/postgres.py:
##########
@@ -278,6 +278,7 @@ class PostgresEngineSpec(BasicParametersMixin,
PostgresBaseEngineSpec):
supports_dynamic_schema = True
supports_catalog = True
supports_dynamic_catalog = True
+ supports_grouping_sets = True
Review Comment:
**Suggestion:** Add an explicit boolean type annotation to this newly
introduced class capability flag. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The new class attribute is a boolean capability flag and is directly
annotatable (for example, `supports_grouping_sets: bool = True`). Because the
added code omits the type hint, it matches the rule requiring type hints for
new or modified Python variables that can be annotated.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=07fc29f6a4824a3e8c125b5af41c5fe6&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=07fc29f6a4824a3e8c125b5af41c5fe6&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/postgres.py
**Line:** 281:281
**Comment:**
*Custom Rule: Add an explicit boolean type annotation to this newly
introduced class capability flag.
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=0c734684148f6c863613b08aa02fb20ba713b55b978ee126fd7b7f5391546c4b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=0c734684148f6c863613b08aa02fb20ba713b55b978ee126fd7b7f5391546c4b&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/utilities.test.ts:
##########
@@ -0,0 +1,446 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { TimeGranularity } from '@superset-ui/core';
+import buildGroupbyCombinations, {
+ isAdditiveMetric,
+ allMetricsAdditive,
+ additiveReducerFor,
+ synthesizeAdditiveLevels,
+ splitGroupingSetsResult,
+ groupingMarkerLabel,
+} from '../../src/plugin/utilities';
+import { PivotTableQueryFormData, MetricsLayoutEnum } from '../../src/types';
+
+const baseFormData = {
+ groupbyRows: ['row1', 'row2'],
+ groupbyColumns: ['col1', 'col2'],
+ metrics: ['metric1', 'metric2'],
+ tableRenderer: 'Table With Subtotal',
+ colOrder: 'key_a_to_z',
+ rowOrder: 'key_a_to_z',
+ aggregateFunction: 'Sum',
+ metricsLayout: MetricsLayoutEnum.ROWS,
+ transposePivot: false,
+ rowSubtotalPosition: true,
+ colSubtotalPosition: true,
+ colTotals: true,
+ colSubTotals: true,
+ rowTotals: true,
+ rowSubTotals: true,
+ valueFormat: 'SMART_NUMBER',
+ datasource: '5__table',
+ viz_type: 'my_chart',
+ width: 800,
+ height: 600,
+ combineMetric: false,
+ verboseMap: {},
+ columnFormats: {},
+ currencyFormats: {},
+ metricColorFormatters: [],
+ dateFormatters: {},
+ setDataMask: () => {},
+ legacy_order_by: 'count',
+ order_desc: true,
+ margin: 0,
+ time_grain_sqla: TimeGranularity.MONTH,
+ temporal_columns_lookup: { col1: true },
+ currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' },
+} as unknown as PivotTableQueryFormData;
+
+test('should build all combinations for basic pivot table', () => {
+ const combinations = buildGroupbyCombinations(baseFormData);
+
+ expect(combinations).toEqual([
+ { rows: [], columns: [] },
+ { rows: [], columns: ['col1'] },
+ { rows: [], columns: ['col1', 'col2'] },
+ { rows: ['row1'], columns: [] },
+ { rows: ['row1'], columns: ['col1'] },
+ { rows: ['row1'], columns: ['col1', 'col2'] },
+ { rows: ['row1', 'row2'], columns: [] },
+ { rows: ['row1', 'row2'], columns: ['col1'] },
+ { rows: ['row1', 'row2'], columns: ['col1', 'col2'] },
+ ]);
+
+ expect(combinations).toHaveLength(9);
+});
+
+test('should handle transposed pivot correctly', () => {
+ const modifiedFormData = {
+ ...baseFormData,
+ transposePivot: true,
+ };
+
+ const combinations = buildGroupbyCombinations(modifiedFormData);
+
+ expect(combinations).toEqual([
+ { rows: [], columns: [] },
+ { rows: [], columns: ['row1'] },
+ { rows: [], columns: ['row1', 'row2'] },
+ { rows: ['col1'], columns: [] },
+ { rows: ['col1'], columns: ['row1'] },
+ { rows: ['col1'], columns: ['row1', 'row2'] },
+ { rows: ['col1', 'col2'], columns: [] },
+ { rows: ['col1', 'col2'], columns: ['row1'] },
+ { rows: ['col1', 'col2'], columns: ['row1', 'row2'] },
+ ]);
+});
+
+test('should filter combinations when combineMetric is true with ROWS layout',
() => {
+ const modifiedFormData = {
+ ...baseFormData,
+ combineMetric: true,
+ metricsLayout: MetricsLayoutEnum.ROWS,
+ };
+
+ const combinations = buildGroupbyCombinations(modifiedFormData);
+
+ expect(combinations).toEqual([
+ { rows: ['row1', 'row2'], columns: [] },
+ { rows: ['row1', 'row2'], columns: ['col1'] },
+ { rows: ['row1', 'row2'], columns: ['col1', 'col2'] },
+ ]);
+
+ expect(combinations).toHaveLength(3);
+});
+
+test('should filter combinations when combineMetric is true with COLUMNS
layout', () => {
+ const modifiedFormData = {
+ ...baseFormData,
+ combineMetric: true,
+ metricsLayout: MetricsLayoutEnum.COLUMNS,
+ };
+
+ const combinations = buildGroupbyCombinations(modifiedFormData);
+
+ expect(combinations).toEqual([
+ { rows: [], columns: ['col1', 'col2'] },
+ { rows: ['row1'], columns: ['col1', 'col2'] },
+ { rows: ['row1', 'row2'], columns: ['col1', 'col2'] },
+ ]);
+
+ expect(combinations).toHaveLength(3);
+});
+
+test('should handle single dimension in rows only', () => {
+ const modifiedFormData = {
+ ...baseFormData,
+ groupbyRows: ['row'],
+ groupbyColumns: [],
+ };
+
+ const combinations = buildGroupbyCombinations(modifiedFormData);
+
+ expect(combinations).toEqual([
+ { rows: [], columns: [] },
+ { rows: ['row'], columns: [] },
+ ]);
+
+ expect(combinations).toHaveLength(2);
+});
+
+test('should handle single dimension in columns only', () => {
+ const modifiedFormData = {
+ ...baseFormData,
+ groupbyRows: [],
+ groupbyColumns: ['col'],
+ };
+
+ const combinations = buildGroupbyCombinations(modifiedFormData);
+
+ expect(combinations).toEqual([
+ { rows: [], columns: [] },
+ { rows: [], columns: ['col'] },
+ ]);
+
+ expect(combinations).toHaveLength(2);
+});
+
+test('should handle empty groupby arrays', () => {
+ const modifiedFormData = {
+ ...baseFormData,
+ groupbyRows: [],
+ groupbyColumns: [],
+ };
+
+ const combinations = buildGroupbyCombinations(modifiedFormData);
+
+ expect(combinations).toEqual([{ rows: [], columns: [] }]);
+
+ expect(combinations).toHaveLength(1);
+});
+
+test('should work with combineMetric and transposed pivot', () => {
+ const modifiedFormData = {
+ ...baseFormData,
+ transposePivot: true,
+ combineMetric: true,
+ metricsLayout: MetricsLayoutEnum.COLUMNS,
+ };
+
+ const combinations = buildGroupbyCombinations(modifiedFormData);
+
+ expect(combinations).toEqual([
+ { rows: [], columns: ['row1', 'row2'] },
+ { rows: ['col1'], columns: ['row1', 'row2'] },
+ { rows: ['col1', 'col2'], columns: ['row1', 'row2'] },
+ ]);
+});
+
+test('should handle combineMetric with empty arrays correctly', () => {
+ const modifiedFormData = {
+ ...baseFormData,
+ groupbyRows: [],
+ groupbyColumns: ['col'],
+ combineMetric: true,
+ metricsLayout: MetricsLayoutEnum.ROWS,
+ };
+
+ const combinations = buildGroupbyCombinations(modifiedFormData);
+
+ expect(combinations).toEqual([
+ { rows: [], columns: [] },
+ { rows: [], columns: ['col'] },
+ ]);
+});
+
+test('should work with large number of dimensions', () => {
+ const modifiedFormData = {
+ ...baseFormData,
+ groupbyRows: ['r1', 'r2', 'r3', 'r4'],
+ groupbyColumns: ['c1', 'c2', 'c3'],
+ };
+
+ const combinations = buildGroupbyCombinations(modifiedFormData);
+
+ expect(combinations).toHaveLength(20);
+
+ expect(combinations).toContainEqual({ rows: [], columns: [] });
+ expect(combinations).toContainEqual({
+ rows: ['r1', 'r2', 'r3', 'r4'],
+ columns: ['c1', 'c2', 'c3'],
+ });
+});
+
+test('isAdditiveMetric: SIMPLE metrics with additive aggregates are additive',
() => {
+ expect(
+ isAdditiveMetric({
+ expressionType: 'SIMPLE',
+ aggregate: 'SUM',
+ column: { column_name: 'num' },
+ label: 'sum_num',
+ } as any),
+ ).toBe(true);
+ expect(
+ isAdditiveMetric({
+ expressionType: 'SIMPLE',
+ aggregate: 'COUNT',
+ column: { column_name: 'num' },
+ label: 'count_num',
+ } as any),
Review Comment:
**Suggestion:** Replace this `any` cast with the proper metric object type
instead of forcing the value through `any`. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This line also uses an `any` cast in new TypeScript test code, so it
violates the no-any-types rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1333e298f0a3414ab46c2be0f55d521c&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=1333e298f0a3414ab46c2be0f55d521c&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/test/plugin/utilities.test.ts
**Line:** 257:257
**Comment:**
*Custom Rule: Replace this `any` cast with the proper metric object
type instead of forcing the value through `any`.
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=a1d9a7f9c78b90a1c411074a4dcfa05dede63b037f545967fd8065061ac644e6&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=a1d9a7f9c78b90a1c411074a4dcfa05dede63b037f545967fd8065061ac644e6&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]
