rusackas commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4145477330
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -483,6 +505,35 @@ export default function transformProps(
);
const isMultiSeries = groupBy.length || metrics?.length > 1;
+ // `x_axis_sort` stores either a series aggregate (a `SortSeriesType`) or
+ // the label of the x-axis column or of a metric. A single-series chart is
+ // sorted by the backend sort operator; with several series the rows are
+ // ordered here instead: by axis value, by an aggregate over every series,
+ // or by the sum of the chosen metric's columns. The field is carried
+ // through as-is, so a column or metric that happens to be named like an
+ // aggregate is never guessed at.
+ const sortSeriesTypes = new Set<string>(Object.values(SortSeriesType));
+ const resolveXAxisSortSeries = (): XAxisSortSeries | undefined => {
+ if (!isMultiSeries || typeof xAxisSort !== 'string') {
+ return undefined;
+ }
+ if (sortSeriesTypes.has(xAxisSort)) {
+ return xAxisSort as SortSeriesType;
+ }
+ if (
+ xAxisSort === xAxisLabel ||
+ xAxisSort === getXAxisLabel(chartProps.rawFormData)
+ ) {
+ return SortSeriesType.Name;
+ }
+ const dataColumns = new Set(Object.keys(rebasedData[0] ?? {}));
Review Comment:
Good catch. Added a fallback: when it's the sole displayed metric and
truncate_metric drops the label, sums every remaining series column instead of
finding nothing. Test added.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -557,8 +608,10 @@ export default function transformProps(
isHorizontal,
sortSeriesType,
sortSeriesAscending,
- xAxisSortSeries: isMultiSeries ? xAxisSort : undefined,
- xAxisSortSeriesAscending: isMultiSeries ? xAxisSortAsc : undefined,
+ xAxisSortSeries,
Review Comment:
Good catch, fixed. Rederiving the percentage-threshold values from the
already-sorted totals instead of the pre-sort array. Test added with a
reordered stacked case.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -64,6 +68,73 @@ describe('Timeseries buildQuery', () => {
expect(query.metrics).toEqual(['bar', 'baz']);
});
+ test('should query the sort-only limit metric with dimensions so its pivoted
columns can order the axis', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ metrics: ['count'],
+ x_axis: 'genre',
+ groupby: ['platform'],
+ timeseries_limit_metric: 'na_sales',
+ x_axis_sort: 'na_sales',
+ x_axis_sort_asc: false,
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['count', 'na_sales']);
+ const pivot = (query.post_processing || []).find(
+ op => op?.operation === 'pivot',
+ );
Review Comment:
Fair, switched both spots to isPostProcessingPivot.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -64,6 +68,73 @@ describe('Timeseries buildQuery', () => {
expect(query.metrics).toEqual(['bar', 'baz']);
});
+ test('should query the sort-only limit metric with dimensions so its pivoted
columns can order the axis', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ metrics: ['count'],
+ x_axis: 'genre',
+ groupby: ['platform'],
+ timeseries_limit_metric: 'na_sales',
+ x_axis_sort: 'na_sales',
+ x_axis_sort_asc: false,
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['count', 'na_sales']);
+ const pivot = (query.post_processing || []).find(
+ op => op?.operation === 'pivot',
+ );
+ expect(pivot?.options).toMatchObject({
Review Comment:
Added the assertion.
--
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]
