sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4155045997
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -463,14 +465,34 @@ 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 &&
+ parts.length > 1 &&
Review Comment:
With two dimensions and Truncate Metric enabled, a displayed series such as
`US, mobile` has `label_map` parts `["US", "mobile"]`; if the sort-only metric
is named `US`, this lookup treats that displayed series as sort-only and
removes it from the chart and stacked totals. Could this distinguish a metric
prefix from a truncated dimension tuple using the dimension count?
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts:
##########
@@ -26,11 +26,15 @@ import {
export function extractExtraMetrics(
formData: QueryFormData,
): QueryFormMetric[] {
- const { groupby, timeseries_limit_metric, x_axis_sort, metrics } = formData;
+ const { timeseries_limit_metric, x_axis_sort, metrics } = formData;
const extra_metrics: QueryFormMetric[] = [];
- const limitMetric = ensureIsArray(timeseries_limit_metric)[0];
+ const [limitMetric] = ensureIsArray(timeseries_limit_metric);
+ // The "Sort By" limit metric is only queried when the axis is sorted by it
+ // and it is not already a value metric. This applies with dimensions too:
+ // the pivot then yields one `<limit metric>, <dimension values>` column
+ // per series, which the chart sums client-side to order the axis (the
+ // backend sort operator only handles the single-series case).
if (
- !(groupby || []).length &&
limitMetric &&
getMetricLabel(limitMetric) === x_axis_sort &&
Review Comment:
If the Sort By metric is labeled `sum`, selecting the Total value aggregate
now adds that hidden metric to a grouped query, and `sortRows` includes it in
the aggregate ordering despite excluding it from displayed totals (displayed
totals 10/20 plus hidden values 100/0 sort as 10 then 20). Could aggregate
selections avoid querying or counting a colliding sort-only metric?
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts:
##########
@@ -26,11 +26,15 @@ import {
export function extractExtraMetrics(
formData: QueryFormData,
): QueryFormMetric[] {
- const { groupby, timeseries_limit_metric, x_axis_sort, metrics } = formData;
+ const { timeseries_limit_metric, x_axis_sort, metrics } = formData;
const extra_metrics: QueryFormMetric[] = [];
- const limitMetric = ensureIsArray(timeseries_limit_metric)[0];
+ const [limitMetric] = ensureIsArray(timeseries_limit_metric);
+ // The "Sort By" limit metric is only queried when the axis is sorted by it
+ // and it is not already a value metric. This applies with dimensions too:
+ // the pivot then yields one `<limit metric>, <dimension values>` column
+ // per series, which the chart sums client-side to order the axis (the
+ // backend sort operator only handles the single-series case).
if (
- !(groupby || []).length &&
limitMetric &&
getMetricLabel(limitMetric) === x_axis_sort &&
!metrics?.some(metric => getMetricLabel(metric) === x_axis_sort)
Review Comment:
A grouped scatter chart with value metric `A`, dot-size metric `B`, and both
Sort By controls set to `B` now gets query metrics `[A, B, B]`: the base query
already includes `size`, but this check only considers displayed metrics.
Superset rejects that query with “Duplicate column/metric labels”; could the
extra metric be deduplicated against all metrics already queried, with a
grouped-scatter regression case?
--
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]
