codeant-ai-for-open-source[bot] commented on code in PR #41729: URL: https://github.com/apache/superset/pull/41729#discussion_r3517977538
########## superset-frontend/plugins/legacy-plugin-chart-partition/src/buildQuery.ts: ########## @@ -0,0 +1,59 @@ +/** + * 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 { + buildQueryContext, + ensureIsArray, + getMetricLabel, + QueryFormData, + QueryFormMetric, + QueryFormOrderBy, +} from '@superset-ui/core'; + +/** + * Mirrors the legacy PartitionViz.query_obj: a query grouped by all the + * hierarchy levels, timeseries only when the time-series option needs + * per-timestamp data, with the legacy sort-metric handling. + */ +export default function buildQuery(formData: QueryFormData) { + const { time_series_option, timeseries_limit_metric, order_desc } = formData; + return buildQueryContext(formData, baseQueryObject => { + let metrics: QueryFormMetric[] = ensureIsArray(baseQueryObject.metrics); + const orderby: QueryFormOrderBy[] = []; + const sortByMetric = ensureIsArray( + timeseries_limit_metric as QueryFormMetric | QueryFormMetric[], + )[0]; Review Comment: **Suggestion:** This misses the legacy fallback to the first metric when no explicit sort metric is provided. As a result, queries that previously had deterministic metric-based ordering now run without the expected default sort behavior. Add a fallback to the first selected metric to preserve the old contract. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Partition charts lose deterministic default metric ordering. - ⚠️ Existing saved charts may display rows in new order. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In the legacy backend, the shared time-series query logic (used by `PartitionViz`) is implemented in `superset/viz.py:988-1007`; there `query_obj = super().query_obj()` is followed by `sort_by = self.form_data.get("timeseries_limit_metric") or utils.get_first_metric_name(query_obj.get("metrics") or [])` (lines 5-7), and if `sort_by` is truthy `query_obj["orderby"] = [(sort_by, is_asc)]` is always set (line 13), giving a default sort by the first metric when no explicit sort metric is configured. 2. With this PR, the Partition Chart now uses the frontend `buildQuery` in `superset-frontend/plugins/legacy-plugin-chart-partition/src/buildQuery.ts:33-59` via the plugin registration in `superset-frontend/plugins/legacy-plugin-chart-partition/src/index.ts:38-46`, so backend `PartitionViz.query_obj` is no longer used to build queries for this visualization. 3. In the new `buildQuery`, `sortByMetric` is calculated solely from `timeseries_limit_metric` at lines 38-40 using `ensureIsArray(timeseries_limit_metric)[0]`, and there is no fallback to the first metric from `baseQueryObject.metrics`; if a user configures a Partition Chart with one or more metrics but leaves `timeseries_limit_metric` unset (the default for the `timeseries_limit_metric` control defined in `src/controlPanel.tsx:4-5`), then `sortByMetric` is undefined and the `if (sortByMetric)` block at line 41 is skipped entirely. 4. As a result, the built query object keeps the original metrics list and leaves `orderby` untouched (line 55), so the chart executes without the deterministic metric-based ordering that the legacy backend supplied by default, changing the row ordering for existing Partition charts that previously relied on this implicit first-metric sort once they are migrated to the v1 chart data API. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4b3ec8c9a9744e6eb0a6742910d9abb8&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=4b3ec8c9a9744e6eb0a6742910d9abb8&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/legacy-plugin-chart-partition/src/buildQuery.ts **Line:** 38:40 **Comment:** *Api Mismatch: This misses the legacy fallback to the first metric when no explicit sort metric is provided. As a result, queries that previously had deterministic metric-based ordering now run without the expected default sort behavior. Add a fallback to the first selected metric to preserve the old contract. 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%2F41729&comment_hash=9721ef8045aaaea7694417ccb04339e1f3c762f174bc6ebc745a2ccdfefe18b1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41729&comment_hash=9721ef8045aaaea7694417ccb04339e1f3c762f174bc6ebc745a2ccdfefe18b1&reaction=dislike'>👎</a> ########## superset-frontend/plugins/legacy-plugin-chart-partition/src/transformData.ts: ########## @@ -0,0 +1,394 @@ +/** + * 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 { DTTM_ALIAS } from '@superset-ui/core'; + +export interface PartitionNode { + name: unknown; + val?: number | null; + children: PartitionNode[]; +} + +type Row = Record<string, unknown>; + +interface Options { + groupbyLabels: string[]; + metricLabels: string[]; + timeSeriesOption?: string; + rollingType?: string; + rollingPeriods?: number; + minPeriods?: number; + contribution?: boolean; +} + +const numeric = (value: unknown): number | null => + typeof value === 'number' && !Number.isNaN(value) ? value : null; + +const sortedUnique = (values: unknown[]): unknown[] => + Array.from(new Set(values)).sort((a, b) => { + if (a === b) return 0; + if (typeof a === 'number' && typeof b === 'number') return a - b; + return String(a) < String(b) ? -1 : 1; + }); + +/** pandas-style aggregate: sum skips nulls, mean of nothing is null */ +const aggregate = (rows: Row[], metric: string, op: 'sum' | 'mean') => { + let total = 0; + let count = 0; + rows.forEach(row => { + const value = numeric(row[metric]); + if (value != null) { + total += value; + count += 1; + } + }); + if (op === 'mean') { + return count === 0 ? null : total / count; + } + return total; +}; + +/** + * Ports PartitionViz.levels_for + nest_values: aggregate each metric at + * every prefix of the group hierarchy and nest the values top-down. The + * `groups` list may include the timestamp column for the plain + * time-series option, exactly like the backend prepended DTTM_ALIAS. + */ +function nestAggregates( + records: Row[], + groups: string[], + metricLabels: string[], + op: 'sum' | 'mean', +): PartitionNode[] { + const nest = ( + metric: string, + rows: Row[], + level: number, + dims: unknown[], + ): PartitionNode[] => { + if (level > groups.length - 1) { + return []; + } + const column = groups[level]; + return sortedUnique(rows.map(row => row[column])).map(value => { + const subset = rows.filter(row => row[column] === value); + return { + // deeper levels carry the dim path in the name, like the backend + name: level === 0 ? value : [...dims, value], + val: aggregate(subset, metric, op), + children: nest(metric, subset, level + 1, [...dims, value]), + }; + }); + }; + return metricLabels.map(metric => ({ + name: metric, + val: aggregate(records, metric, op), + children: nest(metric, records, 0, []), + })); +} + +/** + * Ports PartitionViz.levels_for_diff + nest_values: compare the last + * time grain against the first one at every hierarchy prefix. + */ +function nestPointComparison( + records: Row[], + groups: string[], + metricLabels: string[], + timeOp: 'point_diff' | 'point_factor' | 'point_percent', +): PartitionNode[] { + const times = sortedUnique(records.map(row => row[DTTM_ALIAS])) as number[]; + const since = times[0]; + const until = times[times.length - 1]; Review Comment: **Suggestion:** Point-comparison logic assumes at least one timestamp, but empty query results produce an empty `times` array. That makes `since`/`until` undefined and later arithmetic can yield invalid values (for example `0/0` -> `NaN`) instead of a clean empty result. Add an early return for empty records (or empty time buckets) before computing comparisons. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Point comparison mode returns NaN when no rows. - ⚠️ Charts may render incorrectly for empty time ranges. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. PartitionChartPlugin uses `transformProps` at `superset-frontend/plugins/legacy-plugin-chart-partition/src/transformProps.ts:28-84`, which reads `queriesData[0].data` and, when it is an array, passes it into `transformData` at lines 52-64 along with `timeSeriesOption` derived from `formData`. 2. When the user selects one of the point-comparison time-series options (`'point_diff'`, `'point_factor'`, or `'point_percent'`) from the `time_series_option` control defined in `superset-frontend/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx:55-73`, `transformData` dispatches into `nestPointComparison(records, groupbyLabels, metricLabels, timeSeriesOption)` via the branch at `src/transformData.ts:372-382`. 3. If the query returns no rows for the selected filters or time range (for example because the date range contains no data), `records` is an empty array; inside `nestPointComparison` at `src/transformData.ts:109-179`, `times` is computed from the DTTM column at line 115 and becomes an empty array, so `since = times[0]` (line 116) and `until = times[times.length - 1]` (line 117) both evaluate to `undefined`. 4. Later in the same function, the top-level totals are computed using `aggregate(records.filter(row => row[DTTM_ALIAS] === until), metric, 'sum')` and the corresponding `since` call at lines 163-172; with `records` empty, both aggregates return `0`, and `topCompare` at lines 120-124 computes `a / b` or `a / b - 1`, which for the factor and percent modes is `0/0` and yields `NaN`, causing `PartitionNode.val` fields in the returned tree to contain invalid `NaN` values instead of the expected empty result when there are no matching time buckets. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cc2d87b2e9b248c4a291dc9272392166&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=cc2d87b2e9b248c4a291dc9272392166&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/legacy-plugin-chart-partition/src/transformData.ts **Line:** 115:117 **Comment:** *Logic Error: Point-comparison logic assumes at least one timestamp, but empty query results produce an empty `times` array. That makes `since`/`until` undefined and later arithmetic can yield invalid values (for example `0/0` -> `NaN`) instead of a clean empty result. Add an early return for empty records (or empty time buckets) before computing comparisons. 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%2F41729&comment_hash=05b845d9b2e698e3534f8bc52199887d6e5db6dbdd9a55404648b0ce46332e65&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41729&comment_hash=05b845d9b2e698e3534f8bc52199887d6e5db6dbdd9a55404648b0ce46332e65&reaction=dislike'>👎</a> ########## superset-frontend/plugins/legacy-plugin-chart-partition/src/transformData.ts: ########## @@ -0,0 +1,394 @@ +/** + * 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 { DTTM_ALIAS } from '@superset-ui/core'; + +export interface PartitionNode { + name: unknown; + val?: number | null; + children: PartitionNode[]; +} + +type Row = Record<string, unknown>; + +interface Options { + groupbyLabels: string[]; + metricLabels: string[]; + timeSeriesOption?: string; + rollingType?: string; + rollingPeriods?: number; + minPeriods?: number; + contribution?: boolean; +} + +const numeric = (value: unknown): number | null => + typeof value === 'number' && !Number.isNaN(value) ? value : null; + +const sortedUnique = (values: unknown[]): unknown[] => + Array.from(new Set(values)).sort((a, b) => { + if (a === b) return 0; + if (typeof a === 'number' && typeof b === 'number') return a - b; + return String(a) < String(b) ? -1 : 1; + }); + +/** pandas-style aggregate: sum skips nulls, mean of nothing is null */ +const aggregate = (rows: Row[], metric: string, op: 'sum' | 'mean') => { + let total = 0; + let count = 0; + rows.forEach(row => { + const value = numeric(row[metric]); + if (value != null) { + total += value; + count += 1; + } + }); + if (op === 'mean') { + return count === 0 ? null : total / count; + } + return total; +}; + +/** + * Ports PartitionViz.levels_for + nest_values: aggregate each metric at + * every prefix of the group hierarchy and nest the values top-down. The + * `groups` list may include the timestamp column for the plain + * time-series option, exactly like the backend prepended DTTM_ALIAS. + */ +function nestAggregates( + records: Row[], + groups: string[], + metricLabels: string[], + op: 'sum' | 'mean', +): PartitionNode[] { + const nest = ( + metric: string, + rows: Row[], + level: number, + dims: unknown[], + ): PartitionNode[] => { + if (level > groups.length - 1) { + return []; + } + const column = groups[level]; + return sortedUnique(rows.map(row => row[column])).map(value => { + const subset = rows.filter(row => row[column] === value); + return { + // deeper levels carry the dim path in the name, like the backend + name: level === 0 ? value : [...dims, value], + val: aggregate(subset, metric, op), + children: nest(metric, subset, level + 1, [...dims, value]), + }; + }); + }; + return metricLabels.map(metric => ({ + name: metric, + val: aggregate(records, metric, op), + children: nest(metric, records, 0, []), + })); +} + +/** + * Ports PartitionViz.levels_for_diff + nest_values: compare the last + * time grain against the first one at every hierarchy prefix. + */ +function nestPointComparison( + records: Row[], + groups: string[], + metricLabels: string[], + timeOp: 'point_diff' | 'point_factor' | 'point_percent', +): PartitionNode[] { + const times = sortedUnique(records.map(row => row[DTTM_ALIAS])) as number[]; + const since = times[0]; + const until = times[times.length - 1]; + + // top-level comparison has no fill: a plain a-b / a/b / a/b-1 + const topCompare = (a: number, b: number) => { + if (timeOp === 'point_diff') return a - b; + if (timeOp === 'point_factor') return a / b; + return a / b - 1; + }; + // grouped comparisons treat missing sides as 0 (pandas fill_value=0) + const groupedCompare = (a: number | null, b: number | null) => { + const u = a ?? 0; + const s = b ?? 0; + if (timeOp === 'point_diff') return u - s; + if (timeOp === 'point_factor') return u / s; + return u / s - 1; + }; + + const nest = ( + metric: string, + rows: Row[], + level: number, + dims: unknown[], + ): PartitionNode[] => { + if (level > groups.length - 1) { + return []; + } + const column = groups[level]; + return sortedUnique(rows.map(row => row[column])).map(value => { + const subset = rows.filter(row => row[column] === value); + const untilRows = subset.filter(row => row[DTTM_ALIAS] === until); + const sinceRows = subset.filter(row => row[DTTM_ALIAS] === since); + const untilValue = untilRows.length + ? aggregate(untilRows, metric, 'sum') + : null; + const sinceValue = sinceRows.length + ? aggregate(sinceRows, metric, 'sum') + : null; + return { + name: level === 0 ? value : [...dims, value], + val: groupedCompare(untilValue, sinceValue), + children: nest(metric, subset, level + 1, [...dims, value]), + }; + }); + }; + + return metricLabels.map(metric => { + const untilTotal = aggregate( + records.filter(row => row[DTTM_ALIAS] === until), + metric, + 'sum', + ) as number; + const sinceTotal = aggregate( + records.filter(row => row[DTTM_ALIAS] === since), + metric, + 'sum', + ) as number; + return { + name: metric, + val: topCompare(untilTotal, sinceTotal), + children: nest(metric, records, 0, []), + }; + }); +} + +/** apply_rolling equivalent over a time-indexed value list */ +function applyRolling( + values: (number | null)[], + rollingType: string | undefined, + rollingPeriods: number, + minPeriods: number, +): (number | null)[] { + let result = values; + if ( + rollingType && + ['mean', 'sum', 'std'].includes(rollingType) && + rollingPeriods + ) { + result = values.map((_, index) => { + const start = Math.max(0, index - rollingPeriods + 1); + const window = values + .slice(start, index + 1) + .filter((v): v is number => v != null); + if (window.length < Math.max(minPeriods, 1)) { + return null; + } + const sum = window.reduce((acc, v) => acc + v, 0); + if (rollingType === 'sum') return sum; + const mean = sum / window.length; + if (rollingType === 'mean') return mean; + if (window.length < 2) return null; + const variance = + window.reduce((acc, v) => acc + (v - mean) ** 2, 0) / + (window.length - 1); + return Math.sqrt(variance); + }); + } else if (rollingType === 'cumsum') { + let running = 0; + result = values.map(value => { + running += value ?? 0; + return running; + }); + } + return result; +} + +/** + * Ports PartitionViz.levels_for_time + nest_procs for the "period + * analysis" option: pivot each hierarchy prefix per timestamp (sum, + * missing filled with 0), apply the rolling window and contribution the + * way process_data did, and nest as metric -> time -> group levels. + */ +function nestProcs( + records: Row[], + groups: string[], + options: Options, +): PartitionNode[] { + const { + metricLabels, + rollingType, + rollingPeriods = 0, + minPeriods = 0, + contribution, + } = options; + const allTimes = sortedUnique(records.map(row => row[DTTM_ALIAS])); + // rolling windows may trim the leading periods + const times = minPeriods ? allTimes.slice(minPeriods) : allTimes; + + // value series per (level, metric, group tuple) + const seriesFor = ( + metric: string, + rows: Row[], + ): Map<unknown, number | null> => { + const perTime = new Map<unknown, number | null>(); + allTimes.forEach(time => { + const timeRows = rows.filter(row => row[DTTM_ALIAS] === time); + // pivot fill_value=0: missing combinations become 0 + perTime.set( + time, + timeRows.length ? aggregate(timeRows, metric, 'sum') : 0, + ); + }); + let values: (number | null)[] = allTimes.map( + time => perTime.get(time) ?? 0, + ); + values = applyRolling(values, rollingType, rollingPeriods, minPeriods); + const result = new Map<unknown, number | null>(); + allTimes.forEach((time, index) => result.set(time, values[index])); + return result; + }; + + // contribution normalizes each timestamp across every column of the + // same level (all metric x group-tuple combinations) + const buildLevel = ( + level: number, + ): Map<string, Map<unknown, number | null>> => { + const columns = new Map<string, Row[]>(); + metricLabels.forEach(metric => { + if (level === 0) { + columns.set(JSON.stringify([metric]), records); + } else { + const tuples = new Map<string, Row[]>(); + records.forEach(row => { + const key = JSON.stringify([ + metric, + ...groups.slice(0, level).map(label => row[label]), + ]); + tuples.set(key, [...(tuples.get(key) ?? []), row]); Review Comment: **Suggestion:** This row accumulation pattern copies the full array on every insert, creating quadratic time and memory behavior for larger datasets. In period-analysis mode this can become a client-side bottleneck. Mutate/push into a pre-existing array (or build grouped buckets once) to avoid repeated full-array cloning. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Advanced analytics partition charts slow on large datasets. - ⚠️ Client transform becomes quadratic in per-group row count. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The `time_series_option` control in `superset-frontend/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx:75-78` provides an `'Advanced Analytics'` option with value `'adv_anal'`; when selected, this value is passed through `formData.timeSeriesOption` into `transformProps` at `src/transformProps.ts:41-47`. 2. `transformProps` calls `transformData(rawData, { ... timeSeriesOption })` at lines 52-64; inside `transformData` in `src/transformData.ts:354-394`, when `timeSeriesOption === 'adv_anal'` the function takes the advanced analytics branch and returns `nestProcs(records, groupbyLabels, options)` at lines 384-385. 3. Within `nestProcs`, the helper `buildLevel` at `src/transformData.ts:269-305` constructs grouped time-series columns; for levels greater than zero, it iterates over every row in `records` at lines 278-283, computing a `key` per row and then executing `tuples.set(key, [...(tuples.get(key) ?? []), row]);` at line 283 to accumulate rows for that group. 4. This pattern clones the entire existing array for the group on every insert, so for a group with n rows, building its bucket costs O(n²); when advanced analytics is used on larger datasets or with many rows per group, this O(n²) behavior in `nestProcs` significantly increases client-side transform time and memory usage before the chart is rendered. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=42e381d2b3464659bee92aa803a903e3&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=42e381d2b3464659bee92aa803a903e3&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/legacy-plugin-chart-partition/src/transformData.ts **Line:** 283:283 **Comment:** *Performance: This row accumulation pattern copies the full array on every insert, creating quadratic time and memory behavior for larger datasets. In period-analysis mode this can become a client-side bottleneck. Mutate/push into a pre-existing array (or build grouped buckets once) to avoid repeated full-array cloning. 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%2F41729&comment_hash=b8bacb49f2323d3693f766bb67da0eede6a70c886b473c50ed590fa2e12ce50a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41729&comment_hash=b8bacb49f2323d3693f766bb67da0eede6a70c886b473c50ed590fa2e12ce50a&reaction=dislike'>👎</a> ########## superset-frontend/plugins/legacy-plugin-chart-partition/src/buildQuery.ts: ########## @@ -0,0 +1,59 @@ +/** + * 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 { + buildQueryContext, + ensureIsArray, + getMetricLabel, + QueryFormData, + QueryFormMetric, + QueryFormOrderBy, +} from '@superset-ui/core'; + +/** + * Mirrors the legacy PartitionViz.query_obj: a query grouped by all the + * hierarchy levels, timeseries only when the time-series option needs + * per-timestamp data, with the legacy sort-metric handling. + */ +export default function buildQuery(formData: QueryFormData) { + const { time_series_option, timeseries_limit_metric, order_desc } = formData; + return buildQueryContext(formData, baseQueryObject => { + let metrics: QueryFormMetric[] = ensureIsArray(baseQueryObject.metrics); + const orderby: QueryFormOrderBy[] = []; + const sortByMetric = ensureIsArray( + timeseries_limit_metric as QueryFormMetric | QueryFormMetric[], + )[0]; + if (sortByMetric) { + const sortByLabel = getMetricLabel(sortByMetric); + if (!metrics.some(metric => getMetricLabel(metric) === sortByLabel)) { + metrics = [...metrics, sortByMetric]; + } + if (order_desc) { + orderby.push([sortByMetric, !order_desc]); + } Review Comment: **Suggestion:** The sort direction toggle is currently ignored for ascending mode because `orderby` is only set when descending is enabled. When users uncheck “Sort Descending”, the query drops ordering entirely instead of applying ascending order, so result ordering becomes inconsistent. Always set `orderby` whenever a sort metric exists, using the boolean direction to choose asc/desc. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Partition chart ascending sort option has no effect. - ⚠️ Users see unsorted partitions despite choosing sort metric. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open or create a Partition Chart in Explore; the chart is implemented by PartitionChartPlugin, which wires `loadBuildQuery` to `./buildQuery` in `superset-frontend/plugins/legacy-plugin-chart-partition/src/index.ts:38-46`, so all partition queries go through `buildQuery()`. 2. Configure the chart with a `timeseries_limit_metric` and uncheck the `order_desc` control (Sort descending) defined in the control panel at `superset-frontend/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx:4-5`, so the formData sent to the frontend has `timeseries_limit_metric` set and `order_desc: false`. 3. When the chart runs, the frontend calls `buildQuery(formData)` in `superset-frontend/plugins/legacy-plugin-chart-partition/src/buildQuery.ts:33-59`; within this function, `sortByMetric` is populated from `timeseries_limit_metric` at lines 38-40 and the code enters the `if (sortByMetric)` block at line 41, but the inner `if (order_desc)` at line 46 evaluates to false when `order_desc` is false, so the `orderby.push([sortByMetric, !order_desc])` at line 47 is skipped and `orderby` remains empty. 4. The returned query object therefore has `orderby` undefined (line 55), so the backend executes the SQL without any explicit ORDER BY on the chosen metric; this silently ignores the user’s ascending sort request, unlike the legacy backend behavior in `superset/viz.py:988-1007` where `query_obj["orderby"]` is always set from the sort metric and `order_desc` regardless of sort direction. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3e0e2a9832f24b68a739ad8ab23f14c6&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=3e0e2a9832f24b68a739ad8ab23f14c6&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/legacy-plugin-chart-partition/src/buildQuery.ts **Line:** 46:48 **Comment:** *Incorrect Condition Logic: The sort direction toggle is currently ignored for ascending mode because `orderby` is only set when descending is enabled. When users uncheck “Sort Descending”, the query drops ordering entirely instead of applying ascending order, so result ordering becomes inconsistent. Always set `orderby` whenever a sort metric exists, using the boolean direction to choose asc/desc. 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%2F41729&comment_hash=9aa54713fa488f214e928f57adc9a4954a6959aee2639ec3a554691273099e65&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41729&comment_hash=9aa54713fa488f214e928f57adc9a4954a6959aee2639ec3a554691273099e65&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]
