From 3b835e5a9f8327c75a54c1bbfae203e00376d662 Mon Sep 17 00:00:00 2001 From: Yongjie Zhao Date: Tue, 13 Sep 2022 13:45:34 +0800 Subject: [PATCH 1/3] refactor: get Axis from utils remove FF --- .../src/operators/pivotOperator.ts | 9 +++-- .../src/operators/prophetOperator.ts | 11 +++--- .../src/operators/timeComparePivotOperator.ts | 7 ++-- .../src/operators/utils/getAxis.ts | 34 +++++++++++++++++++ .../src/operators/utils/index.ts | 1 + .../src/query/normalizeTimeColumn.ts | 1 + .../src/Timeseries/transformProps.ts | 7 ++-- 7 files changed, 49 insertions(+), 21 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/getAxis.ts diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts index 564495fcbcffd..efd4f50a8bd0d 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts @@ -17,27 +17,26 @@ * under the License. */ import { - DTTM_ALIAS, ensureIsArray, getColumnLabel, getMetricLabel, PostProcessingPivot, } from '@superset-ui/core'; import { PostProcessingFactory } from './types'; +import { getAxis } from './utils'; export const pivotOperator: PostProcessingFactory = ( formData, queryObject, ) => { const metricLabels = ensureIsArray(queryObject.metrics).map(getMetricLabel); - const { x_axis: xAxis } = formData; + const xAxis = getAxis(formData); - if ((xAxis || queryObject.is_timeseries) && metricLabels.length) { - const index = [getColumnLabel(xAxis || DTTM_ALIAS)]; + if (xAxis && metricLabels.length) { return { operation: 'pivot', options: { - index, + index: [xAxis], columns: ensureIsArray(queryObject.columns).map(getColumnLabel), // Create 'dummy' mean aggregates to assign cell values in pivot table // use the 'mean' aggregates to avoid drop NaN. PR: https://github.com/apache-superset/superset-ui/pull/1231 diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/prophetOperator.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/prophetOperator.ts index ff0fa0fb6544c..71906f904942f 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/prophetOperator.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/prophetOperator.ts @@ -16,19 +16,16 @@ * specific language governing permissions and limitationsxw * under the License. */ -import { - DTTM_ALIAS, - getColumnLabel, - PostProcessingProphet, -} from '@superset-ui/core'; +import { PostProcessingProphet } from '@superset-ui/core'; import { PostProcessingFactory } from './types'; +import { getAxis } from './utils'; /* eslint-disable @typescript-eslint/no-unused-vars */ export const prophetOperator: PostProcessingFactory = ( formData, queryObject, ) => { - const index = getColumnLabel(formData.x_axis || DTTM_ALIAS); + const xAxis = getAxis(formData); if (formData.forecastEnabled) { return { operation: 'prophet', @@ -39,7 +36,7 @@ export const prophetOperator: PostProcessingFactory = ( yearly_seasonality: formData.forecastSeasonalityYearly, weekly_seasonality: formData.forecastSeasonalityWeekly, daily_seasonality: formData.forecastSeasonalityDaily, - index, + xAxis, }, }; } diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts index a4c0f5eea6c6e..024cec4894c3a 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts @@ -18,13 +18,12 @@ * under the License. */ import { - DTTM_ALIAS, ensureIsArray, getColumnLabel, NumpyFunction, PostProcessingPivot, } from '@superset-ui/core'; -import { getMetricOffsetsMap, isTimeComparison } from './utils'; +import { getMetricOffsetsMap, isTimeComparison, getAxis } from './utils'; import { PostProcessingFactory } from './types'; export const timeComparePivotOperator: PostProcessingFactory = @@ -39,12 +38,12 @@ export const timeComparePivotOperator: PostProcessingFactory { + // The formData should be "raw form_data" -- the snake_case version of formData rather than camelCase. + let col: QueryFormColumn = DTTM_ALIAS; + if (formData.x_axis && formData.granularity_sqla) { + col = formData.x_axis; + } + + return getColumnLabel(col); +}; 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 8d65ca1e590fb..809ebe06ed499 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,3 +21,4 @@ export { getMetricOffsetsMap } from './getMetricOffsetsMap'; export { isTimeComparison } from './isTimeComparison'; export { isDerivedSeries } from './isDerivedSeries'; export { TIME_COMPARISON_SEPARATOR } from './constants'; +export { getAxis } from './getAxis'; diff --git a/superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts b/superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts index 95a06fdf6c0bb..9879e9fe0a512 100644 --- a/superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts +++ b/superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts @@ -32,6 +32,7 @@ export function normalizeTimeColumn( formData: QueryFormData, queryObject: QueryObject, ): QueryObject { + // The formData should be "raw form_data" -- the snake_case version of formData rather than camelCase. if (!(isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && formData.x_axis)) { return queryObject; } 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 5a90818e9c9f9..fbe2fb1d6dc65 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -20,9 +20,7 @@ import { AnnotationLayer, CategoricalColorNamespace, - DTTM_ALIAS, GenericDataType, - getColumnLabel, getNumberFormatter, isEventAnnotationLayer, isFormulaAnnotationLayer, @@ -31,7 +29,7 @@ import { TimeseriesChartDataResponseResult, t, } from '@superset-ui/core'; -import { isDerivedSeries } from '@superset-ui/chart-controls'; +import { getAxis, isDerivedSeries } from '@superset-ui/chart-controls'; import { EChartsCoreOption, SeriesOption } from 'echarts'; import { ZRLineType } from 'echarts/types/src/util/types'; import { @@ -149,8 +147,7 @@ export default function transformProps( const colorScale = CategoricalColorNamespace.getScale(colorScheme as string); const rebasedData = rebaseForecastDatum(data, verboseMap); - const xAxisCol = - verboseMap[xAxisOrig] || getColumnLabel(xAxisOrig || DTTM_ALIAS); + const xAxisCol = verboseMap[xAxisOrig] || getAxis(chartProps.rawFormData); const isHorizontal = orientation === OrientationType.horizontal; const { totalStackedValues, thresholdValues } = extractDataTotalValues( rebasedData, From 1992a6a52279032e86f08cdcd00857a8fda8a8b8 Mon Sep 17 00:00:00 2001 From: Yongjie Zhao Date: Tue, 13 Sep 2022 16:40:45 +0800 Subject: [PATCH 2/3] fix ut --- .../src/operators/prophetOperator.ts | 4 ++-- .../src/operators/timeComparePivotOperator.ts | 4 ++-- .../src/operators/utils/getAxis.ts | 13 ++++++------ .../test/operators/pivotOperator.test.ts | 21 ++++++++++--------- .../test/operators/prophetOperator.test.ts | 1 + .../timeComparePivotOperator.test.ts | 2 +- 6 files changed, 24 insertions(+), 21 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/prophetOperator.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/prophetOperator.ts index 71906f904942f..5d8d7feea9345 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/prophetOperator.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/prophetOperator.ts @@ -26,7 +26,7 @@ export const prophetOperator: PostProcessingFactory = ( queryObject, ) => { const xAxis = getAxis(formData); - if (formData.forecastEnabled) { + if (formData.forecastEnabled && xAxis) { return { operation: 'prophet', options: { @@ -36,7 +36,7 @@ export const prophetOperator: PostProcessingFactory = ( yearly_seasonality: formData.forecastSeasonalityYearly, weekly_seasonality: formData.forecastSeasonalityWeekly, daily_seasonality: formData.forecastSeasonalityDaily, - xAxis, + index: xAxis, }, }; } diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts index 024cec4894c3a..e1310a4e3aa32 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts @@ -29,8 +29,9 @@ import { PostProcessingFactory } from './types'; export const timeComparePivotOperator: PostProcessingFactory = (formData, queryObject) => { const metricOffsetMap = getMetricOffsetsMap(formData, queryObject); + const xAxis = getAxis(formData); - if (isTimeComparison(formData, queryObject)) { + if (isTimeComparison(formData, queryObject) && xAxis) { const aggregates = Object.fromEntries( [...metricOffsetMap.values(), ...metricOffsetMap.keys()].map(metric => [ metric, @@ -38,7 +39,6 @@ export const timeComparePivotOperator: PostProcessingFactory { +export const getAxis = (formData: QueryFormData): string | undefined => { // The formData should be "raw form_data" -- the snake_case version of formData rather than camelCase. - let col: QueryFormColumn = DTTM_ALIAS; - if (formData.x_axis && formData.granularity_sqla) { - col = formData.x_axis; + if (!(formData.granularity_sqla || formData.x_axis)) { + return undefined; } - return getColumnLabel(col); + return isDefined(formData.x_axis) + ? getColumnLabel(formData.x_axis) + : DTTM_ALIAS; }; diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/operators/pivotOperator.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/operators/pivotOperator.test.ts index 41c294b405a88..c6c7f905d20ec 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/test/operators/pivotOperator.test.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/pivotOperator.test.ts @@ -56,13 +56,9 @@ const queryObject: QueryObject = { test('skip pivot', () => { expect(pivotOperator(formData, queryObject)).toEqual(undefined); - expect( - pivotOperator(formData, { ...queryObject, is_timeseries: false }), - ).toEqual(undefined); expect( pivotOperator(formData, { ...queryObject, - is_timeseries: true, metrics: [], }), ).toEqual(undefined); @@ -70,7 +66,10 @@ test('skip pivot', () => { test('pivot by __timestamp without groupby', () => { expect( - pivotOperator(formData, { ...queryObject, is_timeseries: true }), + pivotOperator( + { ...formData, granularity_sqla: 'time_column' }, + queryObject, + ), ).toEqual({ operation: 'pivot', options: { @@ -87,11 +86,13 @@ test('pivot by __timestamp without groupby', () => { test('pivot by __timestamp with groupby', () => { expect( - pivotOperator(formData, { - ...queryObject, - columns: ['foo', 'bar'], - is_timeseries: true, - }), + pivotOperator( + { ...formData, granularity_sqla: 'time_column' }, + { + ...queryObject, + columns: ['foo', 'bar'], + }, + ), ).toEqual({ operation: 'pivot', options: { diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/operators/prophetOperator.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/operators/prophetOperator.test.ts index 824efe5c15cc5..2f3c63ddf86fb 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/test/operators/prophetOperator.test.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/prophetOperator.test.ts @@ -47,6 +47,7 @@ test('should do prophetOperator with default index', () => { prophetOperator( { ...formData, + granularity_sqla: 'time_column', forecastEnabled: true, forecastPeriods: '3', forecastInterval: '5', diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/operators/timeComparePivotOperator.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/operators/timeComparePivotOperator.test.ts index 304756dee7637..bf46940d713ec 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/test/operators/timeComparePivotOperator.test.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/timeComparePivotOperator.test.ts @@ -72,10 +72,10 @@ test('should pivot on any type of timeCompare', () => { ...formData, comparison_type: cType, time_compare: ['1 year ago', '1 year later'], + granularity_sqla: 'time_column', }, { ...queryObject, - is_timeseries: true, }, ), ).toEqual({ From 6e8cc965a653116ee91d546cca6051e231bb54a9 Mon Sep 17 00:00:00 2001 From: Yongjie Zhao Date: Tue, 13 Sep 2022 16:47:10 +0800 Subject: [PATCH 3/3] type --- .../plugin-chart-echarts/src/Timeseries/transformProps.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 fbe2fb1d6dc65..5d49b657e847c 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -28,6 +28,7 @@ import { isTimeseriesAnnotationLayer, TimeseriesChartDataResponseResult, t, + DTTM_ALIAS, } from '@superset-ui/core'; import { getAxis, isDerivedSeries } from '@superset-ui/chart-controls'; import { EChartsCoreOption, SeriesOption } from 'echarts'; @@ -147,7 +148,9 @@ export default function transformProps( const colorScale = CategoricalColorNamespace.getScale(colorScheme as string); const rebasedData = rebaseForecastDatum(data, verboseMap); - const xAxisCol = verboseMap[xAxisOrig] || getAxis(chartProps.rawFormData); + // todo: if the both granularity_sqla and x_axis are `null`, should throw an error + const xAxisCol = + verboseMap[xAxisOrig] || getAxis(chartProps.rawFormData) || DTTM_ALIAS; const isHorizontal = orientation === OrientationType.horizontal; const { totalStackedValues, thresholdValues } = extractDataTotalValues( rebasedData,