sadpandajoe commented on code in PR #45555:
URL: https://github.com/apache/superset/pull/45555#discussion_r4344050423
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -544,7 +580,14 @@ export default function transformProps(
let displayName: string;
- if (groupby.length > 0) {
+ if (!hasTimeComparison && truncateMetric && groupby.length > 0) {
+ displayName = getTruncatedSeriesName({
+ labelMapValues: labelMapValues,
Review Comment:
The repo's `object-shorthand` lint rule rewrites this, and the pre-commit
job failed because it modified this file on CI.
```suggestion
labelMapValues,
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -1967,3 +1967,145 @@ 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', () => {
+ const getSeriesNames = (
+ overrides: Partial<EchartsMixedTimeseriesFormData>,
+ ) => {
+ const chartProps = createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: queriesData,
+ formData: { ...formData, ...overrides },
+ queriesData,
+ });
+ const transformed = transformProps(chartProps);
+ return (transformed.echartOptions.series as SeriesOption[]).map(
+ s => s.name,
+ );
+ };
+
+ test('drops the metric prefix for both queries when both flags are on', ()
=> {
+ const seriesNames = getSeriesNames({
+ truncateMetric: true,
+ truncateMetricB: true,
+ });
+
+ expect(seriesNames).toContain('boy');
+ expect(seriesNames).toContain('girl');
+ expect(seriesNames).not.toContain('sum__num, boy');
+ expect(seriesNames).not.toContain('sum__num, girl');
+ });
+
+ test('keeps the "metric, dimension" name when both flags are off', () => {
+ const seriesNames = getSeriesNames({});
+
+ expect(seriesNames).toContain('sum__num, boy');
+ expect(seriesNames).toContain('sum__num, girl');
+ });
+
+ test('truncateMetricB truncates query B independently of query A', () => {
+ const seriesNames = getSeriesNames({ truncateMetricB: true });
Review Comment:
Swapping `truncateMetric` and `truncateMetricB` at the two call sites in
`transformProps.ts` would still pass this test: both queries use identical data
and the assertions look at the combined name list without identifiers, so `boy`
is satisfied by either query. Could this case turn on `showQueryIdentifiers`
and assert `sum__num (Query A), boy` is present while `boy (Query B)` is, so
A/B attribution is actually pinned?
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -651,7 +694,14 @@ export default function transformProps(
let displayName: string;
- if (groupbyB.length > 0) {
+ if (!hasTimeComparison && truncateMetricB && groupbyB.length > 0) {
Review Comment:
`hasTimeComparison` is derived only from `time_compare`, but Query B has its
own `time_compare_b` (the B query is built from it). With `time_compare` empty
and `time_compare_b` set, Query B's offset series are still truncated here, and
with `time_compare` set on A, B's truncation is switched off even though B has
no comparison. So with the same inputs, B's comparison series can have the
wrong name shape, or the Truncate Metric B setting does nothing. Should the
bypass for Query B key off `time_compare_b` instead (and A off `time_compare`)?
--
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]
