sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4173683555
##########
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:
With two displayed metrics and a Difference comparison, selecting metric `A`
for axis sorting leaves the categories in query order: comparison processing
removes `A`, and rename produces prefixes such as `A, 1 week ago`, so this
lookup finds no columns and the single-metric fallback cannot apply. Could
metric selections resolve their comparison columns too, with a regression where
the expected order differs from the query order?
##########
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:
With one displayed metric, an actual-values time comparison, and a sort-only
metric labeled `1 year ago`, retaining that metric makes the subsequent rename
of `count__1 year ago` to `1 year ago` fail with “Label already exists,” so the
chart cannot render. Could the sort metric retain a collision-safe identity
through pivot and rename, with a regression exercising both operators together?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -463,14 +465,38 @@ export default function transformProps(
rebaseToPercentChange(forecastRebasedData, xAxisLabel || DTTM_ALIAS)
: forecastRebasedData;
const isHorizontal = orientation === OrientationType.Horizontal;
+ // With dimensions set, the pivot splits a metric into one
+ // `<metric>, <dimension values>` column per series. `label_map` lists each
+ // flattened column as `[metric, ...dimension values]`, so a metric's
+ // columns are resolved through it rather than by matching on the column
+ // name. A single-metric chart with `truncate_metric` drops the metric part
+ // from its value columns, which is fine: only sort-only metrics need
+ // resolving, and those always keep it.
+ const pivotedColumnsOf = (metricLabel: string): string[] =>
+ Object.entries(labelMap)
+ .filter(
+ ([column, parts]) =>
+ column !== metricLabel &&
+ // a metric-prefixed column carries the metric plus one part per
+ // dimension; a truncated dimension tuple has no metric part and
+ // must not be mistaken for one when a dimension value reads like
+ // the metric label
+ parts.length === groupBy.length + 1 &&
+ parts[0] === metricLabel &&
+ !derivedComparisonSeries.has(column),
+ )
+ .map(([column]) => column);
// rebasedData's keys have already been through rebaseForecastDatum, which
// renames a key to its verboseMap entry when one is configured for that
// metric. extraMetricLabels must be mapped the same way, or a sort-only
// metric with a verbose_name set would silently fail to match here (and in
- // extractSeries below, which has the same requirement).
+ // extractSeries below, which has the same requirement). The pivoted columns
+ // keep their raw names (verbose mapping only applies to an exact metric
+ // label), and must be excluded too so a sort-only metric is neither
+ // rendered as series nor counted in stacked totals.
const extraMetricLabels = extractExtraMetrics(chartProps.rawFormData)
.map(getMetricLabel)
- .map(label => verboseMap[label] ?? label);
+ .flatMap(label => [verboseMap[label] ?? label,
...pivotedColumnsOf(label)]);
Review Comment:
The grouped-scatter test never selects the size metric for axis sorting, so
it would still pass if those columns were stripped again and every dot reverted
to a fixed size. Could we add that combination to the `transformProps` test and
assert sorted categories plus distinct minimum/maximum `symbolSize` values?
--
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]
