codeant-ai-for-open-source[bot] commented on code in PR #41184: URL: https://github.com/apache/superset/pull/41184#discussion_r3555845735
########## superset/common/grouping_sets.py: ########## @@ -0,0 +1,116 @@ +# 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. + +""" +SQL building blocks for the pivot-table non-additive totals optimization +(SIP.md, phase 3b). When a datasource engine reports +``supports_grouping_sets``, the N per-rollup-level queries can be collapsed into +a single ``GROUPING SETS`` query: the database computes every level in one scan, +and each returned row is attributed to its level via ``GROUPING()`` markers. + +These are the engine-agnostic SQL primitives. Wiring them into the query context +(emitting one query and splitting the result back into per-level results) is the +remaining integration; see SIP.md. +""" + +from __future__ import annotations + +from collections.abc import Sequence + +import pandas as pd +from sqlalchemy import func, tuple_ +from sqlalchemy.sql.elements import ColumnElement + +# Suffix for the per-column GROUPING() marker columns added to a GROUPING SETS +# query. Chosen to be unlikely to collide with a real metric/column label. +GROUPING_MARKER_SUFFIX = "__superset_grouping" Review Comment: **Suggestion:** Add an explicit type annotation to this module-level constant so it complies with the required type-hinting rule for annotatable variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This module-level constant is a simple string value that can be annotated, but it is declared without a type hint. That matches the rule requiring type hints on relevant 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=b88ee1d9981948808b1e6f5347a0adb2&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=b88ee1d9981948808b1e6f5347a0adb2&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/grouping_sets.py **Line:** 40:40 **Comment:** *Custom Rule: Add an explicit type annotation to this module-level constant so it complies with the required type-hinting rule for annotatable variables. 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=fd447a30ae1ac8fad98b2d8da8be51b48307da59579fe6d2a29222080c471bed&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=fd447a30ae1ac8fad98b2d8da8be51b48307da59579fe6d2a29222080c471bed&reaction=dislike'>👎</a> ########## superset/db_engine_specs/base.py: ########## @@ -560,6 +560,12 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods # if True, database will be listed as option in the upload file form supports_file_upload = True + # Whether the engine supports SQL GROUPING SETS / ROLLUP / CUBE. When True, + # consumers (e.g. the pivot table's non-additive totals) can collapse the + # per-rollup-level queries into a single GROUPING SETS query instead of + # issuing one query per level. Conservative default of False; engines opt in. + supports_grouping_sets = False Review Comment: **Suggestion:** Add an explicit boolean type annotation to this newly introduced class attribute to comply with the type-hint requirement. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The new class attribute is a boolean constant assigned without a type hint. The rule explicitly requires type hints on new or modified variables that can be annotated, so 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=53bd1a7673a649918ea161db912faa06&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=53bd1a7673a649918ea161db912faa06&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/base.py **Line:** 567:567 **Comment:** *Custom Rule: Add an explicit boolean type annotation to this newly introduced class attribute to comply with the 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=3b382d9496e03986a168eb41a18ad90e83ef6be104dbdac6d606dc556fcd7d35&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=3b382d9496e03986a168eb41a18ad90e83ef6be104dbdac6d606dc556fcd7d35&reaction=dislike'>👎</a> ########## superset/common/query_object.py: ########## @@ -157,6 +158,7 @@ def __init__( # pylint: disable=too-many-locals, too-many-arguments self.series_limit = series_limit self.series_limit_metric = series_limit_metric self.group_others_when_limit_reached = group_others_when_limit_reached + self.grouping_sets = grouping_sets or [] Review Comment: **Suggestion:** Add an explicit type annotation for the new instance attribute so `grouping_sets` is declared alongside other typed class attributes. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The new instance attribute `grouping_sets` is assigned without an explicit type annotation, while nearby class attributes are typed. This matches the rule for flagging newly introduced 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=0e974df621bd4c2db3fa774d6e42b52a&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=0e974df621bd4c2db3fa774d6e42b52a&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_object.py **Line:** 161:161 **Comment:** *Custom Rule: Add an explicit type annotation for the new instance attribute so `grouping_sets` is declared alongside other typed class attributes. 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=7aafc6b02a17730c0819bf78e1221615136c3f66ce83bdc239729578dc972d09&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=7aafc6b02a17730c0819bf78e1221615136c3f66ce83bdc239729578dc972d09&reaction=dislike'>👎</a> ########## superset/db_engine_specs/bigquery.py: ########## @@ -287,6 +287,7 @@ class BigQueryEngineSpec(BaseEngineSpec): # pylint: disable=too-many-public-met supports_catalog = supports_dynamic_catalog = supports_cross_catalog_queries = True supports_dynamic_schema = True + supports_grouping_sets = True Review Comment: **Suggestion:** Add an explicit boolean type annotation to this newly introduced class attribute. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is a newly added class attribute in Python and its type is clearly inferable as bool, so it can be annotated as `supports_grouping_sets: bool = True` under the type-hint requirement. </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=dcca576b97b340f9a9cfe05c7912535d&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=dcca576b97b340f9a9cfe05c7912535d&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/bigquery.py **Line:** 290:290 **Comment:** *Custom Rule: Add an explicit boolean type annotation to this newly introduced class attribute. 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=4d82b19e4f258106ea05aa390db0e051953f7823edc33d124603579238201a40&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=4d82b19e4f258106ea05aa390db0e051953f7823edc33d124603579238201a40&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts: ########## @@ -56,9 +55,41 @@ const formData: PivotTableQueryFormData = { currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' }, }; -test('should build groupby with series in form data', () => { +test('additive metrics use the fast-path: a single full-detail query', () => { + const { queries } = buildQuery({ + ...formData, + metrics: [ + { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'num' }, + label: 'sum_num', + }, + ] as any, + }); + expect(queries).toHaveLength(1); + // The single leaf query carries all dimensions (2 rows + 2 cols). + expect(queries[0].columns).toHaveLength(4); +}); + +test('non-additive metrics emit a single GROUPING SETS query with all levels', () => { + // 2 row dims x 2 col dims -> (2+1) x (2+1) = 9 rollup levels, carried as + // grouping_sets on a single query (saved-metric strings are non-additive). const queryContext = buildQuery(formData); + expect(queryContext.queries).toHaveLength(1); const [query] = queryContext.queries; + // The single query selects the full set of dimensions ... + expect(query.columns).toHaveLength(4); + // ... and requests every rollup level via grouping_sets (grand total = []). + expect((query as any).grouping_sets).toHaveLength(9); Review Comment: **Suggestion:** Avoid casting `query` to `any` and use a typed query interface that includes `grouping_sets`. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The changed test uses `query as any` to access `grouping_sets`, which is exactly the kind of `any` usage the rule forbids in new or modified TypeScript code. </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=5681f28a9f524fdea5b6b616353c9ae7&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=5681f28a9f524fdea5b6b616353c9ae7&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/buildQuery.test.ts **Line:** 84:84 **Comment:** *Custom Rule: Avoid casting `query` to `any` and use a typed query interface that includes `grouping_sets`. 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=f98638557a803b6d4c59578ba3a0e79bd8840b29dc2e8cc05899a474ffaf883c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=f98638557a803b6d4c59578ba3a0e79bd8840b29dc2e8cc05899a474ffaf883c&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts: ########## @@ -56,9 +55,41 @@ const formData: PivotTableQueryFormData = { currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' }, }; -test('should build groupby with series in form data', () => { +test('additive metrics use the fast-path: a single full-detail query', () => { + const { queries } = buildQuery({ + ...formData, + metrics: [ + { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'num' }, + label: 'sum_num', + }, + ] as any, Review Comment: **Suggestion:** Replace this `any` cast with a concrete metric array type so the test data is typed explicitly. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The new test code explicitly casts the metric object array to `any`, which matches the rule forbidding `any` in changed TypeScript code. A concrete metric type should be used instead. </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=e35f122c33c849bf903e929c74f22693&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=e35f122c33c849bf903e929c74f22693&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/buildQuery.test.ts **Line:** 68:68 **Comment:** *Custom Rule: Replace this `any` cast with a concrete metric array type so the test data is typed explicitly. 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=2a083ee7fede73106bfaf8a0e06ba5d480170176a7ad34d95b9f2d72909f9ebd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=2a083ee7fede73106bfaf8a0e06ba5d480170176a7ad34d95b9f2d72909f9ebd&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts: ########## @@ -56,9 +55,41 @@ const formData: PivotTableQueryFormData = { currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' }, }; -test('should build groupby with series in form data', () => { +test('additive metrics use the fast-path: a single full-detail query', () => { + const { queries } = buildQuery({ + ...formData, + metrics: [ + { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'num' }, + label: 'sum_num', + }, + ] as any, + }); + expect(queries).toHaveLength(1); + // The single leaf query carries all dimensions (2 rows + 2 cols). + expect(queries[0].columns).toHaveLength(4); +}); + +test('non-additive metrics emit a single GROUPING SETS query with all levels', () => { + // 2 row dims x 2 col dims -> (2+1) x (2+1) = 9 rollup levels, carried as + // grouping_sets on a single query (saved-metric strings are non-additive). const queryContext = buildQuery(formData); + expect(queryContext.queries).toHaveLength(1); const [query] = queryContext.queries; + // The single query selects the full set of dimensions ... + expect(query.columns).toHaveLength(4); + // ... and requests every rollup level via grouping_sets (grand total = []). + expect((query as any).grouping_sets).toHaveLength(9); + expect((query as any).grouping_sets[0]).toEqual([]); Review Comment: **Suggestion:** Replace the `any` cast with a concrete typed structure before checking individual grouping set entries. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is another direct `any` cast introduced in the changed TypeScript test, so it violates the no-`any` 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=e457f48d5f2e4b59a585939378217210&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=e457f48d5f2e4b59a585939378217210&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/buildQuery.test.ts **Line:** 85:85 **Comment:** *Custom Rule: Replace the `any` cast with a concrete typed structure before checking individual grouping set entries. 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=1c87bc670ed2dbedbf8bb31d7d9711b69cba6f2db1ecfdf24019429e56d27342&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=1c87bc670ed2dbedbf8bb31d7d9711b69cba6f2db1ecfdf24019429e56d27342&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/nonAdditiveTotals.test.ts: ########## @@ -0,0 +1,69 @@ +/** + * 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. + */ + +/** + * Pivot-table acceptance cases for non-additive metric totals (see SIP.md). + * + * The pivot computes its grand total / subtotals client-side using the + * `aggregators` from react-pivottable, applied to the already-aggregated cell + * values. For a non-additive metric (a ratio like SUM(actual)/SUM(target)) + * that is wrong: summing the per-group ratios is meaningless. This pins the + * mechanism behind #25747 / #32260 against the real production aggregator. + * + * The fix is not at this layer (no client-side aggregator over divided cells + * can recover the right answer) - it requires the DB-computed per-level rollup + * queries from phase 2. The `test.failing` case documents that target. + */ +import { aggregators } from '../../src/react-pivottable/utilities'; + +// Per-group completion ratios (actual/target), as the single pivot query +// returns them today. +const ratioCells = [{ completion: 10 / 40 }, { completion: 30 / 60 }]; // 0.25, 0.5 + +// Correct grand total: SUM(actual)/SUM(target) = 40/100 = 0.4 +const CORRECT_COMPLETION = 0.4; + +function sumOf(field: string, rows: Record<string, number>[]): number { + // Build the real "Sum" aggregator the pivot uses for totals. + const aggregator = (aggregators as Record<string, any>).Sum([field])( Review Comment: **Suggestion:** Replace the `any` cast with a concrete aggregator interface/type so the `Sum` access and return value are strongly typed. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The code uses `any` in a changed TypeScript file via `Record<string, any>`, which directly violates the no-any-types rule. A concrete type should be used instead. </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=a84b07f8227d410fbf735386c820f276&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=a84b07f8227d410fbf735386c820f276&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/react-pivottable/nonAdditiveTotals.test.ts **Line:** 44:44 **Comment:** *Custom Rule: Replace the `any` cast with a concrete aggregator interface/type so the `Sum` access and return value are strongly typed. 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=af978e726f2c35c6bd974a6e671a0a7c6a1997cd5323554584d96246c24c3206&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=af978e726f2c35c6bd974a6e671a0a7c6a1997cd5323554584d96246c24c3206&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts: ########## @@ -56,9 +55,41 @@ const formData: PivotTableQueryFormData = { currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' }, }; -test('should build groupby with series in form data', () => { +test('additive metrics use the fast-path: a single full-detail query', () => { + const { queries } = buildQuery({ + ...formData, + metrics: [ + { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'num' }, + label: 'sum_num', + }, + ] as any, + }); + expect(queries).toHaveLength(1); + // The single leaf query carries all dimensions (2 rows + 2 cols). + expect(queries[0].columns).toHaveLength(4); +}); + +test('non-additive metrics emit a single GROUPING SETS query with all levels', () => { + // 2 row dims x 2 col dims -> (2+1) x (2+1) = 9 rollup levels, carried as + // grouping_sets on a single query (saved-metric strings are non-additive). const queryContext = buildQuery(formData); + expect(queryContext.queries).toHaveLength(1); const [query] = queryContext.queries; + // The single query selects the full set of dimensions ... + expect(query.columns).toHaveLength(4); + // ... and requests every rollup level via grouping_sets (grand total = []). + expect((query as any).grouping_sets).toHaveLength(9); + expect((query as any).grouping_sets[0]).toEqual([]); + expect((query as any).grouping_sets[8]).toHaveLength(4); Review Comment: **Suggestion:** Remove this `any` cast by typing the query object with a specific type that exposes `grouping_sets` length safely. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The code added in this hunk uses `any` again to access `grouping_sets`, which is a real violation of the rule against `any` in changed TypeScript code. </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=091b7f14be6a4301b6f893fe48ede44d&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=091b7f14be6a4301b6f893fe48ede44d&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/buildQuery.test.ts **Line:** 86:86 **Comment:** *Custom Rule: Remove this `any` cast by typing the query object with a specific type that exposes `grouping_sets` length safely. 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=a1a906908101502d2f13fd521e5eff914653358d3505a28eb3e43397e0dfea9b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=a1a906908101502d2f13fd521e5eff914653358d3505a28eb3e43397e0dfea9b&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]
