sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4217189335
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -464,14 +466,49 @@ 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),
Review Comment:
With forecasting enabled on a grouped chart, Prophet's `<metric>,
<dims>__yhat`, `__yhat_lower` and `__yhat_upper` columns are in `label_map`
with the same shape as a metric-prefixed column, so they pass this filter. They
end up in `sumOfColumns` next to the observed values, which means the axis
order depends on the predictions and confidence bounds rather than the selected
metric. For example, with observed `sales` of 100 and 80 for two regions, the
forecast and band columns can flip them to 80 then 100, and changing the
confidence interval changes the order without any change to the data. Could the
sort lookup exclude the forecast-derived columns (separately from how they are
hidden from the series)?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -464,14 +466,49 @@ 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 &&
Review Comment:
This length check is what keeps a displayed column like `SUM(na_sales), US`
(two dimensions, `truncate_metric` on, first dimension value equal to the sort
metric label) from being treated as sort-only and dropped from the series and
stacked totals, but I don't see a test that exercises it. The `transformProps`
fixtures all use a single dimension, so reverting this to `parts.length > 1`
would keep every existing test green while hiding that real series again. Could
we add a two-dimension case where a dimension value matches the sort metric
label, asserting the two-part series still renders and counts toward the
stacked totals while the three-part sort-only columns stay hidden?
--
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]
