aminghadersohi commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4376301780
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -464,14 +493,74 @@ 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,
+ { includeForecast = false }: { includeForecast?: boolean } = {},
+ ): 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:
Without dimensions `label_map` maps a SQL metric to its expression, so a
displayed SQL metric `Revenue` (`SUM(s)`) matches here when sorting by a
sort-only `SUM(s)` and is hidden: the chart renders no series. A metric has no
pivoted columns without dimensions.
```suggestion
groupBy.length > 0 &&
parts.length === groupBy.length + 1 &&
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -4303,6 +4303,60 @@ test('tooltip does not apply a metric currency format to
a grouped Percentage ti
expect(result).not.toContain('$ 0.25');
});
+test('tooltip does not apply a metric currency format to a Percentage
comparison labeled `<metric>, <offset>`', () => {
+ // A hidden sort-only metric makes renameOperator keep the metric prefix on
the
+ // comparison column, so the label_map entry leads with `<metric>, <offset>`.
+ const chartProps = createTestChartProps({
+ formData: {
+ metric: 'revenue',
+ metrics: ['revenue'],
+ groupby: ['region'],
+ richTooltip: true,
+ time_compare: ['1 year ago'],
+ timeCompare: ['1 year ago'],
+ comparison_type: ComparisonType.Percentage,
+ },
+ queriesData: [
+ createTestQueryData(
+ [
+ {
+ 'revenue, East': 100,
+ 'revenue, 1 year ago, East': 0.25,
+ __timestamp: BASE_TIMESTAMP,
+ },
+ ],
+ {
+ label_map: {
+ 'revenue, East': ['revenue', 'East'],
+ 'revenue, 1 year ago, East': ['revenue, 1 year ago', 'East'],
+ },
+ },
+ ),
+ ],
+ datasource: {
+ verboseMap: {},
+ columnFormats: {},
+ currencyFormats: {
+ revenue: { symbol: 'USD', symbolPosition: 'prefix' },
+ },
+ },
+ });
+
+ const { echartOptions } = transformProps(chartProps);
+ const { tooltip } = echartOptions as unknown as TooltipFormatterOptions;
+
+ const result = tooltip.formatter([
+ {
+ seriesId: 'revenue, 1 year ago, East',
+ seriesName: 'revenue, 1 year ago, East',
+ value: [BASE_TIMESTAMP, 0.25],
+ },
+ ]);
+
+ expect(result).toContain('25.00%');
+ expect(result).not.toContain('$ 0.25');
+});
Review Comment:
Nit: Nothing pins the ungrouped branch at `transformProps.ts:465`: removing
it breaks no existing test, and the Percentage tooltip then shows `$ 0.25`
instead of `25.00%`. A test for that case:
```suggestion
});
test('tooltip does not apply a metric currency format to an ungrouped
Percentage comparison with a sort-only metric', () => {
// Without dimensions the flat `revenue, 1 year ago` label is split into a
// two-part label_map tuple, so only the whole label marks it as derived.
const chartProps = createTestChartProps({
formData: {
metric: 'revenue',
metrics: ['revenue'],
groupby: [],
richTooltip: true,
timeseries_limit_metric: 'sales',
x_axis_sort: 'sales',
time_compare: ['1 year ago'],
timeCompare: ['1 year ago'],
comparison_type: ComparisonType.Percentage,
},
queriesData: [
createTestQueryData(
[
{
revenue: 100,
'revenue, 1 year ago': 0.25,
sales: 3,
__timestamp: BASE_TIMESTAMP,
},
],
{
label_map: {
revenue: ['revenue'],
'revenue, 1 year ago': ['revenue', '1 year ago'],
sales: ['sales'],
},
},
),
],
datasource: {
verboseMap: {},
columnFormats: {},
currencyFormats: {
revenue: { symbol: 'USD', symbolPosition: 'prefix' },
},
},
});
const { echartOptions } = transformProps(chartProps);
const { tooltip } = echartOptions as unknown as TooltipFormatterOptions;
const result = tooltip.formatter([
{
seriesId: 'revenue, 1 year ago',
seriesName: 'revenue, 1 year ago',
value: [BASE_TIMESTAMP, 0.25],
},
]);
expect(result).toContain('25.00%');
expect(result).not.toContain('$ 0.25');
});
```
##########
superset-frontend/packages/superset-ui-chart-controls/test/operators/contributionOperator.test.ts:
##########
@@ -52,3 +57,61 @@ test('should do contributionOperator', () => {
},
});
});
+
+const sortByFormData: SqlaFormData = {
+ ...formData,
+ metrics: ['count'],
+ groupby: ['platform'],
+ x_axis: 'ds',
+ contributionMode: 'row',
+ timeseries_limit_metric: 'sales',
+ x_axis_sort: 'sales',
+};
+const sortByQueryObject: QueryObject = {
+ ...queryObject,
+ metrics: ['count'],
+};
+
+test('should leave the sort-only metric out of the row contribution', () => {
+ expect(contributionOperator(sortByFormData, sortByQueryObject)).toEqual({
+ operation: 'contribution',
+ options: { orientation: 'row', columns: ['count'] },
+ });
+});
+
+test('should leave the sort-only metric out with time comparison values', ()
=> {
+ expect(
+ contributionOperator(
+ {
+ ...sortByFormData,
+ time_compare: ['1 year ago'],
+ comparison_type: ComparisonType.Values,
+ },
+ sortByQueryObject,
+ ['1 year ago'],
+ ),
+ ).toEqual({
+ operation: 'contribution',
+ options: {
+ orientation: 'row',
+ time_shifts: ['1 year ago'],
+ columns: ['count', 'count__1 year ago'],
+ },
+ });
+});
Review Comment:
Nit: No test reaches the Difference/Percentage/Ratio branch at
`contributionOperator.ts:63`, so a wrong derived name would only surface as a
backend `Referenced columns not available` error. A test for it:
```suggestion
});
test('should leave the sort-only metric out with a percentage comparison',
() => {
const percentageFormData = {
...sortByFormData,
time_compare: ['1 year ago'],
comparison_type: ComparisonType.Percentage,
};
expect(
contributionOperator(percentageFormData, sortByQueryObject)?.options,
).toEqual({
orientation: 'row',
columns: ['percentage__count__count__1 year ago'],
});
});
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -484,6 +573,105 @@ 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 { axisValue: true };
+ }
+ 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) &&
+ // forecast trend and bounds never drive the sort key
+ extractForecastSeriesContext(column).type ===
+ ForecastSeriesEnum.Observation,
+ )
+ : [];
+ 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)
Review Comment:
With two or more metrics, a dimension and a Difference/Percentage/Ratio
comparison, sorting by a displayed metric keeps query order: its columns read
`<metric>, <offset>, <dims>` and neither this nor `pivotedColumnsOf` matches
them, though the control now offers that metric.
--
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]
