sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4289490497
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/renameOperator.ts:
##########
@@ -87,7 +105,7 @@ export const renameOperator:
PostProcessingFactory<PostProcessingRename> = (
ComparisonType.Percentage,
ComparisonType.Ratio,
].includes(formData.comparison_type) &&
- metrics.length === 1 &&
+ metricCountWithExtras === 1 &&
Review Comment:
With one displayed metric, a dimension, `truncate_metric` on and a Sort By
metric set, `metricCountWithExtras` is 2, so this truncation rename is skipped
and the value columns come out as `count, PS4` instead of `PS4` (before this
change the rename still ran). `transformProps` still treats a single displayed
metric as having no metric prefix, so with "Split stack by" platform it reads
`count` for every series and merges the separate stacks into one. Should the
extra-metric count apply only to the time-comparison naming above, keeping
`metrics.length === 1` here, with a `buildQuery` case that sets
`truncate_metric` plus a sort-only metric and asserts the `{ count: null }`
rename is still emitted?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -464,14 +467,60 @@ 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.
+ // `label_map` is built from the cached DataFrame's final columns, after
+ // every post-processing operator (including Prophet) has run, so a
+ // forecast's `<metric>, <dims>__yhat`/`__yhat_lower`/`__yhat_upper`
+ // columns have the same `[metric, ...dims]` shape and would otherwise be
+ // mistaken for one of the metric's own pivoted columns. Those are excluded
+ // the same way they are hidden from the rendered series (see
+ // extractForecastSeriesContext usage above), so the sort key never
+ // depends on the forecast trend or confidence bounds.
+ 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) &&
+ extractForecastSeriesContext(column).type ===
+ ForecastSeriesEnum.Observation,
Review Comment:
`pivotedColumnsOf` also feeds `extraMetricLabels`, so filtering Prophet
columns out here stops hiding them from rendering and stacked totals. With a
displayed `count`, a hidden sort-only `sales` metric and forecasting on,
`sales, US__yhat` (and its bounds) no longer match the sort-only exclusion
list, so they are drawn as series and added into the stacked totals. Could the
forecast filter apply only to the sort key, with the hidden-series list still
including every pivoted column of the sort-only metric, and a case asserting
the rendered series exclude them?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -484,6 +533,95 @@ 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 baseColumn = verboseMap[xAxisSort] ?? xAxisSort;
+ // A Difference/Percentage/Ratio comparison drops the metric's own base
+ // column (see the `compare` post-processing operator's
+ // `drop_original_columns`) and replaces it with one
+ // `<metric>, <offset>` column per configured offset (see
+ // renameOperator), so when that metric is also the sort target, its
+ // base column is gone from `dataColumns` and the renamed comparison
+ // column(s) have to be summed instead. Guarded on the base column being
+ // missing so an "actual values" comparison, which keeps it, is
+ // unaffected. Scoped to no dimensions: combining a dimension pivot with
+ // a comparison produces a column shape not accounted for here, a
+ // pre-existing limitation this does not attempt to fix.
+ const comparisonColumnsOf = (metricLabel: string): string[] =>
+ groupBy.length === 0 && !dataColumns.has(metricLabel)
+ ? ensureIsArray(timeCompare).map(offset => `${metricLabel}, ${offset}`)
+ : [];
+ const sumOfColumns = [
+ baseColumn,
+ ...pivotedColumnsOf(xAxisSort),
+ ...truncatedMetricColumns,
+ ...comparisonColumnsOf(baseColumn),
+ ].filter(column => dataColumns.has(column));
+ if (!sumOfColumns.length) {
+ return undefined;
+ }
+ // The ungrouped MIN/MAX of a metric equals the MIN/MAX of its
+ // per-dimension MIN/MAX values, the same way the ungrouped SUM equals
+ // the sum of its per-dimension sums, so a simple MIN/MAX metric rolls
+ // up the same way across the pivoted columns above. Other aggregates
+ // (AVG, COUNT DISTINCT, ...) are not decomposable this way from
+ // already-aggregated values and keep the Sum default (see
+ // XAxisSortBySumOfColumns), a known limitation for those.
+ const sortMetric = [...ensureIsArray(metrics), ...extraMetrics].find(
+ candidate => getMetricLabel(candidate) === xAxisSort,
+ );
+ const aggregation =
+ isAdhocMetricSimple(sortMetric) &&
+ (sortMetric.aggregate === 'MIN' || sortMetric.aggregate === 'MAX')
Review Comment:
The MIN branch of this rollup has no `transformProps`-level test: the only
new case covers MAX, and the existing `series.test.ts` MIN cases pass the
aggregate enum directly, so they never go through this metric lookup. Removing
the MIN mapping would leave every test green while a grouped chart falls back
to summing. Could we add a grouped `SIMPLE` MIN metric, ascending, with A =
`[0, 100]` and B = `[60, 60]` per series, asserting A then B (a sum would give
B then 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]
