From 923cab303e9824b13d0292f68a27325beda71d3f Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Fri, 12 May 2023 15:33:41 -0300 Subject: [PATCH 1/3] feat: Make offset series colors match the original series --- .../src/operators/utils/index.ts | 1 + .../src/operators/utils/isDerivedSeries.ts | 7 +-- .../src/operators/utils/timeOffset.ts | 49 ++++++++++++++++ .../src/Timeseries/transformProps.ts | 56 ++++++++++++------- .../src/Timeseries/transformers.ts | 5 +- 5 files changed, 90 insertions(+), 28 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/timeOffset.ts diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/index.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/index.ts index 67f230dd631b4..1d91a6965f52b 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/index.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/index.ts @@ -21,4 +21,5 @@ export { getMetricOffsetsMap } from './getMetricOffsetsMap'; export { isTimeComparison } from './isTimeComparison'; export { isDerivedSeries } from './isDerivedSeries'; export { extractExtraMetrics } from './extractExtraMetrics'; +export { getOriginalSeries, hasTimeOffset } from './timeOffset'; export { TIME_COMPARISON_SEPARATOR } from './constants'; diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/isDerivedSeries.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/isDerivedSeries.ts index 67815756f842b..ddb3e425df05e 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/isDerivedSeries.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/isDerivedSeries.ts @@ -23,7 +23,7 @@ import { QueryFormData, ComparisonType, } from '@superset-ui/core'; -import { isString } from 'lodash'; +import { hasTimeOffset } from './timeOffset'; export const isDerivedSeries = ( series: JsonObject, @@ -33,9 +33,6 @@ export const isDerivedSeries = ( if (comparisonType !== ComparisonType.Values) { return false; } - const timeCompare: string[] = ensureIsArray(formData?.time_compare); - return isString(series.name) - ? !!timeCompare.find(timeOffset => series.name.endsWith(timeOffset)) - : false; + return hasTimeOffset(series, timeCompare); }; diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/timeOffset.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/timeOffset.ts new file mode 100644 index 0000000000000..b11572c6dda69 --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/timeOffset.ts @@ -0,0 +1,49 @@ +/* eslint-disable camelcase */ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitationsxw + * under the License. + */ +import { JsonObject } from '@superset-ui/core'; +import { isString } from 'lodash'; + +export const hasTimeOffset = ( + series: JsonObject, + timeCompare: string[], +): boolean => + isString(series.name) + ? !!timeCompare.find( + timeOffset => + // offset is represented as , group by list + series.name.includes(`${timeOffset},`) || + // offset is represented as __ + series.name.includes(`__${timeOffset}`), + ) + : false; + +export const getOriginalSeries = ( + seriesName: string, + timeCompare: string[], +): string => { + let result = seriesName; + timeCompare.forEach(compare => { + // offset is represented as , group by list + result = result.replace(`${compare},`, ''); + // offset is represented as __ + result = result.replace(`__${compare}`, ''); + }); + return result.trim(); +}; diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index ac096a0697c1d..6235b02707f5c 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -17,10 +17,12 @@ * under the License. */ /* eslint-disable camelcase */ +import { invert } from 'lodash'; import { AnnotationLayer, AxisType, CategoricalColorNamespace, + ensureIsArray, GenericDataType, getMetricLabel, getNumberFormatter, @@ -36,6 +38,7 @@ import { } from '@superset-ui/core'; import { extractExtraMetrics, + getOriginalSeries, isDerivedSeries, } from '@superset-ui/chart-controls'; import { EChartsCoreOption, SeriesOption } from 'echarts'; @@ -227,30 +230,43 @@ export default function transformProps( contributionMode || isAreaExpand ? ',.0%' : yAxisFormat, ); + const array = ensureIsArray(chartProps.rawFormData?.time_compare); + const inverted = invert(verboseMap); + rawSeries.forEach(entry => { const lineStyle = isDerivedSeries(entry, chartProps.rawFormData) ? { type: 'dashed' as ZRLineType } : {}; - const transformedSeries = transformSeries(entry, colorScale, { - area, - filterState, - seriesContexts, - markerEnabled, - markerSize, - areaOpacity: opacity, - seriesType, - stack, - formatter, - showValue, - onlyTotal, - totalStackedValues: sortedTotalValues, - showValueIndexes, - thresholdValues, - richTooltip, - sliceId, - isHorizontal, - lineStyle, - }); + + const entryName = String(entry.name || ''); + const seriesName = inverted[entryName] || entryName; + const originalSeriesName = getOriginalSeries(seriesName, array); + + const transformedSeries = transformSeries( + entry, + originalSeriesName, + colorScale, + { + area, + filterState, + seriesContexts, + markerEnabled, + markerSize, + areaOpacity: opacity, + seriesType, + stack, + formatter, + showValue, + onlyTotal, + totalStackedValues: sortedTotalValues, + showValueIndexes, + thresholdValues, + richTooltip, + sliceId, + isHorizontal, + lineStyle, + }, + ); if (transformedSeries) { if (stack === StackControlsValue.Stream) { // bug in Echarts - `stackStrategy: 'all'` doesn't work with nulls, so we cast them to 0 diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts index 953037c3d5dbd..90b61847ec019 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -51,7 +51,6 @@ import { MarkArea2DDataItemOption, } from 'echarts/types/src/component/marker/MarkAreaModel'; import { MarkLine1DDataItemOption } from 'echarts/types/src/component/marker/MarkLineModel'; - import { extractForecastSeriesContext } from '../utils/forecast'; import { EchartsTimeseriesSeriesType, @@ -143,6 +142,7 @@ export const getBaselineSeriesForStream = ( export function transformSeries( series: SeriesOption, + originalSeriesName: string, colorScale: CategoricalColorScale, opts: { area?: boolean; @@ -186,7 +186,6 @@ export function transformSeries( showValueIndexes = [], thresholdValues = [], richTooltip, - seriesKey, sliceId, isHorizontal = false, queryIndex = 0, @@ -236,7 +235,7 @@ export function transformSeries( } // forcing the colorScale to return a different color for same metrics across different queries const itemStyle = { - color: colorScale(seriesKey || forecastSeries.name, sliceId), + color: colorScale(originalSeriesName, sliceId), opacity, }; let emphasis = {}; From e0ee20f27eb04d64841e57707c60c5afda5504a4 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Fri, 12 May 2023 16:13:47 -0300 Subject: [PATCH 2/3] Adds logic to MixedTimeseries as well --- .../src/MixedTimeseries/transformProps.ts | 112 +++++++++++------- .../src/Timeseries/transformProps.ts | 4 +- .../src/Timeseries/transformers.ts | 4 +- 3 files changed, 72 insertions(+), 48 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts index d47b0047c94db..a14904b05c7b1 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -17,6 +17,7 @@ * under the License. */ /* eslint-disable camelcase */ +import { invert } from 'lodash'; import { AnnotationLayer, CategoricalColorNamespace, @@ -32,7 +33,9 @@ import { getXAxisLabel, isPhysicalColumn, isDefined, + ensureIsArray, } from '@superset-ui/core'; +import { getOriginalSeries } from '@superset-ui/chart-controls'; import { EChartsCoreOption, SeriesOption } from 'echarts'; import { DEFAULT_FORM_DATA, @@ -296,55 +299,76 @@ export default function transformProps( formatter: formatterSecondary, }); + const array = ensureIsArray(chartProps.rawFormData?.time_compare); + const inverted = invert(verboseMap); + rawSeriesA.forEach(entry => { - const transformedSeries = transformSeries(entry, colorScale, { - area, - markerEnabled, - markerSize, - areaOpacity: opacity, - seriesType, - showValue, - stack: Boolean(stack), - yAxisIndex, - filterState, - seriesKey: entry.name, - sliceId, - queryIndex: 0, - formatter: - seriesType === EchartsTimeseriesSeriesType.Bar - ? maxLabelFormatter - : formatter, - showValueIndexes: showValueIndexesA, - totalStackedValues, - thresholdValues, - }); + const entryName = String(entry.name || ''); + const seriesName = inverted[entryName] || entryName; + const colorScaleKey = getOriginalSeries(seriesName, array); + + const transformedSeries = transformSeries( + entry, + colorScale, + colorScaleKey, + { + area, + markerEnabled, + markerSize, + areaOpacity: opacity, + seriesType, + showValue, + stack: Boolean(stack), + yAxisIndex, + filterState, + seriesKey: entry.name, + sliceId, + queryIndex: 0, + formatter: + seriesType === EchartsTimeseriesSeriesType.Bar + ? maxLabelFormatter + : formatter, + showValueIndexes: showValueIndexesA, + totalStackedValues, + thresholdValues, + }, + ); if (transformedSeries) series.push(transformedSeries); }); rawSeriesB.forEach(entry => { - const transformedSeries = transformSeries(entry, colorScale, { - area: areaB, - markerEnabled: markerEnabledB, - markerSize: markerSizeB, - areaOpacity: opacityB, - seriesType: seriesTypeB, - showValue: showValueB, - stack: Boolean(stackB), - yAxisIndex: yAxisIndexB, - filterState, - seriesKey: primarySeries.has(entry.name as string) - ? `${entry.name} (1)` - : entry.name, - sliceId, - queryIndex: 1, - formatter: - seriesTypeB === EchartsTimeseriesSeriesType.Bar - ? maxLabelFormatterSecondary - : formatterSecondary, - showValueIndexes: showValueIndexesB, - totalStackedValues: totalStackedValuesB, - thresholdValues: thresholdValuesB, - }); + const entryName = String(entry.name || ''); + const seriesName = `${inverted[entryName] || entryName} (1)`; + const colorScaleKey = getOriginalSeries(seriesName, array); + + const transformedSeries = transformSeries( + entry, + colorScale, + colorScaleKey, + { + area: areaB, + markerEnabled: markerEnabledB, + markerSize: markerSizeB, + areaOpacity: opacityB, + seriesType: seriesTypeB, + showValue: showValueB, + stack: Boolean(stackB), + yAxisIndex: yAxisIndexB, + filterState, + seriesKey: primarySeries.has(entry.name as string) + ? `${entry.name} (1)` + : entry.name, + sliceId, + queryIndex: 1, + formatter: + seriesTypeB === EchartsTimeseriesSeriesType.Bar + ? maxLabelFormatterSecondary + : formatterSecondary, + showValueIndexes: showValueIndexesB, + totalStackedValues: totalStackedValuesB, + thresholdValues: thresholdValuesB, + }, + ); if (transformedSeries) series.push(transformedSeries); }); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index 6235b02707f5c..40ce5323e702e 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -240,12 +240,12 @@ export default function transformProps( const entryName = String(entry.name || ''); const seriesName = inverted[entryName] || entryName; - const originalSeriesName = getOriginalSeries(seriesName, array); + const colorScaleKey = getOriginalSeries(seriesName, array); const transformedSeries = transformSeries( entry, - originalSeriesName, colorScale, + colorScaleKey, { area, filterState, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts index 90b61847ec019..37e3eb9fcea04 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -142,8 +142,8 @@ export const getBaselineSeriesForStream = ( export function transformSeries( series: SeriesOption, - originalSeriesName: string, colorScale: CategoricalColorScale, + colorScaleKey: string, opts: { area?: boolean; filterState?: FilterState; @@ -235,7 +235,7 @@ export function transformSeries( } // forcing the colorScale to return a different color for same metrics across different queries const itemStyle = { - color: colorScale(originalSeriesName, sliceId), + color: colorScale(colorScaleKey, sliceId), opacity, }; let emphasis = {}; From eaa3ae7cda9c201cdd4b7424f848180b2f1e51fb Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Fri, 12 May 2023 16:51:33 -0300 Subject: [PATCH 3/3] Adds a test --- .../test/operators/utils/timeOffset.test.ts | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/timeOffset.test.ts diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/timeOffset.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/timeOffset.test.ts new file mode 100644 index 0000000000000..bee8c5bbbd269 --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/timeOffset.test.ts @@ -0,0 +1,30 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { getOriginalSeries } from '@superset-ui/chart-controls'; + +test('returns the series name when time compare is empty', () => { + const seriesName = 'sum'; + expect(getOriginalSeries(seriesName, [])).toEqual(seriesName); +}); + +test('returns the original series name', () => { + const seriesName = 'sum__1_month_ago'; + const timeCompare = ['1_month_ago']; + expect(getOriginalSeries(seriesName, timeCompare)).toEqual('sum'); +});