Copilot commented on code in PR #42372:
URL: https://github.com/apache/superset/pull/42372#discussion_r4153568287
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -39,6 +44,66 @@ const config: ControlPanelConfig = {
['time_grain_sqla'],
['groupby'],
['metric'],
+ [
+ {
+ name: 'x_axis_sort',
+ config: {
+ type: 'SelectControl',
Review Comment:
`shouldReset` is ignored because this renders the plain `SelectControl`;
only `XAxisSortControl` consumes that prop and calls `onChange(undefined)`
(`src/explore/components/controls/XAxisSortControl.tsx:30-37`). After sorting
by a metric and then changing the metric, the stale label therefore remains and
`buildQuery` treats it as a dataset column, producing `MIN(<old metric label>)`
and an unknown-column query failure. Render `XAxisSortControl` so the intended
reset actually executes.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/buildQuery.ts:
##########
@@ -17,22 +17,70 @@
* under the License.
*/
import {
+ AdhocMetricSimple,
buildQueryContext,
ensureIsArray,
+ getColumnLabel,
+ getMetricLabel,
+ QueryFormColumn,
QueryFormData,
+ QueryFormMetric,
+ QueryFormOrderBy,
} from '@superset-ui/core';
export default function buildQuery(formData: QueryFormData) {
- const { x_axis, granularity_sqla, groupby } = formData;
+ const {
+ x_axis,
+ granularity_sqla,
+ groupby,
+ x_axis_sort,
+ x_axis_sort_asc = true,
+ } = formData;
const columns = [
...ensureIsArray(x_axis || granularity_sqla),
...ensureIsArray(groupby),
];
- return buildQueryContext(formData, baseQueryObject => [
- {
- ...baseQueryObject,
- columns,
- orderby: columns?.map(column => [column, true]),
- },
- ]);
+ return buildQueryContext(formData, baseQueryObject => {
+ // Keep the category's rows contiguous by falling back to the x-axis (and
+ // breakdown) columns. This is also the ordering when no custom sort is
set.
+ const baseOrderby: QueryFormOrderBy[] = columns.map(
+ (column: QueryFormColumn) => [column, true],
+ );
+ if (!x_axis_sort) {
+ return [{ ...baseQueryObject, columns, orderby: baseOrderby }];
+ }
+
+ const ascending = !!x_axis_sort_asc;
+ const isGroupedColumn = columns.some(
+ (column: QueryFormColumn) => getColumnLabel(column) === x_axis_sort,
+ );
+ const isMetric = ensureIsArray(baseQueryObject.metrics).some(
+ metric => getMetricLabel(metric) === x_axis_sort,
+ );
+ // A sort key that is already a grouping column or a selected metric can be
+ // referenced by label. Anything else is a bare dataset column, which an
+ // aggregated query cannot ORDER BY unless it is itself aggregated: adding
+ // it to `columns` instead would put it in the GROUP BY and change the
grain
+ // of the query, inflating the row count until `row_limit` truncates it and
+ // the bars silently under-report (#42372). Wrapping it in MIN() orders the
+ // categories by that column without touching the grain.
+ const sortBy: QueryFormMetric =
+ isGroupedColumn || isMetric
+ ? x_axis_sort
+ : ({
+ expressionType: 'SIMPLE',
+ column: { column_name: x_axis_sort },
+ aggregate: 'MIN',
+ label: x_axis_sort,
+ hasCustomLabel: true,
+ } as AdhocMetricSimple);
Review Comment:
The PR description and testing notes still say that an unselected sort
column is auto-added to the query, but this implementation deliberately wraps
it in `MIN()` and leaves `columns` unchanged (as the updated test asserts).
Please update the PR text to describe the hidden aggregate approach rather than
the old grain-changing behavior.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Waterfall/controlPanel.sort.test.ts:
##########
@@ -0,0 +1,68 @@
+/**
+ * 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 config from '../../src/Waterfall/controlPanel';
+
+// Walk the control panel config to grab a control's config by name.
+const findControl = (name: string): any => {
+ for (const section of (config as any).controlPanelSections) {
Review Comment:
The new helper opts out of type checking for both the control lookup and the
config object. That hides contract mistakes such as returning `shouldReset` to
a component that does not support it—the functional bug in this PR passes these
tests for exactly that reason. Give the helper a narrow typed return shape and
traverse the typed `ControlPanelConfig` directly instead of using `any`.
--
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]
