sadpandajoe commented on code in PR #45555:
URL: https://github.com/apache/superset/pull/45555#discussion_r4342645009
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -544,7 +546,23 @@ export default function transformProps(
let displayName: string;
- if (groupby.length > 0) {
+ if (truncateMetric && groupby.length > 0) {
+ // Drop the metric part entirely and show only the groupby/dimension
+ // values. The label-map tuple is [metric, ...dimensions] only when the
+ // query has more than one metric; with a single metric the whole
+ // tuple is dimensions. Still apply the query-identifier suffix when
asked
+ // for, or Query A and Query B become indistinguishable whenever they
+ // share a groupby value (#38451).
+ const groupbyValues =
+ labelMapValues && ensureIsArray(metrics).length > 1
+ ? labelMapValues.slice(1)
Review Comment:
With a time comparison on, the label-map tuples no longer have the shape
this branch assumes. `renameOperator` renames the comparison series to the
offset label (or `"metric, offset"` when there are several metrics) and leaves
the original series as `[metric, ...dims]`. So with several metrics, `slice(1)`
drops the comparison window along with the metric, and the current and
historical series for the same group collapse to the same name even with query
identifiers on. With one metric and a time comparison, the original series
keeps its `[metric, dim]` tuple, so the whole tuple is joined and the name
still shows the metric, as `sum__sales, US`, despite Truncate Metric being
checked. Should the metric part be detected from the tuple itself instead of
from the metric count, so the offset label survives and the metric is dropped?
The same applies to the Query B branch.
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -1967,3 +1967,105 @@ test('converts xAxisLabelInterval string "0" to number
0', () => {
// "All" must also disable hideOverlap so ECharts never drops a label.
expect(xAxis.axisLabel.hideOverlap).toBe(false);
});
+
+describe('truncateMetric / truncateMetricB', () => {
Review Comment:
The new cases only use fixtures with one metric per query, so the `slice(1)`
branch for several metrics never runs here (or for Query B). Returning the
whole tuple, or slicing the wrong element, would still pass every assertion,
and `toContain` on names like `boy` and `girl` also holds for the untruncated
path. Could you add a two-metric case, with `[metric, dimension]` tuples for
both queries, that asserts the exact set of names with `showQueryIdentifiers`
on? A time-comparison case would cover the issue above too.
--
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]
