rusackas commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4289137412
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts:
##########
@@ -36,7 +41,16 @@ export const timeComparePivotOperator: PostProcessingFactory<
if (isTimeComparison(formData, queryObject) && xAxisLabel) {
const aggregates = Object.fromEntries(
- [...metricOffsetMap.values(), ...metricOffsetMap.keys()].map(metric => [
+ [
+ ...metricOffsetMap.values(),
+ ...metricOffsetMap.keys(),
+ // The "Sort By" metric is queried alongside the value metrics when
+ // the axis is sorted by it (see extractExtraMetrics), and the pivot
+ // has to keep it for the client-side sort. Only its base label is
+ // kept: the query also fetches a `<metric>__<offset>` column for it,
+ // but that variant is never rendered, so the pivot drops it.
+ ...extractExtraMetrics(formData).map(getMetricLabel),
Review Comment:
Good catch, fixed. The sort-only extra metric gets pivoted and offset-mapped
here same as any other, but renameOperator wasn't counting it, so when it was
labeled like the time offset itself its renamed column collided with its own
unrenamed base column. Counts it now, with a test for that exact collision.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -483,6 +520,58 @@ 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] ?? {}));
+ // `truncate_metric` drops the metric label from its own pivoted columns
+ // when it is the sole displayed metric (see the comment on
+ // `pivotedColumnsOf`), so that lookup finds nothing when a chart's only
+ // metric is also the chosen sort target. With a single displayed metric,
+ // every remaining series column is that metric's own pivoted value, so
+ // fall back to summing them directly.
+ const soleValueMetricLabel = isMultiSeries
+ ? ensureIsArray(metrics).length === 1
+ ? getMetricLabel(ensureIsArray(metrics)[0])
+ : undefined
+ : undefined;
+ const isSoleValueMetricSort =
+ isDefined(soleValueMetricLabel) &&
+ (verboseMap[soleValueMetricLabel!] ?? soleValueMetricLabel) ===
+ (verboseMap[xAxisSort] ?? xAxisSort);
+ const truncatedMetricColumns =
+ isSoleValueMetricSort && !pivotedColumnsOf(xAxisSort).length
+ ? Object.keys(rebasedData[0] ?? {}).filter(
+ column =>
+ column !== xAxisLabel && !extraMetricLabels.includes(column),
+ )
+ : [];
+ const sumOfColumns = [
+ verboseMap[xAxisSort] ?? xAxisSort,
+ ...pivotedColumnsOf(xAxisSort),
Review Comment:
Good catch, fixed. With a Difference/Percentage/Ratio comparison the compare
operator drops the metric's own base column and renameOperator replaces it with
the renamed `<metric>, <offset>` column, so the sort lookup needed a fallback
to that renamed column when the base one's gone. Added a case for the
no-dimensions scenario, that's as far as I traced it so far.
--
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]
