From 79435968fd17a98925c2e3d6c32e4438da35d473 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Mon, 8 Apr 2024 20:25:21 +0200 Subject: [PATCH 01/27] BigNumber with Time Comparison: - Use existig time_offset APIs instead of the instant comparison ones - Create new shared section to include the controls for time comparison, some taken from Advanced Analytics - Create a new control label to display the comparison ranges using existing time_range API and not a new one Notes: - Tooltip is no longer displaying comparison ranges in chart - Time shift doesnt include Custom nor Inherit options - Time grain is part of the controls but it might be removed --- .../src/sections/index.ts | 1 + .../src/sections/timeComparison.tsx | 85 ++++++++++++++++ .../src/time-comparison/fetchTimeRange.ts | 57 +++++++++-- .../BigNumberPeriodOverPeriod/PopKPI.tsx | 2 +- .../BigNumberPeriodOverPeriod/buildQuery.ts | 58 +++++------ .../BigNumberPeriodOverPeriod/controlPanel.ts | 94 ++++-------------- .../transformProps.ts | 31 +++--- .../controls/ComparisonRangeLabel.tsx | 96 +++++++++++++++++++ .../src/explore/components/controls/index.js | 2 + superset/views/api.py | 7 +- tests/integration_tests/charts/api_tests.py | 14 +++ 11 files changed, 316 insertions(+), 131 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx create mode 100644 superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/index.ts b/superset-frontend/packages/superset-ui-chart-controls/src/sections/index.ts index c0113b189fd8e..caa07faa9c485 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/index.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/index.ts @@ -23,3 +23,4 @@ export * from './annotationsAndLayers'; export * from './forecastInterval'; export * from './chartTitle'; export * from './echartsTimeSeriesQuery'; +export * from './timeComparison'; diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx new file mode 100644 index 0000000000000..002862ff862c6 --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx @@ -0,0 +1,85 @@ +/** + * 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 { t, ComparisonType } from '@superset-ui/core'; + +import { ControlPanelSectionConfig } from '../types'; + +export const timeComparisonControls: ControlPanelSectionConfig = { + label: t('Time Comparison'), + tabOverride: 'data', + description: t( + 'This section contains options ' + + 'that allow for time comparison ' + + 'of query results using some portions of the ' + + 'existing advanced analytics section', + ), + controlSetRows: [ + [ + { + name: 'time_compare', + config: { + type: 'SelectControl', + multi: true, + freeForm: true, + label: t('Time shift'), + choices: [ + ['1 day ago', t('1 day ago')], + ['1 week ago', t('1 week ago')], + ['28 days ago', t('28 days ago')], + ['30 days ago', t('30 days ago')], + ['52 weeks ago', t('52 weeks ago')], + ['1 year ago', t('1 year ago')], + ['104 weeks ago', t('104 weeks ago')], + ['2 years ago', t('2 years ago')], + ['156 weeks ago', t('156 weeks ago')], + ['3 years ago', t('3 years ago')], + ], + description: t( + 'Overlay one or more timeseries from a ' + + 'relative time period. Expects relative time deltas ' + + 'in natural language (example: 24 hours, 7 days, ' + + '52 weeks, 365 days). Free text is supported.', + ), + }, + }, + ], + ['time_grain_sqla'], + [ + { + name: 'comparison_type', + config: { + type: 'SelectControl', + label: t('Calculation type'), + default: 'values', + choices: [ + [ComparisonType.Values, t('Actual values')], + [ComparisonType.Difference, t('Difference')], + [ComparisonType.Percentage, t('Percentage change')], + [ComparisonType.Ratio, t('Ratio')], + ], + description: t( + 'How to display time shifts: as individual lines; as the ' + + 'difference between the main time series and each time shift; ' + + 'as the percentage change; or as the ratio between series and time shifts.', + ), + }, + }, + ], + ], +}; diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts index 50509af52b96f..8de50d501bb30 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts @@ -18,6 +18,7 @@ */ import rison from 'rison'; import { SupersetClient, getClientErrorObject } from '@superset-ui/core'; +import { isEmpty } from 'lodash'; export const SEPARATOR = ' : '; @@ -39,20 +40,64 @@ export const formatTimeRange = ( )} ≤ ${columnPlaceholder} < ${formatDateEndpoint(splitDateRange[1])}`; }; +export const formatTimeRangeComparison = ( + initialTimeRange: string, + shiftedTimeRange: string, + columnPlaceholder = 'col', +) => { + const splitInitialDateRange = initialTimeRange.split(SEPARATOR); + const splitShiftedDateRange = shiftedTimeRange.split(SEPARATOR); + return `${columnPlaceholder}: ${formatDateEndpoint( + splitInitialDateRange[0], + true, + )} to ${formatDateEndpoint(splitInitialDateRange[1])} vs + ${formatDateEndpoint(splitShiftedDateRange[0], true)} to ${formatDateEndpoint( + splitShiftedDateRange[1], + )}`; +}; + export const fetchTimeRange = async ( timeRange: string, columnPlaceholder = 'col', + shifts?: string[], ) => { - const query = rison.encode_uri(timeRange); - const endpoint = `/api/v1/time_range/?q=${query}`; + let query; + let endpoint; + if (!isEmpty(shifts)) { + const timeRanges = shifts?.map(shift => ({ + timeRange, + shift, + })); + query = rison.encode_uri([{ timeRange }, ...(timeRanges || [])]); + endpoint = `/api/v1/time_range/?q=${query}`; + } else { + query = rison.encode_uri(timeRange); + endpoint = `/api/v1/time_range/?q=${query}`; + } try { const response = await SupersetClient.get({ endpoint }); - const timeRangeString = buildTimeRangeString( - response?.json?.result[0]?.since || '', - response?.json?.result[0]?.until || '', + if (isEmpty(shifts)) { + const timeRangeString = buildTimeRangeString( + response?.json?.result[0]?.since || '', + response?.json?.result[0]?.until || '', + ); + return { + value: formatTimeRange(timeRangeString, columnPlaceholder), + }; + } + const timeRanges = response?.json?.result.map((result: any) => + buildTimeRangeString(result.since, result.until), ); return { - value: formatTimeRange(timeRangeString, columnPlaceholder), + value: timeRanges + .slice(1) + .map((timeRange: string) => + formatTimeRangeComparison( + timeRanges[0], + timeRange, + columnPlaceholder, + ), + ), }; } catch (response) { const clientError = await getClientErrorObject(response); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx index b0d9d912d79bf..20339cd3ce77c 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx @@ -150,7 +150,7 @@ export default function PopKPI(props: PopKPIProps) { { symbol: '#', value: prevNumber, - tooltipText: t('Data for %s', comparatorText), + tooltipText: t('Data for comparison range', comparatorText), }, { symbol: '△', diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts index abf91104034b6..167ff6a24d8e3 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts @@ -18,50 +18,42 @@ */ import { buildQueryContext, - getComparisonInfo, - ComparisonTimeRangeType, QueryFormData, + PostProcessingRule, } from '@superset-ui/core'; +import { + isTimeComparison, + timeCompareOperator, +} from '@superset-ui/chart-controls'; export default function buildQuery(formData: QueryFormData) { - const { - cols: groupby, - time_comparison: timeComparison, - extra_form_data: extraFormData, - } = formData; - - const queryContextA = buildQueryContext(formData, baseQueryObject => [ - { - ...baseQueryObject, - groupby, - }, - ]); - - const comparisonFormData = getComparisonInfo( - formData, - timeComparison, - extraFormData, - ); + const { cols: groupby, time_grain_sqla } = formData; - const queryContextB = buildQueryContext( - comparisonFormData, - baseQueryObject => [ + const queryContextA = buildQueryContext(formData, baseQueryObject => { + const postProcessing: PostProcessingRule[] = []; + postProcessing.push(timeCompareOperator(formData, baseQueryObject)); + return [ { ...baseQueryObject, + columns: [ + { + timeGrain: time_grain_sqla || 'P1Y', // Group by year by default + columnType: 'BASE_AXIS', + sqlExpression: baseQueryObject.filters?.[0]?.col.toString() || '', + label: baseQueryObject.filters?.[0]?.col.toString() || '', + expressionType: 'SQL', + }, + ], groupby, - extras: { - ...baseQueryObject.extras, - instant_time_comparison_range: - timeComparison !== ComparisonTimeRangeType.Custom - ? timeComparison - : undefined, - }, + post_processing: postProcessing, + time_offsets: isTimeComparison(formData, baseQueryObject) + ? formData.time_compare + : [], }, - ], - ); + ]; + }); return { ...queryContextA, - queries: [...queryContextA.queries, ...queryContextB.queries], }; } diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts index f6f81d98d75f3..cb4a6b842b10e 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts @@ -16,20 +16,12 @@ * specific language governing permissions and limitations * under the License. */ +import { t } from '@superset-ui/core'; import { - AdhocFilter, - ComparisonTimeRangeType, - SimpleAdhocFilter, - t, - validateTimeComparisonRangeValues, -} from '@superset-ui/core'; -import { - ColumnMeta, ControlPanelConfig, - ControlPanelState, - ControlState, getStandardizedControls, sharedControls, + sections, } from '@superset-ui/chart-controls'; import { headerFontSize, subheaderFontSize } from '../sharedControls'; import { ColorSchemeEnum } from './types'; @@ -42,70 +34,6 @@ const config: ControlPanelConfig = { controlSetRows: [ ['metric'], ['adhoc_filters'], - [ - { - name: 'time_comparison', - config: { - type: 'SelectControl', - label: t('Range for Comparison'), - default: 'r', - choices: [ - ['r', 'Inherit range from time filters'], - ['y', 'Year'], - ['m', 'Month'], - ['w', 'Week'], - ['c', 'Custom'], - ], - rerender: ['adhoc_custom'], - description: t( - 'Set the time range that will be used for the comparison metrics. ' + - 'For example, "Year" will compare to the same dates one year earlier. ' + - 'Use "Inherit range from time filters" to shift the comparison time range' + - 'by the same length as your time range and use "Custom" to set a custom comparison range.', - ), - }, - }, - ], - [ - { - name: `adhoc_custom`, - config: { - ...sharedControls.adhoc_filters, - label: t('Filters for Comparison'), - description: - 'This only applies when selecting the Range for Comparison Type: Custom', - visibility: ({ controls }) => - controls?.time_comparison?.value === - ComparisonTimeRangeType.Custom, - mapStateToProps: ( - state: ControlPanelState, - controlState: ControlState, - ) => { - const originalMapStateToPropsRes = - sharedControls.adhoc_filters.mapStateToProps?.( - state, - controlState, - ) || {}; - const columns = originalMapStateToPropsRes.columns.filter( - (col: ColumnMeta) => - col.is_dttm && - (state.controls.adhoc_filters.value as AdhocFilter[]).some( - (val: SimpleAdhocFilter) => - val.subject === col.column_name, - ), - ); - return { - ...originalMapStateToPropsRes, - columns, - externalValidationErrors: validateTimeComparisonRangeValues( - state.controls?.time_comparison?.value, - controlState.value, - ), - }; - }, - }, - }, - ], [ { name: 'row_limit', @@ -180,14 +108,26 @@ const config: ControlPanelConfig = { ], ], }, + { + ...sections.timeComparisonControls, + controlSetRows: [ + ...sections.timeComparisonControls.controlSetRows, + [ + { + name: 'comparison_range_label', + config: { + type: 'ComparisonRangeLabel', + multi: false, + }, + }, + ], + ], + }, ], controlOverrides: { y_axis_format: { label: t('Number format'), }, - adhoc_filters: { - rerender: ['adhoc_custom'], - }, }, formDataOverrides: formData => ({ ...formData, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts index a17fb8edd08fd..0e15a753fcea8 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts @@ -22,7 +22,6 @@ import { getMetricLabel, getValueFormatter, getNumberFormatter, - formatTimeRange, } from '@superset-ui/core'; import { getComparisonFontSize, getHeaderFontSize } from './utils'; @@ -87,17 +86,28 @@ export default function transformProps(chartProps: ChartProps) { percentDifferenceFormat, } = formData; const { data: dataA = [] } = queriesData[0]; - const { - data: dataB = [], - from_dttm: comparisonFromDatetime, - to_dttm: comparisonToDatetime, - } = queriesData[1]; const data = dataA; const metricName = getMetricLabel(metric); + const timeComparison = chartProps.rawFormData?.time_compare?.[0]; + + const { value1, value2 } = data.reduce( + (acc: { value1: number; value2: number }, curr: { [x: string]: any }) => { + Object.keys(curr).forEach(key => { + if (key.includes(`${metricName}__${timeComparison}`)) { + acc.value2 += curr[key]; + } else if (key.includes(metricName)) { + acc.value1 += curr[key]; + } + }); + return acc; + }, + { value1: 0, value2: 0 }, + ); + let bigNumber: number | string = - data.length === 0 ? 0 : parseMetricValue(data[0][metricName]); + data.length === 0 ? 0 : parseMetricValue(value1); let prevNumber: number | string = - data.length === 0 ? 0 : parseMetricValue(dataB[0][metricName]); + data.length === 0 ? 0 : parseMetricValue(value2); const numberFormatter = getValueFormatter( metric, @@ -133,10 +143,6 @@ export default function transformProps(chartProps: ChartProps) { prevNumber = numberFormatter(prevNumber); valueDifference = numberFormatter(valueDifference); const percentDifference: string = formatPercentChange(percentDifferenceNum); - const comparatorText = formatTimeRange('%Y-%m-%d', [ - comparisonFromDatetime, - comparisonToDatetime, - ]); return { width, @@ -155,6 +161,5 @@ export default function transformProps(chartProps: ChartProps) { comparisonColorEnabled, comparisonColorScheme, percentDifferenceNumber: percentDifferenceNum, - comparatorText, }; } diff --git a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx new file mode 100644 index 0000000000000..b05638810731b --- /dev/null +++ b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx @@ -0,0 +1,96 @@ +/** + * 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 React, { useEffect, useState } from 'react'; +import { useSelector } from 'react-redux'; +import { isEmpty, isEqual } from 'lodash'; +import { + BinaryAdhocFilter, + css, + fetchTimeRange, + SimpleAdhocFilter, + t, +} from '@superset-ui/core'; +import ControlHeader, { + ControlHeaderProps, +} from 'src/explore/components/ControlHeader'; +import { RootState } from 'src/views/store'; + +const isTimeRangeEqual = ( + left: BinaryAdhocFilter[], + right: BinaryAdhocFilter[], +) => isEqual(left, right); + +type ComparisonRangeLabelProps = ControlHeaderProps & { + multi?: boolean; +}; + +export const ComparisonRangeLabel = ({ + multi = true, +}: ComparisonRangeLabelProps) => { + const [labels, setLabels] = useState([]); + const currentTimeRangeFilters = useSelector( + state => + state.explore.form_data.adhoc_filters.filter( + (adhoc_filter: SimpleAdhocFilter) => + adhoc_filter.operator === 'TEMPORAL_RANGE', + ), + isTimeRangeEqual, + ); + const shifts = useSelector( + state => state.explore.form_data.time_compare, + ); + + useEffect(() => { + if (isEmpty(currentTimeRangeFilters) || isEmpty(shifts)) { + setLabels([]); + } else if (!isEmpty(shifts)) { + const promises = currentTimeRangeFilters.map(filter => + fetchTimeRange( + filter.comparator, + filter.subject, + multi ? shifts : shifts.slice(0, 1), + ), + ); + Promise.all(promises).then(res => { + // access the value property inside the res and set the labels with it in the state + setLabels(res.map(r => r.value ?? '')); + }); + } + }, [currentTimeRangeFilters, shifts]); + + return labels.length ? ( + <> + + {labels.flat().map(label => ( + <> +
css` + font-size: ${theme.typography.sizes.m}px; + color: ${theme.colors.grayscale.dark1}; + `} + key={label} + > + {label} +
+ + ))} + + ) : null; +}; diff --git a/superset-frontend/src/explore/components/controls/index.js b/superset-frontend/src/explore/components/controls/index.js index a5d65f7768375..2bf2662d0c087 100644 --- a/superset-frontend/src/explore/components/controls/index.js +++ b/superset-frontend/src/explore/components/controls/index.js @@ -48,6 +48,7 @@ import DndColumnSelectControl, { import XAxisSortControl from './XAxisSortControl'; import CurrencyControl from './CurrencyControl'; import ColumnConfigControl from './ColumnConfigControl'; +import { ComparisonRangeLabel } from './ComparisonRangeLabel'; const controlMap = { AnnotationLayerControl, @@ -80,6 +81,7 @@ const controlMap = { ConditionalFormattingControl, XAxisSortControl, ContourControl, + ComparisonRangeLabel, ...sharedControlComponents, }; export default controlMap; diff --git a/superset/views/api.py b/superset/views/api.py index 2e3c3b9bdd9a3..d5dd0eca4c5ec 100644 --- a/superset/views/api.py +++ b/superset/views/api.py @@ -46,6 +46,7 @@ "type": "object", "properties": { "timeRange": {"type": "string"}, + "shift": {"type": "string"}, }, }, } @@ -110,12 +111,16 @@ def time_range(self, **kwargs: Any) -> FlaskResponse: rv = [] for time_range in time_ranges: - since, until = get_since_until(time_range["timeRange"]) + since, until = get_since_until( + time_range=time_range["timeRange"], + time_shift=time_range.get("shift"), + ) rv.append( { "since": since.isoformat() if since else "", "until": until.isoformat() if until else "", "timeRange": time_range["timeRange"], + "shift": time_range.get("shift"), } ) return self.json_response({"result": rv}) diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py index e6c68147c213e..65ede9221cff1 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -1520,6 +1520,20 @@ def test_get_time_range(self): assert "until" in data["result"][0] assert "timeRange" in data["result"][0] + humanize_time_range = [ + {"timeRange": "2021-01-01 : 2022-02-01", "shift": "1 year ago"}, + {"timeRange": "2022-01-01 : 2023-02-01", "shift": "2 year ago"}, + ] + uri = f"api/v1/time_range/?q={prison.dumps(humanize_time_range)}" + rv = self.client.get(uri) + data = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 200 + assert len(data["result"]) == 2 + assert "since" in data["result"][0] + assert "until" in data["result"][0] + assert "timeRange" in data["result"][0] + assert "shift" in data["result"][0] + def test_query_form_data(self): """ Chart API: Test query form data From eaef79da10fa9ce41dee28e094075c99a24ad6c0 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Mon, 8 Apr 2024 20:31:37 +0200 Subject: [PATCH 02/27] BigNumber with Time Comparison: - Use core method to render comparison date in tooltip --- .../BigNumberPeriodOverPeriod/PopKPI.tsx | 31 ++++++++++++++++--- .../transformProps.ts | 7 +++++ .../BigNumberPeriodOverPeriod/types.ts | 4 ++- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx index 20339cd3ce77c..83c51b726d938 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx @@ -16,9 +16,10 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useMemo } from 'react'; -import { css, styled, t, useTheme } from '@superset-ui/core'; +import React, { useEffect, useMemo, useState } from 'react'; +import { css, fetchTimeRange, styled, t, useTheme } from '@superset-ui/core'; import { Tooltip } from '@superset-ui/chart-controls'; +import { isEmpty } from 'lodash'; import { ColorSchemeEnum, PopKPIComparisonSymbolStyleProps, @@ -69,9 +70,29 @@ export default function PopKPI(props: PopKPIProps) { comparisonColorEnabled, comparisonColorScheme, percentDifferenceNumber, - comparatorText, + currentTimeRangeFilter, + shift, } = props; + const [comparisonRange, setComparisonRange] = useState(''); + + useEffect(() => { + if (!currentTimeRangeFilter || !shift) { + setComparisonRange(''); + } else if (!isEmpty(shift)) { + const promise: any = fetchTimeRange( + (currentTimeRangeFilter as any).comparator, + currentTimeRangeFilter.subject, + shift ? [shift] : [], + ); + Promise.resolve(promise).then((res: any) => { + const response: string[] = res.value; + const firstRange: string = response.flat()[0]; + setComparisonRange(firstRange.split('vs\n')[1].trim()); + }); + } + }, [currentTimeRangeFilter, shift]); + const theme = useTheme(); const flexGap = theme.gridUnit * 5; const wrapperDivStyles = css` @@ -150,7 +171,7 @@ export default function PopKPI(props: PopKPIProps) { { symbol: '#', value: prevNumber, - tooltipText: t('Data for comparison range', comparatorText), + tooltipText: t('Data for %s', comparisonRange || 'previous range'), }, { symbol: '△', @@ -164,7 +185,7 @@ export default function PopKPI(props: PopKPIProps) { }, ], [ - comparatorText, + comparisonRange, prevNumber, valueDifference, percentDifferenceFormattedString, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts index 0e15a753fcea8..b6b74e564882b 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts @@ -22,6 +22,7 @@ import { getMetricLabel, getValueFormatter, getNumberFormatter, + SimpleAdhocFilter, } from '@superset-ui/core'; import { getComparisonFontSize, getHeaderFontSize } from './utils'; @@ -89,6 +90,10 @@ export default function transformProps(chartProps: ChartProps) { const data = dataA; const metricName = getMetricLabel(metric); const timeComparison = chartProps.rawFormData?.time_compare?.[0]; + const currentTimeRangeFilter = chartProps.rawFormData?.adhoc_filters?.filter( + (adhoc_filter: SimpleAdhocFilter) => + adhoc_filter.operator === 'TEMPORAL_RANGE', + )?.[0]; const { value1, value2 } = data.reduce( (acc: { value1: number; value2: number }, curr: { [x: string]: any }) => { @@ -161,5 +166,7 @@ export default function transformProps(chartProps: ChartProps) { comparisonColorEnabled, comparisonColorScheme, percentDifferenceNumber: percentDifferenceNum, + currentTimeRangeFilter, + shift: timeComparison, }; } diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts index a2282febbb1c5..ba4eb6bf063fc 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts @@ -21,6 +21,7 @@ import { supersetTheme, TimeseriesDataRecord, Metric, + SimpleAdhocFilter, } from '@superset-ui/core'; export interface PopKPIStylesProps { @@ -60,8 +61,9 @@ export type PopKPIProps = PopKPIStylesProps & percentDifferenceFormattedString: string; compType: string; percentDifferenceNumber: number; - comparatorText: string; comparisonColorScheme?: string; + currentTimeRangeFilter?: SimpleAdhocFilter; + shift: string; }; export enum ColorSchemeEnum { From 090ba4ec82241e98c610fb0620f1640b0e971d6d Mon Sep 17 00:00:00 2001 From: lilykuang Date: Wed, 10 Apr 2024 19:26:45 -0700 Subject: [PATCH 03/27] implement start date offset controls --- .../src/sections/timeComparison.tsx | 9 ++ .../src/shared-controls/components/index.tsx | 2 + .../BigNumberPeriodOverPeriod/PopKPI.tsx | 14 ++- .../transformProps.ts | 2 + .../BigNumberPeriodOverPeriod/types.ts | 1 + .../controls/ComparisonRangeLabel.tsx | 29 +++-- .../components/controls/TimeOffsetControl.tsx | 112 ++++++++++++++++++ .../src/explore/components/controls/index.js | 2 + 8 files changed, 159 insertions(+), 12 deletions(-) create mode 100644 superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx index 002862ff862c6..eb097340ed58f 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx @@ -59,6 +59,15 @@ export const timeComparisonControls: ControlPanelSectionConfig = { }, }, ], + [ + { + name: 'start_date_offset', + config: { + type: 'TimeOffsetControl', + label: t('shift start date'), + }, + }, + ], ['time_grain_sqla'], [ { diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/index.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/index.tsx index 93bfbcbe6871e..a138195618c1a 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/index.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/index.tsx @@ -27,3 +27,5 @@ export * from './RadioButtonControl'; export default { RadioButtonControl, }; + +export { RadioButtonControl }; diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx index 83c51b726d938..0ca7625f4a95a 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx @@ -20,6 +20,7 @@ import React, { useEffect, useMemo, useState } from 'react'; import { css, fetchTimeRange, styled, t, useTheme } from '@superset-ui/core'; import { Tooltip } from '@superset-ui/chart-controls'; import { isEmpty } from 'lodash'; +import moment from 'moment'; import { ColorSchemeEnum, PopKPIComparisonSymbolStyleProps, @@ -71,19 +72,24 @@ export default function PopKPI(props: PopKPIProps) { comparisonColorScheme, percentDifferenceNumber, currentTimeRangeFilter, + startDateOffset, shift, } = props; const [comparisonRange, setComparisonRange] = useState(''); useEffect(() => { - if (!currentTimeRangeFilter || !shift) { + if (!currentTimeRangeFilter || (!shift && !startDateOffset)) { setComparisonRange(''); - } else if (!isEmpty(shift)) { + } else if (!isEmpty(shift) || startDateOffset) { + const startDateShift = moment( + (currentTimeRangeFilter as any).comparator.split(' : ')[0], + ).diff(moment(startDateOffset), 'days'); + const newshift = startDateShift ? `${startDateShift} days ago` : shift; const promise: any = fetchTimeRange( (currentTimeRangeFilter as any).comparator, currentTimeRangeFilter.subject, - shift ? [shift] : [], + newshift ? [newshift] : [], ); Promise.resolve(promise).then((res: any) => { const response: string[] = res.value; @@ -91,7 +97,7 @@ export default function PopKPI(props: PopKPIProps) { setComparisonRange(firstRange.split('vs\n')[1].trim()); }); } - }, [currentTimeRangeFilter, shift]); + }, [currentTimeRangeFilter, shift, startDateOffset]); const theme = useTheme(); const flexGap = theme.gridUnit * 5; diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts index b6b74e564882b..3fcc4704ca422 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts @@ -90,6 +90,7 @@ export default function transformProps(chartProps: ChartProps) { const data = dataA; const metricName = getMetricLabel(metric); const timeComparison = chartProps.rawFormData?.time_compare?.[0]; + const startDateOffset = chartProps.rawFormData?.start_date_offset; const currentTimeRangeFilter = chartProps.rawFormData?.adhoc_filters?.filter( (adhoc_filter: SimpleAdhocFilter) => adhoc_filter.operator === 'TEMPORAL_RANGE', @@ -167,6 +168,7 @@ export default function transformProps(chartProps: ChartProps) { comparisonColorScheme, percentDifferenceNumber: percentDifferenceNum, currentTimeRangeFilter, + startDateOffset, shift: timeComparison, }; } diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts index ba4eb6bf063fc..e18e04261f58d 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts @@ -63,6 +63,7 @@ export type PopKPIProps = PopKPIStylesProps & percentDifferenceNumber: number; comparisonColorScheme?: string; currentTimeRangeFilter?: SimpleAdhocFilter; + startDateOffset?: string; shift: string; }; diff --git a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx index b05638810731b..ff5008f2d2c92 100644 --- a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx +++ b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx @@ -20,6 +20,7 @@ import React, { useEffect, useState } from 'react'; import { useSelector } from 'react-redux'; import { isEmpty, isEqual } from 'lodash'; +import moment from 'moment'; import { BinaryAdhocFilter, css, @@ -56,24 +57,36 @@ export const ComparisonRangeLabel = ({ const shifts = useSelector( state => state.explore.form_data.time_compare, ); + const startDate = useSelector( + state => state.explore.form_data.start_date_offset, + ); useEffect(() => { - if (isEmpty(currentTimeRangeFilters) || isEmpty(shifts)) { + if (isEmpty(currentTimeRangeFilters) || (isEmpty(shifts) && !startDate)) { setLabels([]); - } else if (!isEmpty(shifts)) { - const promises = currentTimeRangeFilters.map(filter => - fetchTimeRange( + } else if (!isEmpty(shifts) || startDate) { + const promises = currentTimeRangeFilters.map(filter => { + const startDateShift = moment( + (filter as any).comparator.split(' : ')[0], + ).diff(moment(startDate), 'days'); + const newshift = startDateShift + ? [`${startDateShift} days ago`] + : shifts + ? shifts.slice(0, 1) + : undefined; + + return fetchTimeRange( filter.comparator, filter.subject, - multi ? shifts : shifts.slice(0, 1), - ), - ); + multi ? shifts : newshift, + ); + }); Promise.all(promises).then(res => { // access the value property inside the res and set the labels with it in the state setLabels(res.map(r => r.value ?? '')); }); } - }, [currentTimeRangeFilters, shifts]); + }, [currentTimeRangeFilters, shifts, startDate]); return labels.length ? ( <> diff --git a/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx b/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx new file mode 100644 index 0000000000000..bf192b7f3684c --- /dev/null +++ b/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx @@ -0,0 +1,112 @@ +/** + * 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 React, { ReactNode } from 'react'; +import { isEqual } from 'lodash'; +import moment, { Moment } from 'moment'; +import { BinaryAdhocFilter, SimpleAdhocFilter, css } from '@superset-ui/core'; +import { DatePicker } from 'antd'; +import { RangePickerProps } from 'antd/lib/date-picker'; +import { useSelector } from 'react-redux'; + +import ControlHeader from 'src/explore/components/ControlHeader'; +import { RootState } from 'src/views/store'; + +export interface TimeOffsetControlsProps { + label?: ReactNode; + startDate?: string; + description?: string; + hovered?: boolean; + value?: Moment; + onChange: (datetime: string) => void; +} +const MOMENT_FORMAT = 'YYYY-MM-DD'; +const dttmToMoment = (dttm: string): Moment => { + if (dttm === 'now') { + return moment().utc().startOf('second'); + } + if (dttm === 'today' || dttm === 'No filter') { + return moment().utc().startOf('day'); + } + if (dttm === 'last week') { + return moment().utc().startOf('day').subtract(7, 'day'); + } + if (dttm === 'last month') { + return moment().utc().startOf('day').subtract(1, 'month'); + } + if (dttm === 'last quarter') { + return moment().utc().startOf('day').subtract(1, 'quarter'); + } + if (dttm === 'last year') { + return moment().utc().startOf('day').subtract(1, 'year'); + } + if (dttm === 'previous calendar week') { + return moment().utc().subtract(1, 'weeks').startOf('isoWeek'); + } + if (dttm === 'previous calendar month') { + return moment().utc().subtract(1, 'months').startOf('month'); + } + if (dttm === 'previous calendar year') { + return moment().utc().subtract(1, 'years').startOf('year'); + } + + return moment(dttm); +}; +const isTimeRangeEqual = ( + left: BinaryAdhocFilter[], + right: BinaryAdhocFilter[], +) => isEqual(left, right); + +export default function TimeOffsetControls({ + onChange, + ...props +}: TimeOffsetControlsProps) { + const currentTimeRangeFilters = useSelector( + state => + state.explore.form_data.adhoc_filters.filter( + (adhoc_filter: SimpleAdhocFilter) => + adhoc_filter.operator === 'TEMPORAL_RANGE', + ), + isTimeRangeEqual, + ); + const startDate = currentTimeRangeFilters[0]?.comparator.split(' : ')[0]; + + const formatedDate = startDate + ? dttmToMoment(startDate) + : dttmToMoment('now'); + const disabledDate: RangePickerProps['disabledDate'] = current => + current && current >= formatedDate; + + return ( +
+ + + onChange(datetime ? datetime.format(MOMENT_FORMAT) : '') + } + defaultPickerValue={ + startDate ? moment(formatedDate).subtract(1, 'day') : undefined + } + disabledDate={disabledDate} + /> +
+ ); +} diff --git a/superset-frontend/src/explore/components/controls/index.js b/superset-frontend/src/explore/components/controls/index.js index 2bf2662d0c087..4c2cf00420823 100644 --- a/superset-frontend/src/explore/components/controls/index.js +++ b/superset-frontend/src/explore/components/controls/index.js @@ -34,6 +34,7 @@ import SpatialControl from './SpatialControl'; import TextAreaControl from './TextAreaControl'; import TextControl from './TextControl'; import TimeSeriesColumnControl from './TimeSeriesColumnControl'; +import TimeOffsetControl from './TimeOffsetControl'; import ViewportControl from './ViewportControl'; import VizTypeControl from './VizTypeControl'; import MetricsControl from './MetricControl/MetricsControl'; @@ -82,6 +83,7 @@ const controlMap = { XAxisSortControl, ContourControl, ComparisonRangeLabel, + TimeOffsetControl, ...sharedControlComponents, }; export default controlMap; From 72539d8907df88113c56c2ffa77cc647d8e94a49 Mon Sep 17 00:00:00 2001 From: lilykuang Date: Wed, 24 Apr 2024 11:13:22 -0700 Subject: [PATCH 04/27] fix time shift cherry picked from 6cacbe6519811d3d0d7907202f9d997336ce225f --- .../src/time-comparison/fetchTimeRange.ts | 2 +- .../components/controls/ComparisonRangeLabel.tsx | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts index 8de50d501bb30..b98ac4e35729c 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts @@ -63,7 +63,7 @@ export const fetchTimeRange = async ( ) => { let query; let endpoint; - if (!isEmpty(shifts)) { + if (shifts && !isEmpty(shifts)) { const timeRanges = shifts?.map(shift => ({ timeRange, shift, diff --git a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx index ff5008f2d2c92..616cb1698d605 100644 --- a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx +++ b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx @@ -66,13 +66,18 @@ export const ComparisonRangeLabel = ({ setLabels([]); } else if (!isEmpty(shifts) || startDate) { const promises = currentTimeRangeFilters.map(filter => { - const startDateShift = moment( - (filter as any).comparator.split(' : ')[0], - ).diff(moment(startDate), 'days'); + const startDateShift = + startDate && + moment((filter as any).comparator.split(' : ')[0]).diff( + moment(startDate), + 'days', + ); const newshift = startDateShift ? [`${startDateShift} days ago`] : shifts - ? shifts.slice(0, 1) + ? Array.isArray(shifts) + ? shifts + : [shifts] : undefined; return fetchTimeRange( From 9d3d149ff870cfc73da2b99cb07df61a9893ad4e Mon Sep 17 00:00:00 2001 From: lilykuang Date: Wed, 24 Apr 2024 17:55:38 -0700 Subject: [PATCH 05/27] custom and inherit start date cherry picked from d3d3b31453847244f7b17a58ded7d39ce4b5b514 --- .../src/sections/timeComparison.tsx | 4 + .../src/time-comparison/fetchTimeRange.ts | 12 +-- .../controls/ComparisonRangeLabel.tsx | 89 +++++++++++++++---- .../components/controls/TimeOffsetControl.tsx | 20 ++++- 4 files changed, 99 insertions(+), 26 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx index eb097340ed58f..fd65da257137d 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx @@ -49,6 +49,8 @@ export const timeComparisonControls: ControlPanelSectionConfig = { ['2 years ago', t('2 years ago')], ['156 weeks ago', t('156 weeks ago')], ['3 years ago', t('3 years ago')], + ['custom', t('Custom')], + ['inherit', t('Inherit')], ], description: t( 'Overlay one or more timeseries from a ' + @@ -65,6 +67,8 @@ export const timeComparisonControls: ControlPanelSectionConfig = { config: { type: 'TimeOffsetControl', label: t('shift start date'), + visibility: ({ controls }) => + controls?.time_compare.value === 'custom', }, }, ], diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts index b98ac4e35729c..8a130e225e584 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts @@ -64,11 +64,13 @@ export const fetchTimeRange = async ( let query; let endpoint; if (shifts && !isEmpty(shifts)) { - const timeRanges = shifts?.map(shift => ({ - timeRange, - shift, - })); - query = rison.encode_uri([{ timeRange }, ...(timeRanges || [])]); + const timeRanges = (Array.isArray(shifts) ? shifts : [shifts])?.map( + shift => ({ + timeRange, + shift, + }), + ); + query = rison.encode_uri([{ timeRange }, ...timeRanges]); endpoint = `/api/v1/time_range/?q=${query}`; } else { query = rison.encode_uri(timeRange); diff --git a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx index 616cb1698d605..18a2e3186a202 100644 --- a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx +++ b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx @@ -20,7 +20,7 @@ import React, { useEffect, useState } from 'react'; import { useSelector } from 'react-redux'; import { isEmpty, isEqual } from 'lodash'; -import moment from 'moment'; +import moment, { Moment } from 'moment'; import { BinaryAdhocFilter, css, @@ -42,6 +42,52 @@ type ComparisonRangeLabelProps = ControlHeaderProps & { multi?: boolean; }; +const dttmToMoment = (dttm: string): Moment => { + if (dttm === 'now') { + return moment().utc().startOf('second'); + } + if (dttm === 'today' || dttm === 'No filter') { + return moment().utc().startOf('day'); + } + if (dttm === 'Last week') { + return moment().utc().startOf('day').subtract(7, 'day'); + } + if (dttm === 'Last month') { + return moment().utc().startOf('day').subtract(1, 'month'); + } + if (dttm === 'Last quarter') { + return moment().utc().startOf('day').subtract(1, 'quarter'); + } + if (dttm === 'Last year') { + return moment().utc().startOf('day').subtract(1, 'year'); + } + if (dttm === 'previous calendar week') { + return moment().utc().subtract(1, 'weeks').startOf('isoWeek'); + } + if (dttm === 'previous calendar month') { + return moment().utc().subtract(1, 'months').startOf('month'); + } + if (dttm === 'previous calendar year') { + return moment().utc().subtract(1, 'years').startOf('year'); + } + if (dttm.includes('ago')) { + const parts = dttm.split(' '); + const amount = parseInt(parts[0], 10); + const unit = parts[1] as + | 'day' + | 'week' + | 'month' + | 'year' + | 'days' + | 'weeks' + | 'months' + | 'years'; + return moment().utc().subtract(amount, unit); + } + + return moment(dttm); +}; + export const ComparisonRangeLabel = ({ multi = true, }: ComparisonRangeLabelProps) => { @@ -57,6 +103,7 @@ export const ComparisonRangeLabel = ({ const shifts = useSelector( state => state.explore.form_data.time_compare, ); + console.log('lily shifts', shifts); const startDate = useSelector( state => state.explore.form_data.start_date_offset, ); @@ -65,26 +112,32 @@ export const ComparisonRangeLabel = ({ if (isEmpty(currentTimeRangeFilters) || (isEmpty(shifts) && !startDate)) { setLabels([]); } else if (!isEmpty(shifts) || startDate) { + const isCustom = shifts?.includes('custom'); + const isInherit = shifts?.includes('inherit'); const promises = currentTimeRangeFilters.map(filter => { - const startDateShift = - startDate && - moment((filter as any).comparator.split(' : ')[0]).diff( - moment(startDate), + const customStartDate = isCustom && startDate; + const customShift = + customStartDate && + moment(dttmToMoment((filter as any).comparator.split(' : ')[0])).diff( + moment(customStartDate), + 'days', + ) + 1; + + const inInheritShift = + isInherit && + moment(dttmToMoment((filter as any).comparator.split(' : ')[0])).diff( + moment(dttmToMoment((filter as any).comparator.split(' : ')[1])), 'days', - ); - const newshift = startDateShift - ? [`${startDateShift} days ago`] - : shifts - ? Array.isArray(shifts) - ? shifts - : [shifts] - : undefined; + ) + 1; + let newShifts = shifts; + if (isCustom) { + newShifts = [`${customShift} days ago`]; + } + if (isInherit) { + newShifts = [`${inInheritShift} days ago`]; + } - return fetchTimeRange( - filter.comparator, - filter.subject, - multi ? shifts : newshift, - ); + return fetchTimeRange(filter.comparator, filter.subject, newShifts); }); Promise.all(promises).then(res => { // access the value property inside the res and set the labels with it in the state diff --git a/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx b/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx index bf192b7f3684c..806335560a9f7 100644 --- a/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx +++ b/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx @@ -64,6 +64,20 @@ const dttmToMoment = (dttm: string): Moment => { if (dttm === 'previous calendar year') { return moment().utc().subtract(1, 'years').startOf('year'); } + if (dttm.includes('ago')) { + const parts = dttm.split(' '); + const amount = parseInt(parts[0], 10); + const unit = parts[1] as + | 'day' + | 'week' + | 'month' + | 'year' + | 'days' + | 'weeks' + | 'months' + | 'years'; + return moment().utc().subtract(amount, unit); + } return moment(dttm); }; @@ -84,11 +98,10 @@ export default function TimeOffsetControls({ ), isTimeRangeEqual, ); + const startDate = currentTimeRangeFilters[0]?.comparator.split(' : ')[0]; - const formatedDate = startDate - ? dttmToMoment(startDate) - : dttmToMoment('now'); + const formatedDate = dttmToMoment(startDate); const disabledDate: RangePickerProps['disabledDate'] = current => current && current >= formatedDate; @@ -106,6 +119,7 @@ export default function TimeOffsetControls({ startDate ? moment(formatedDate).subtract(1, 'day') : undefined } disabledDate={disabledDate} + defaultValue={formatedDate} /> ); From 57e7aed114d42b551d7380755c2a7d78180190ca Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Thu, 25 Apr 2024 13:26:59 +0200 Subject: [PATCH 06/27] Table with Time Comparison: - When calling the time range method ensure we pass an array so we can remove the array check and follow the method signature --- .../src/time-comparison/fetchTimeRange.ts | 10 ++++------ .../components/controls/ComparisonRangeLabel.tsx | 8 ++++++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts index 8a130e225e584..8dff2d9ac9221 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts @@ -64,12 +64,10 @@ export const fetchTimeRange = async ( let query; let endpoint; if (shifts && !isEmpty(shifts)) { - const timeRanges = (Array.isArray(shifts) ? shifts : [shifts])?.map( - shift => ({ - timeRange, - shift, - }), - ); + const timeRanges = shifts?.map(shift => ({ + timeRange, + shift, + })); query = rison.encode_uri([{ timeRange }, ...timeRanges]); endpoint = `/api/v1/time_range/?q=${query}`; } else { diff --git a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx index 18a2e3186a202..3f3029b35badf 100644 --- a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx +++ b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx @@ -24,6 +24,7 @@ import moment, { Moment } from 'moment'; import { BinaryAdhocFilter, css, + ensureIsArray, fetchTimeRange, SimpleAdhocFilter, t, @@ -103,7 +104,6 @@ export const ComparisonRangeLabel = ({ const shifts = useSelector( state => state.explore.form_data.time_compare, ); - console.log('lily shifts', shifts); const startDate = useSelector( state => state.explore.form_data.start_date_offset, ); @@ -137,7 +137,11 @@ export const ComparisonRangeLabel = ({ newShifts = [`${inInheritShift} days ago`]; } - return fetchTimeRange(filter.comparator, filter.subject, newShifts); + return fetchTimeRange( + filter.comparator, + filter.subject, + ensureIsArray(newShifts), + ); }); Promise.all(promises).then(res => { // access the value property inside the res and set the labels with it in the state From 908cea354be46993e38e76da5641bb177ee02338 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Wed, 24 Apr 2024 17:11:33 +0200 Subject: [PATCH 07/27] Fixes cherry picked from 669d060ab1f94218449c609596c04f6c6f924e41 --- .../src/sections/timeComparison.tsx | 17 ++++++++++++++--- .../BigNumberPeriodOverPeriod/buildQuery.ts | 3 ++- .../BigNumberPeriodOverPeriod/controlPanel.ts | 16 +--------------- .../BigNumberPeriodOverPeriod/transformProps.ts | 3 ++- .../controls/ComparisonRangeLabel.tsx | 8 ++++++-- 5 files changed, 25 insertions(+), 22 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx index fd65da257137d..d60ff93cb0779 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx @@ -20,7 +20,9 @@ import { t, ComparisonType } from '@superset-ui/core'; import { ControlPanelSectionConfig } from '../types'; -export const timeComparisonControls: ControlPanelSectionConfig = { +export const timeComparisonControls: ( + multi?: boolean, +) => ControlPanelSectionConfig = (multi = true) => ({ label: t('Time Comparison'), tabOverride: 'data', description: t( @@ -35,7 +37,7 @@ export const timeComparisonControls: ControlPanelSectionConfig = { name: 'time_compare', config: { type: 'SelectControl', - multi: true, + multi, freeForm: true, label: t('Time shift'), choices: [ @@ -94,5 +96,14 @@ export const timeComparisonControls: ControlPanelSectionConfig = { }, }, ], + [ + { + name: 'comparison_range_label', + config: { + type: 'ComparisonRangeLabel', + multi, + }, + }, + ], ], -}; +}); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts index 167ff6a24d8e3..1de563dd51eb1 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts @@ -20,6 +20,7 @@ import { buildQueryContext, QueryFormData, PostProcessingRule, + ensureIsArray, } from '@superset-ui/core'; import { isTimeComparison, @@ -47,7 +48,7 @@ export default function buildQuery(formData: QueryFormData) { groupby, post_processing: postProcessing, time_offsets: isTimeComparison(formData, baseQueryObject) - ? formData.time_compare + ? ensureIsArray(formData.time_compare) : [], }, ]; diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts index cb4a6b842b10e..3507f6566dbbb 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts @@ -108,21 +108,7 @@ const config: ControlPanelConfig = { ], ], }, - { - ...sections.timeComparisonControls, - controlSetRows: [ - ...sections.timeComparisonControls.controlSetRows, - [ - { - name: 'comparison_range_label', - config: { - type: 'ComparisonRangeLabel', - multi: false, - }, - }, - ], - ], - }, + sections.timeComparisonControls(false), ], controlOverrides: { y_axis_format: { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts index 3fcc4704ca422..d9e909ed2a650 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts @@ -23,6 +23,7 @@ import { getValueFormatter, getNumberFormatter, SimpleAdhocFilter, + ensureIsArray, } from '@superset-ui/core'; import { getComparisonFontSize, getHeaderFontSize } from './utils'; @@ -89,7 +90,7 @@ export default function transformProps(chartProps: ChartProps) { const { data: dataA = [] } = queriesData[0]; const data = dataA; const metricName = getMetricLabel(metric); - const timeComparison = chartProps.rawFormData?.time_compare?.[0]; + const timeComparison = ensureIsArray(chartProps.rawFormData?.time_compare)[0]; const startDateOffset = chartProps.rawFormData?.start_date_offset; const currentTimeRangeFilter = chartProps.rawFormData?.adhoc_filters?.filter( (adhoc_filter: SimpleAdhocFilter) => diff --git a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx index 3f3029b35badf..379130346621f 100644 --- a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx +++ b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx @@ -109,7 +109,11 @@ export const ComparisonRangeLabel = ({ ); useEffect(() => { - if (isEmpty(currentTimeRangeFilters) || (isEmpty(shifts) && !startDate)) { + const shiftsArray = ensureIsArray(shifts); + if ( + isEmpty(currentTimeRangeFilters) || + (isEmpty(shiftsArray) && !startDate) + ) { setLabels([]); } else if (!isEmpty(shifts) || startDate) { const isCustom = shifts?.includes('custom'); @@ -140,7 +144,7 @@ export const ComparisonRangeLabel = ({ return fetchTimeRange( filter.comparator, filter.subject, - ensureIsArray(newShifts), + multi ? shiftsArray : ensureIsArray(newShifts), ); }); Promise.all(promises).then(res => { From 333e93ea33ff086caab75b9a9637e3e110591844 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Wed, 24 Apr 2024 17:24:23 +0200 Subject: [PATCH 08/27] Improve coverage cherry picked from c1f73ab9611b14d466074739b788f06f05104684 --- .../src/time-comparison/fetchTimeRange.ts | 10 ++-- .../time-comparison/fetchTimeRange.test.ts | 52 +++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts index 8dff2d9ac9221..61c2a2f8ad306 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/fetchTimeRange.ts @@ -17,8 +17,12 @@ * under the License. */ import rison from 'rison'; -import { SupersetClient, getClientErrorObject } from '@superset-ui/core'; import { isEmpty } from 'lodash'; +import { + SupersetClient, + getClientErrorObject, + ensureIsArray, +} from '@superset-ui/core'; export const SEPARATOR = ' : '; @@ -63,8 +67,8 @@ export const fetchTimeRange = async ( ) => { let query; let endpoint; - if (shifts && !isEmpty(shifts)) { - const timeRanges = shifts?.map(shift => ({ + if (!isEmpty(shifts)) { + const timeRanges = ensureIsArray(shifts).map(shift => ({ timeRange, shift, })); diff --git a/superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts b/superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts index e07fa8617f459..84644564f66c0 100644 --- a/superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts @@ -22,6 +22,7 @@ import { fetchTimeRange } from '@superset-ui/core'; import { buildTimeRangeString, formatTimeRange, + formatTimeRangeComparison, } from '../../src/time-comparison/fetchTimeRange'; afterEach(fetchMock.restore); @@ -116,3 +117,54 @@ it('returns a formatted error message from response', async () => { error: 'Network error', }); }); + +it('fetchTimeRange with shift', async () => { + fetchMock.getOnce( + "glob:*/api/v1/time_range/?q=!((timeRange:'Last+day'),(shift%3A'last%20month'%2CtimeRange%3A'Last%20day'))", + { + result: [ + { + since: '2021-04-13T00:00:00', + until: '2021-04-14T00:00:00', + timeRange: 'Last day', + shift: null, + }, + { + since: '2021-03-13T00:00:00', + until: '2021-03-14T00:00:00', + timeRange: 'Last day', + shift: 'last month', + }, + ], + }, + ); + + const timeRange = await fetchTimeRange('Last day', 'temporal_col', [ + 'last month', + ]); + + expect(timeRange).toEqual({ + value: [ + 'temporal_col: 2021-04-13 to 2021-04-14 vs\n 2021-03-13 to 2021-03-14', + ], + }); +}); + +it('formatTimeRangeComparison', () => { + expect( + formatTimeRangeComparison( + '2021-04-13T00:00:00 : 2021-04-14T00:00:00', + '2021-03-13T00:00:00 : 2021-03-14T00:00:00', + ), + ).toEqual('col: 2021-04-13 to 2021-04-14 vs\n 2021-03-13 to 2021-03-14'); + + expect( + formatTimeRangeComparison( + '2021-04-13T00:00:00 : 2021-04-14T00:00:00', + '2021-03-13T00:00:00 : 2021-03-14T00:00:00', + 'col_name', + ), + ).toEqual( + 'col_name: 2021-04-13 to 2021-04-14 vs\n 2021-03-13 to 2021-03-14', + ); +}); From 701a38b885b8c64a6c084cd331721d2a17317e8a Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Thu, 25 Apr 2024 16:46:25 +0200 Subject: [PATCH 09/27] Big Number with Time Comparison: - Remove time grain from time_comparison controls --- .../src/sections/timeComparison.tsx | 1 - .../src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx index d60ff93cb0779..4f7bd4a0c6f87 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx @@ -74,7 +74,6 @@ export const timeComparisonControls: ( }, }, ], - ['time_grain_sqla'], [ { name: 'comparison_type', diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts index 1de563dd51eb1..44495554678fc 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts @@ -28,7 +28,7 @@ import { } from '@superset-ui/chart-controls'; export default function buildQuery(formData: QueryFormData) { - const { cols: groupby, time_grain_sqla } = formData; + const { cols: groupby } = formData; const queryContextA = buildQueryContext(formData, baseQueryObject => { const postProcessing: PostProcessingRule[] = []; @@ -38,7 +38,7 @@ export default function buildQuery(formData: QueryFormData) { ...baseQueryObject, columns: [ { - timeGrain: time_grain_sqla || 'P1Y', // Group by year by default + timeGrain: 'P1Y', // Group by year by default columnType: 'BASE_AXIS', sqlExpression: baseQueryObject.filters?.[0]?.col.toString() || '', label: baseQueryObject.filters?.[0]?.col.toString() || '', From b90fa307047543c9ecba1bab0ba282a1a2733fef Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Wed, 24 Apr 2024 00:01:10 +0200 Subject: [PATCH 10/27] Table with Time Comparison: - Support textual columns in time_offsets comparisons - Because textual columns have no time grain nor they use xaxis labels all the time, the time_grain and xaxis are not required for the comparison queries that use textual columns only cherry picked from 5819e3a527c67c4357db0cef5e2e8b0d47624d37 --- superset/common/query_context_processor.py | 96 ++++++++++++++-------- superset/common/utils/time_range_utils.py | 8 +- 2 files changed, 64 insertions(+), 40 deletions(-) diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index c47e295e96c52..ba1a43bc73075 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -19,7 +19,7 @@ import copy import logging import re -from typing import Any, ClassVar, TYPE_CHECKING, TypedDict +from typing import Any, cast, ClassVar, TYPE_CHECKING, TypedDict import numpy as np import pandas as pd @@ -55,6 +55,7 @@ DateColumn, DTTM_ALIAS, error_msg_from_exception, + FilterOperator, get_base_axis_labels, get_column_names_from_columns, get_column_names_from_metrics, @@ -390,11 +391,6 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme time_grain = self.get_time_grain(query_object) - if not time_grain: - raise QueryObjectValidationError( - _("Time Grain must be specified when using Time Shift.") - ) - metric_names = get_metric_names(query_object.metrics) # use columns that are not metrics as join keys @@ -429,6 +425,28 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme query_object_clone.inner_to_dttm = outer_to_dttm query_object_clone.time_offsets = [] query_object_clone.post_processing = [] + # Get time offset index + index = (get_base_axis_labels(query_object.columns) or [DTTM_ALIAS])[0] + # The comparison is not using a temporal column so we need to modify + # the temporal filter so we run the query with the correct time range + if not dataframe_utils.is_datetime_series(df.get(index)): + # Lets find the first temporal filter in the filters array and change + # its val to be the result of get_since_until with the offset + for flt in query_object_clone.filter: + if flt.get( + "op" + ) == FilterOperator.TEMPORAL_RANGE.value and isinstance( + flt.get("val"), str + ): + time_range = cast(str, flt.get("val")) + ( + new_outer_from_dttm, + new_outer_to_dttm, + ) = get_since_until_from_time_range( + time_range=time_range, + time_shift=offset, + ) + flt["val"] = f"{new_outer_from_dttm} : {new_outer_to_dttm}" query_object_clone.filter = [ flt for flt in query_object_clone.filter @@ -488,16 +506,6 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme # 2. rename extra query columns offset_metrics_df = offset_metrics_df.rename(columns=metrics_mapping) - # 3. set time offset for index - index = (get_base_axis_labels(query_object.columns) or [DTTM_ALIAS])[0] - if not dataframe_utils.is_datetime_series(offset_metrics_df.get(index)): - raise QueryObjectValidationError( - _( - "A time column must be specified " - "when using a Time Comparison." - ) - ) - # cache df and query value = { "df": offset_metrics_df, @@ -526,7 +534,7 @@ def join_offset_dfs( self, df: pd.DataFrame, offset_dfs: dict[str, pd.DataFrame], - time_grain: str, + time_grain: str | None, join_keys: list[str], ) -> pd.DataFrame: """ @@ -541,36 +549,54 @@ def join_offset_dfs( time_grain ) + use_aggregated_join_column = ( + join_column_producer or time_grain in AGGREGATED_JOIN_GRAINS + ) + + if use_aggregated_join_column: + if not time_grain: + raise QueryObjectValidationError( + _("Time Grain must be specified when using Time Shift.") + ) + # iterate on offset_dfs, left join each with df for offset, offset_df in offset_dfs.items(): # defines a column name for the offset join column column_name = OFFSET_JOIN_COLUMN_SUFFIX + offset - # add offset join column to df - self.add_offset_join_column( - df, column_name, time_grain, offset, join_column_producer - ) + if use_aggregated_join_column: + # add offset join column to df + self.add_offset_join_column( + df, column_name, time_grain, offset, join_column_producer + ) - # add offset join column to offset_df - self.add_offset_join_column( - offset_df, column_name, time_grain, None, join_column_producer - ) + # add offset join column to offset_df + self.add_offset_join_column( + offset_df, column_name, time_grain, None, join_column_producer + ) # the temporal column is the first column in the join keys # so we use the join column instead of the temporal column actual_join_keys = [column_name, *join_keys[1:]] - # left join df with offset_df - df = dataframe_utils.left_join_df( - left_df=df, - right_df=offset_df, - join_keys=actual_join_keys, - rsuffix=R_SUFFIX, - ) + if join_keys: + df = dataframe_utils.left_join_df( + left_df=df, + right_df=offset_df, + join_keys=actual_join_keys, + rsuffix=R_SUFFIX, + ) + else: + df = dataframe_utils.full_outer_join_df( + left_df=df, + right_df=offset_df, + rsuffix=R_SUFFIX, + ) - # move the temporal column to the first column in df - col = df.pop(join_keys[0]) - df.insert(0, col.name, col) + if use_aggregated_join_column: + # move the temporal column to the first column in df + col = df.pop(join_keys[0]) + df.insert(0, col.name, col) # removes columns created only for join purposes df.drop( diff --git a/superset/common/utils/time_range_utils.py b/superset/common/utils/time_range_utils.py index 2ceb9f766e58a..4969988657c06 100644 --- a/superset/common/utils/time_range_utils.py +++ b/superset/common/utils/time_range_utils.py @@ -21,7 +21,7 @@ from superset import app from superset.common.query_object import QueryObject -from superset.utils.core import FilterOperator, get_xaxis_label +from superset.utils.core import FilterOperator from superset.utils.date_parser import get_since_until @@ -66,10 +66,8 @@ def get_since_until_from_query_object( time_range = None for flt in query_object.filter: - if ( - flt.get("op") == FilterOperator.TEMPORAL_RANGE.value - and flt.get("col") == get_xaxis_label(query_object.columns) - and isinstance(flt.get("val"), str) + if flt.get("op") == FilterOperator.TEMPORAL_RANGE.value and isinstance( + flt.get("val"), str ): time_range = cast(str, flt.get("val")) From 41f52d9228f3a808ec25fabddc13813da3bc996b Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Thu, 25 Apr 2024 18:05:30 +0200 Subject: [PATCH 11/27] Table with Time Comparison: - Support time comparison with no columns, meaning we want to compare just metrics. If so, we do an outer join --- superset/common/utils/dataframe_utils.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/superset/common/utils/dataframe_utils.py b/superset/common/utils/dataframe_utils.py index 7772ec58509bb..89563dec06f72 100644 --- a/superset/common/utils/dataframe_utils.py +++ b/superset/common/utils/dataframe_utils.py @@ -40,6 +40,17 @@ def left_join_df( return df +def full_outer_join_df( + left_df: pd.DataFrame, + right_df: pd.DataFrame, + lsuffix: str = "", + rsuffix: str = "", +) -> pd.DataFrame: + df = left_df.join(right_df, lsuffix=lsuffix, rsuffix=rsuffix, how="outer") + df.reset_index(inplace=True) + return df + + def df_metrics_to_num(df: pd.DataFrame, query_object: QueryObject) -> None: """Converting metrics to numeric when pandas.read_sql cannot""" for col, dtype in df.dtypes.items(): From 29b988050e6a662cece7c96a0595af4852356687 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Thu, 25 Apr 2024 18:26:29 +0200 Subject: [PATCH 12/27] BigNumber with Time Comparison: - Do not pass a column because it's not required anymore --- .../BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts index 44495554678fc..09f6f05b453e0 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts @@ -36,15 +36,6 @@ export default function buildQuery(formData: QueryFormData) { return [ { ...baseQueryObject, - columns: [ - { - timeGrain: 'P1Y', // Group by year by default - columnType: 'BASE_AXIS', - sqlExpression: baseQueryObject.filters?.[0]?.col.toString() || '', - label: baseQueryObject.filters?.[0]?.col.toString() || '', - expressionType: 'SQL', - }, - ], groupby, post_processing: postProcessing, time_offsets: isTimeComparison(formData, baseQueryObject) From 74e31270091b66b5194ac08013280db0b2ef38d0 Mon Sep 17 00:00:00 2001 From: lilykuang Date: Thu, 25 Apr 2024 21:48:41 -0700 Subject: [PATCH 13/27] added getTimeOffset --- .../packages/superset-ui-core/package.json | 1 + .../src/time-comparison/getTimeOffset.ts | 101 ++++++++++++++++++ .../src/time-comparison/index.ts | 1 + .../BigNumberPeriodOverPeriod/buildQuery.ts | 18 +++- .../controls/ComparisonRangeLabel.tsx | 74 +------------ 5 files changed, 123 insertions(+), 72 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts diff --git a/superset-frontend/packages/superset-ui-core/package.json b/superset-frontend/packages/superset-ui-core/package.json index 1a26e0f49d275..fb428e01a0e93 100644 --- a/superset-frontend/packages/superset-ui-core/package.json +++ b/superset-frontend/packages/superset-ui-core/package.json @@ -51,6 +51,7 @@ "jed": "^1.1.1", "lodash": "^4.17.11", "math-expression-evaluator": "^1.3.8", + "moment": "^2.30.1", "pretty-ms": "^7.0.0", "react-error-boundary": "^1.2.5", "react-markdown": "^8.0.3", diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts new file mode 100644 index 0000000000000..97ced3c7132ee --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts @@ -0,0 +1,101 @@ +/** + * 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 moment, { Moment } from 'moment'; +import { ensureIsArray } from '../utils'; + +export const parseDttmToMoment = (dttm: string): Moment => { + if (dttm === 'now') { + return moment().utc().startOf('second'); + } + if (dttm === 'today' || dttm === 'No filter') { + return moment().utc().startOf('day'); + } + if (dttm === 'Last week') { + return moment().utc().startOf('day').subtract(7, 'day'); + } + if (dttm === 'Last month') { + return moment().utc().startOf('day').subtract(1, 'month'); + } + if (dttm === 'Last quarter') { + return moment().utc().startOf('day').subtract(1, 'quarter'); + } + if (dttm === 'Last year') { + return moment().utc().startOf('day').subtract(1, 'year'); + } + if (dttm === 'previous calendar week') { + return moment().utc().subtract(1, 'weeks').startOf('isoWeek'); + } + if (dttm === 'previous calendar month') { + return moment().utc().subtract(1, 'months').startOf('month'); + } + if (dttm === 'previous calendar year') { + return moment().utc().subtract(1, 'years').startOf('year'); + } + if (dttm.includes('ago')) { + const parts = dttm.split(' '); + const amount = parseInt(parts[0], 10); + const unit = parts[1] as + | 'day' + | 'week' + | 'month' + | 'year' + | 'days' + | 'weeks' + | 'months' + | 'years'; + return moment().utc().subtract(amount, unit); + } + + return moment(dttm); +}; + +export const getTimeOffset = ( + timeRangeFilter: any, + shifts: string[], + startDate: string, +): string[] => { + const isCustom = shifts?.includes('custom'); + const isInherit = shifts?.includes('inherit'); + + const customStartDate = isCustom && startDate; + const customShift = + customStartDate && + moment( + parseDttmToMoment((timeRangeFilter as any).comparator.split(' : ')[0]), + ).diff(moment(customStartDate), 'days'); + + const inInheritShift = + isInherit && + moment( + parseDttmToMoment((timeRangeFilter as any).comparator.split(' : ')[1]), + ).diff( + moment( + parseDttmToMoment((timeRangeFilter as any).comparator.split(' : ')[0]), + ), + 'days', + ); + let newShifts = shifts; + if (isCustom) { + newShifts = [`${customShift} days ago`]; + } + if (isInherit) { + newShifts = [`${inInheritShift} days ago`]; + } + return ensureIsArray(newShifts); +}; diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/index.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/index.ts index ad5b5f59182ae..208b6ecc4450b 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/index.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/index.ts @@ -21,4 +21,5 @@ export * from './types'; export { default as getComparisonInfo } from './getComparisonInfo'; export { default as getComparisonFilters } from './getComparisonFilters'; +export { parseDttmToMoment, getTimeOffset } from './getTimeOffset'; export { SEPARATOR, fetchTimeRange } from './fetchTimeRange'; diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts index 09f6f05b453e0..f9b4725cb08c7 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts @@ -21,6 +21,8 @@ import { QueryFormData, PostProcessingRule, ensureIsArray, + SimpleAdhocFilter, + getTimeOffset, } from '@superset-ui/core'; import { isTimeComparison, @@ -33,13 +35,27 @@ export default function buildQuery(formData: QueryFormData) { const queryContextA = buildQueryContext(formData, baseQueryObject => { const postProcessing: PostProcessingRule[] = []; postProcessing.push(timeCompareOperator(formData, baseQueryObject)); + const TimeRangeFilters = + formData.adhoc_filters?.filter( + (filter: SimpleAdhocFilter) => filter.operator === 'TEMPORAL_RANGE', + ) || []; + + const timeOffsets = ensureIsArray( + isTimeComparison(formData, baseQueryObject) + ? getTimeOffset( + TimeRangeFilters[0], + formData.time_compare, + formData.start_date_offset, + ) + : [], + ); return [ { ...baseQueryObject, groupby, post_processing: postProcessing, time_offsets: isTimeComparison(formData, baseQueryObject) - ? ensureIsArray(formData.time_compare) + ? ensureIsArray(timeOffsets) : [], }, ]; diff --git a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx index 379130346621f..cb20396989277 100644 --- a/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx +++ b/superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx @@ -20,12 +20,12 @@ import React, { useEffect, useState } from 'react'; import { useSelector } from 'react-redux'; import { isEmpty, isEqual } from 'lodash'; -import moment, { Moment } from 'moment'; import { BinaryAdhocFilter, css, ensureIsArray, fetchTimeRange, + getTimeOffset, SimpleAdhocFilter, t, } from '@superset-ui/core'; @@ -43,52 +43,6 @@ type ComparisonRangeLabelProps = ControlHeaderProps & { multi?: boolean; }; -const dttmToMoment = (dttm: string): Moment => { - if (dttm === 'now') { - return moment().utc().startOf('second'); - } - if (dttm === 'today' || dttm === 'No filter') { - return moment().utc().startOf('day'); - } - if (dttm === 'Last week') { - return moment().utc().startOf('day').subtract(7, 'day'); - } - if (dttm === 'Last month') { - return moment().utc().startOf('day').subtract(1, 'month'); - } - if (dttm === 'Last quarter') { - return moment().utc().startOf('day').subtract(1, 'quarter'); - } - if (dttm === 'Last year') { - return moment().utc().startOf('day').subtract(1, 'year'); - } - if (dttm === 'previous calendar week') { - return moment().utc().subtract(1, 'weeks').startOf('isoWeek'); - } - if (dttm === 'previous calendar month') { - return moment().utc().subtract(1, 'months').startOf('month'); - } - if (dttm === 'previous calendar year') { - return moment().utc().subtract(1, 'years').startOf('year'); - } - if (dttm.includes('ago')) { - const parts = dttm.split(' '); - const amount = parseInt(parts[0], 10); - const unit = parts[1] as - | 'day' - | 'week' - | 'month' - | 'year' - | 'days' - | 'weeks' - | 'months' - | 'years'; - return moment().utc().subtract(amount, unit); - } - - return moment(dttm); -}; - export const ComparisonRangeLabel = ({ multi = true, }: ComparisonRangeLabelProps) => { @@ -116,35 +70,13 @@ export const ComparisonRangeLabel = ({ ) { setLabels([]); } else if (!isEmpty(shifts) || startDate) { - const isCustom = shifts?.includes('custom'); - const isInherit = shifts?.includes('inherit'); const promises = currentTimeRangeFilters.map(filter => { - const customStartDate = isCustom && startDate; - const customShift = - customStartDate && - moment(dttmToMoment((filter as any).comparator.split(' : ')[0])).diff( - moment(customStartDate), - 'days', - ) + 1; - - const inInheritShift = - isInherit && - moment(dttmToMoment((filter as any).comparator.split(' : ')[0])).diff( - moment(dttmToMoment((filter as any).comparator.split(' : ')[1])), - 'days', - ) + 1; - let newShifts = shifts; - if (isCustom) { - newShifts = [`${customShift} days ago`]; - } - if (isInherit) { - newShifts = [`${inInheritShift} days ago`]; - } + const newShifts = getTimeOffset(filter, shiftsArray, startDate); return fetchTimeRange( filter.comparator, filter.subject, - multi ? shiftsArray : ensureIsArray(newShifts), + ensureIsArray(newShifts), ); }); Promise.all(promises).then(res => { From 2407e006f666b6e4ffc0b133a5fa25af87bec3c7 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Fri, 26 Apr 2024 10:45:46 +0200 Subject: [PATCH 14/27] BigNumber with Time Comparison: - Use the new getTimeOffset to render the correct time range in the tooltip - Update the lock file with moment --- superset-frontend/package-lock.json | 2 ++ .../BigNumberPeriodOverPeriod/PopKPI.tsx | 22 +++++++++++++------ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index deaa3748f5983..039d7a6c0bb2f 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -67531,6 +67531,7 @@ "jed": "^1.1.1", "lodash": "^4.17.11", "math-expression-evaluator": "^1.3.8", + "moment": "^2.30.1", "pretty-ms": "^7.0.0", "react-error-boundary": "^1.2.5", "react-markdown": "^8.0.3", @@ -86238,6 +86239,7 @@ "jest-mock-console": "^1.0.0", "lodash": "^4.17.11", "math-expression-evaluator": "^1.3.8", + "moment": "^2.30.1", "pretty-ms": "^7.0.0", "react-error-boundary": "^1.2.5", "react-markdown": "^8.0.3", diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx index 0ca7625f4a95a..d8d373aeb3377 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx @@ -17,10 +17,17 @@ * under the License. */ import React, { useEffect, useMemo, useState } from 'react'; -import { css, fetchTimeRange, styled, t, useTheme } from '@superset-ui/core'; +import { + css, + ensureIsArray, + fetchTimeRange, + getTimeOffset, + styled, + t, + useTheme, +} from '@superset-ui/core'; import { Tooltip } from '@superset-ui/chart-controls'; import { isEmpty } from 'lodash'; -import moment from 'moment'; import { ColorSchemeEnum, PopKPIComparisonSymbolStyleProps, @@ -82,14 +89,15 @@ export default function PopKPI(props: PopKPIProps) { if (!currentTimeRangeFilter || (!shift && !startDateOffset)) { setComparisonRange(''); } else if (!isEmpty(shift) || startDateOffset) { - const startDateShift = moment( - (currentTimeRangeFilter as any).comparator.split(' : ')[0], - ).diff(moment(startDateOffset), 'days'); - const newshift = startDateShift ? `${startDateShift} days ago` : shift; + const newShift = getTimeOffset( + currentTimeRangeFilter, + ensureIsArray(shift), + startDateOffset || '', + ); const promise: any = fetchTimeRange( (currentTimeRangeFilter as any).comparator, currentTimeRangeFilter.subject, - newshift ? [newshift] : [], + newShift || [], ); Promise.resolve(promise).then((res: any) => { const response: string[] = res.value; From fcc37975c3502f7cd7eb5a38f43d2bd21c6cf0b3 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Fri, 26 Apr 2024 11:03:20 +0200 Subject: [PATCH 15/27] BigNumber with Time Comparison: - Process data when custom or inherit offset is used - Handle responses from time_range api that don't include the comparison ranges --- .../BigNumberPeriodOverPeriod/PopKPI.tsx | 7 +++++-- .../transformProps.ts | 19 ++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx index d8d373aeb3377..5292432fd21f9 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx @@ -100,9 +100,12 @@ export default function PopKPI(props: PopKPIProps) { newShift || [], ); Promise.resolve(promise).then((res: any) => { - const response: string[] = res.value; + const response: string[] = ensureIsArray(res.value); const firstRange: string = response.flat()[0]; - setComparisonRange(firstRange.split('vs\n')[1].trim()); + const rangeText = firstRange.split('vs\n'); + setComparisonRange( + rangeText.length > 1 ? rangeText[1].trim() : rangeText[0], + ); }); } }, [currentTimeRangeFilter, shift, startDateOffset]); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts index d9e909ed2a650..1039df0de9d20 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts @@ -24,6 +24,7 @@ import { getNumberFormatter, SimpleAdhocFilter, ensureIsArray, + getTimeOffset, } from '@superset-ui/core'; import { getComparisonFontSize, getHeaderFontSize } from './utils'; @@ -96,11 +97,27 @@ export default function transformProps(chartProps: ChartProps) { (adhoc_filter: SimpleAdhocFilter) => adhoc_filter.operator === 'TEMPORAL_RANGE', )?.[0]; + const isCustomOrInherit = + timeComparison === 'custom' || timeComparison === 'inherit'; + let dataOffset: string[] = []; + if (isCustomOrInherit) { + dataOffset = getTimeOffset( + currentTimeRangeFilter, + ensureIsArray(timeComparison), + startDateOffset || '', + ); + } const { value1, value2 } = data.reduce( (acc: { value1: number; value2: number }, curr: { [x: string]: any }) => { Object.keys(curr).forEach(key => { - if (key.includes(`${metricName}__${timeComparison}`)) { + if ( + key.includes( + `${metricName}__${ + !isCustomOrInherit ? timeComparison : dataOffset[0] + }`, + ) + ) { acc.value2 += curr[key]; } else if (key.includes(metricName)) { acc.value1 += curr[key]; From 7f6634829cedf9f479b26fa2b2264be86039f4a0 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Fri, 26 Apr 2024 11:17:41 +0200 Subject: [PATCH 16/27] BigNumber with Time Comparison: - Add tests for the new getTimeOffset method - Hide label if no comparison is being made --- .../src/sections/timeComparison.tsx | 1 + .../time-comparison/getTimeOffset.test.ts | 113 ++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx index 4f7bd4a0c6f87..6fe8a8152aa8e 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx @@ -101,6 +101,7 @@ export const timeComparisonControls: ( config: { type: 'ComparisonRangeLabel', multi, + visibility: ({ controls }) => Boolean(controls?.time_compare.value), }, }, ], diff --git a/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts b/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts new file mode 100644 index 0000000000000..c66816366654c --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts @@ -0,0 +1,113 @@ +/** + * 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 { getTimeOffset, parseDttmToMoment } from '@superset-ui/core'; +import moment from 'moment'; + +describe('parseDttmToMoment', () => { + it('should handle "now"', () => { + const now = parseDttmToMoment('now'); + expect(now.utc().format()).toEqual( + moment().utc().startOf('second').format(), + ); + }); + + it('should handle "today" and "No filter"', () => { + const today = parseDttmToMoment('today'); + const noFilter = parseDttmToMoment('No filter'); + const expected = moment().utc().startOf('day').format(); + expect(today.format()).toEqual(expected); + expect(noFilter.format()).toEqual(expected); + }); + + it('should handle relative time strings', () => { + const lastWeek = parseDttmToMoment('Last week'); + const lastMonth = parseDttmToMoment('Last month'); + const lastQuarter = parseDttmToMoment('Last quarter'); + const lastYear = parseDttmToMoment('Last year'); + expect(lastWeek.format()).toEqual( + moment().utc().startOf('day').subtract(7, 'days').format(), + ); + expect(lastMonth.format()).toEqual( + moment().utc().startOf('day').subtract(1, 'months').format(), + ); + expect(lastQuarter.format()).toEqual( + moment().utc().startOf('day').subtract(1, 'quarters').format(), + ); + expect(lastYear.format()).toEqual( + moment().utc().startOf('day').subtract(1, 'years').format(), + ); + }); + + it('should handle previous calendar units', () => { + const previousWeek = parseDttmToMoment('previous calendar week'); + const previousMonth = parseDttmToMoment('previous calendar month'); + const previousYear = parseDttmToMoment('previous calendar year'); + expect(previousWeek.format()).toEqual( + moment().utc().subtract(1, 'weeks').startOf('isoWeek').format(), + ); + expect(previousMonth.format()).toEqual( + moment().utc().subtract(1, 'months').startOf('month').format(), + ); + expect(previousYear.format()).toEqual( + moment().utc().subtract(1, 'years').startOf('year').format(), + ); + }); + + it('should handle dynamic "ago" times', () => { + const fiveDaysAgo = parseDttmToMoment('5 days ago'); + expect(fiveDaysAgo.format()).toEqual( + moment().utc().subtract(5, 'days').format(), + ); + }); + + it('should parse valid moment strings', () => { + const specificDate = '2023-01-01'; + const parsedDate = parseDttmToMoment(specificDate); + expect(parsedDate.format()).toEqual(moment(specificDate).format()); + }); +}); + +describe('getTimeOffset', () => { + it('handles custom shifts', () => { + const shifts = ['custom']; + const startDate = '2023-01-01'; + const timeRangeFilter = { comparator: '2023-01-03 : 2023-01-10' }; + + const result = getTimeOffset(timeRangeFilter, shifts, startDate); + expect(result).toEqual(['2 days ago']); + }); + + it('handles inherit shifts', () => { + const shifts = ['inherit']; + const startDate = ''; + const timeRangeFilter = { comparator: '2023-01-03 : 2023-01-10' }; + + const result = getTimeOffset(timeRangeFilter, shifts, startDate); + expect(result).toEqual(['7 days ago']); + }); + + it('handles no custom or inherit shifts', () => { + const shifts = ['1 week ago']; + const startDate = ''; + const timeRangeFilter = { comparator: '2023-01-03 : 2023-01-10' }; + + const result = getTimeOffset(timeRangeFilter, shifts, startDate); + expect(result).toEqual(['1 week ago']); + }); +}); From 7c16e6097d491bc1384ef3a34e8678e340e9a8ca Mon Sep 17 00:00:00 2001 From: lilykuang Date: Fri, 26 Apr 2024 09:40:46 -0700 Subject: [PATCH 17/27] user parseDttmToMoment --- .../src/shared-controls/components/index.tsx | 2 - .../components/controls/TimeOffsetControl.tsx | 53 +++---------------- 2 files changed, 7 insertions(+), 48 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/index.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/index.tsx index a138195618c1a..93bfbcbe6871e 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/index.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/index.tsx @@ -27,5 +27,3 @@ export * from './RadioButtonControl'; export default { RadioButtonControl, }; - -export { RadioButtonControl }; diff --git a/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx b/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx index 806335560a9f7..4c08fb3fffcdd 100644 --- a/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx +++ b/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx @@ -19,7 +19,12 @@ import React, { ReactNode } from 'react'; import { isEqual } from 'lodash'; import moment, { Moment } from 'moment'; -import { BinaryAdhocFilter, SimpleAdhocFilter, css } from '@superset-ui/core'; +import { + parseDttmToMoment, + BinaryAdhocFilter, + SimpleAdhocFilter, + css, +} from '@superset-ui/core'; import { DatePicker } from 'antd'; import { RangePickerProps } from 'antd/lib/date-picker'; import { useSelector } from 'react-redux'; @@ -36,51 +41,7 @@ export interface TimeOffsetControlsProps { onChange: (datetime: string) => void; } const MOMENT_FORMAT = 'YYYY-MM-DD'; -const dttmToMoment = (dttm: string): Moment => { - if (dttm === 'now') { - return moment().utc().startOf('second'); - } - if (dttm === 'today' || dttm === 'No filter') { - return moment().utc().startOf('day'); - } - if (dttm === 'last week') { - return moment().utc().startOf('day').subtract(7, 'day'); - } - if (dttm === 'last month') { - return moment().utc().startOf('day').subtract(1, 'month'); - } - if (dttm === 'last quarter') { - return moment().utc().startOf('day').subtract(1, 'quarter'); - } - if (dttm === 'last year') { - return moment().utc().startOf('day').subtract(1, 'year'); - } - if (dttm === 'previous calendar week') { - return moment().utc().subtract(1, 'weeks').startOf('isoWeek'); - } - if (dttm === 'previous calendar month') { - return moment().utc().subtract(1, 'months').startOf('month'); - } - if (dttm === 'previous calendar year') { - return moment().utc().subtract(1, 'years').startOf('year'); - } - if (dttm.includes('ago')) { - const parts = dttm.split(' '); - const amount = parseInt(parts[0], 10); - const unit = parts[1] as - | 'day' - | 'week' - | 'month' - | 'year' - | 'days' - | 'weeks' - | 'months' - | 'years'; - return moment().utc().subtract(amount, unit); - } - return moment(dttm); -}; const isTimeRangeEqual = ( left: BinaryAdhocFilter[], right: BinaryAdhocFilter[], @@ -101,7 +62,7 @@ export default function TimeOffsetControls({ const startDate = currentTimeRangeFilters[0]?.comparator.split(' : ')[0]; - const formatedDate = dttmToMoment(startDate); + const formatedDate = parseDttmToMoment(startDate); const disabledDate: RangePickerProps['disabledDate'] = current => current && current >= formatedDate; From 131a6e6537e27e0b8bf9c983843fcc7f8e88a681 Mon Sep 17 00:00:00 2001 From: lilykuang Date: Fri, 26 Apr 2024 17:45:07 -0700 Subject: [PATCH 18/27] change parse dttm from moment to date --- .../packages/superset-ui-core/package.json | 1 - .../src/time-comparison/getTimeOffset.ts | 100 +++++++++------- .../src/time-comparison/index.ts | 2 +- .../time-comparison/getTimeOffset.test.ts | 111 ++++++++++-------- .../components/controls/TimeOffsetControl.tsx | 6 +- 5 files changed, 128 insertions(+), 92 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/package.json b/superset-frontend/packages/superset-ui-core/package.json index fb428e01a0e93..1a26e0f49d275 100644 --- a/superset-frontend/packages/superset-ui-core/package.json +++ b/superset-frontend/packages/superset-ui-core/package.json @@ -51,7 +51,6 @@ "jed": "^1.1.1", "lodash": "^4.17.11", "math-expression-evaluator": "^1.3.8", - "moment": "^2.30.1", "pretty-ms": "^7.0.0", "react-error-boundary": "^1.2.5", "react-markdown": "^8.0.3", diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts index 97ced3c7132ee..0838e2ea3e4c6 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts @@ -16,53 +16,77 @@ * specific language governing permissions and limitations * under the License. */ -import moment, { Moment } from 'moment'; import { ensureIsArray } from '../utils'; -export const parseDttmToMoment = (dttm: string): Moment => { - if (dttm === 'now') { - return moment().utc().startOf('second'); - } - if (dttm === 'today' || dttm === 'No filter') { - return moment().utc().startOf('day'); +const DAY_IN_MS = 24 * 60 * 60 * 1000; +export const parseDttmToDate = (dttm: string): Date => { + const now = new Date(); + now.setUTCHours(0, 0, 0, 0); + + if (dttm === 'now' || dttm === 'today' || dttm === 'No filter') { + return now; } if (dttm === 'Last week') { - return moment().utc().startOf('day').subtract(7, 'day'); + now.setUTCDate(now.getUTCDate() - 7); + return now; } if (dttm === 'Last month') { - return moment().utc().startOf('day').subtract(1, 'month'); + now.setUTCMonth(now.getUTCMonth() - 1); + now.setUTCDate(1); + return now; } if (dttm === 'Last quarter') { - return moment().utc().startOf('day').subtract(1, 'quarter'); + now.setUTCMonth(now.getUTCMonth() - 3); + now.setUTCDate(1); + return now; } if (dttm === 'Last year') { - return moment().utc().startOf('day').subtract(1, 'year'); + now.setUTCFullYear(now.getUTCFullYear() - 1); + now.setUTCDate(1); + return now; } if (dttm === 'previous calendar week') { - return moment().utc().subtract(1, 'weeks').startOf('isoWeek'); + now.setUTCDate(now.getUTCDate() - (now.getUTCDay() || 7)); + return now; } if (dttm === 'previous calendar month') { - return moment().utc().subtract(1, 'months').startOf('month'); + now.setUTCMonth(now.getUTCMonth() - 1, 1); + return now; } if (dttm === 'previous calendar year') { - return moment().utc().subtract(1, 'years').startOf('year'); + now.setUTCFullYear(now.getUTCFullYear() - 1, 0, 1); + return now; } if (dttm.includes('ago')) { const parts = dttm.split(' '); const amount = parseInt(parts[0], 10); - const unit = parts[1] as - | 'day' - | 'week' - | 'month' - | 'year' - | 'days' - | 'weeks' - | 'months' - | 'years'; - return moment().utc().subtract(amount, unit); + const unit = parts[1]; + switch (unit) { + case 'day': + case 'days': + now.setUTCDate(now.getUTCDate() - amount); + break; + case 'week': + case 'weeks': + now.setUTCDate(now.getUTCDate() - amount * 7); + break; + case 'month': + case 'months': + now.setUTCMonth(now.getUTCMonth() - amount); + break; + case 'year': + case 'years': + now.setUTCFullYear(now.getUTCFullYear() - amount); + break; + default: + throw new Error('Unsupported time unit'); + } + now.setUTCHours(0, 0, 0, 0); + return now; } - - return moment(dttm); + const parsed = new Date(dttm); + parsed.setUTCHours(0, 0, 0, 0); + return parsed; }; export const getTimeOffset = ( @@ -72,24 +96,20 @@ export const getTimeOffset = ( ): string[] => { const isCustom = shifts?.includes('custom'); const isInherit = shifts?.includes('inherit'); + const customStartDate = isCustom && parseDttmToDate(startDate).getTime(); + const filterStartDate = parseDttmToDate( + timeRangeFilter.comparator.split(' : ')[0], + ).getTime(); + const filterEndDate = parseDttmToDate( + timeRangeFilter.comparator.split(' : ')[1], + ).getTime(); - const customStartDate = isCustom && startDate; const customShift = customStartDate && - moment( - parseDttmToMoment((timeRangeFilter as any).comparator.split(' : ')[0]), - ).diff(moment(customStartDate), 'days'); - + Math.ceil((filterStartDate - customStartDate) / DAY_IN_MS); const inInheritShift = - isInherit && - moment( - parseDttmToMoment((timeRangeFilter as any).comparator.split(' : ')[1]), - ).diff( - moment( - parseDttmToMoment((timeRangeFilter as any).comparator.split(' : ')[0]), - ), - 'days', - ); + isInherit && Math.ceil((filterEndDate - filterStartDate) / DAY_IN_MS); + let newShifts = shifts; if (isCustom) { newShifts = [`${customShift} days ago`]; diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/index.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/index.ts index 208b6ecc4450b..7856d344ff54d 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/index.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/index.ts @@ -21,5 +21,5 @@ export * from './types'; export { default as getComparisonInfo } from './getComparisonInfo'; export { default as getComparisonFilters } from './getComparisonFilters'; -export { parseDttmToMoment, getTimeOffset } from './getTimeOffset'; +export { parseDttmToDate, getTimeOffset } from './getTimeOffset'; export { SEPARATOR, fetchTimeRange } from './fetchTimeRange'; diff --git a/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts b/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts index c66816366654c..3d681a7d933b9 100644 --- a/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts @@ -16,70 +16,87 @@ * specific language governing permissions and limitations * under the License. */ -import { getTimeOffset, parseDttmToMoment } from '@superset-ui/core'; -import moment from 'moment'; +import { getTimeOffset, parseDttmToDate } from '@superset-ui/core'; -describe('parseDttmToMoment', () => { +describe('parseDttmToDate', () => { it('should handle "now"', () => { - const now = parseDttmToMoment('now'); - expect(now.utc().format()).toEqual( - moment().utc().startOf('second').format(), - ); + const now = parseDttmToDate('now'); + const expected = new Date(); + expected.setUTCHours(0, 0, 0, 0); + expect(expected).toEqual(now); }); it('should handle "today" and "No filter"', () => { - const today = parseDttmToMoment('today'); - const noFilter = parseDttmToMoment('No filter'); - const expected = moment().utc().startOf('day').format(); - expect(today.format()).toEqual(expected); - expect(noFilter.format()).toEqual(expected); + const today = parseDttmToDate('today'); + const noFilter = parseDttmToDate('No filter'); + const expected = new Date(); + expected.setUTCHours(0, 0, 0, 0); + expect(today).toEqual(expected); + expect(noFilter).toEqual(expected); }); it('should handle relative time strings', () => { - const lastWeek = parseDttmToMoment('Last week'); - const lastMonth = parseDttmToMoment('Last month'); - const lastQuarter = parseDttmToMoment('Last quarter'); - const lastYear = parseDttmToMoment('Last year'); - expect(lastWeek.format()).toEqual( - moment().utc().startOf('day').subtract(7, 'days').format(), - ); - expect(lastMonth.format()).toEqual( - moment().utc().startOf('day').subtract(1, 'months').format(), - ); - expect(lastQuarter.format()).toEqual( - moment().utc().startOf('day').subtract(1, 'quarters').format(), - ); - expect(lastYear.format()).toEqual( - moment().utc().startOf('day').subtract(1, 'years').format(), - ); + const lastWeek = parseDttmToDate('Last week'); + const lastMonth = parseDttmToDate('Last month'); + const lastQuarter = parseDttmToDate('Last quarter'); + const lastYear = parseDttmToDate('Last year'); + let now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCDate(now.getUTCDate() - 7); + expect(lastWeek).toEqual(now); + + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCMonth(now.getUTCMonth() - 1); + now.setUTCDate(1); + expect(lastMonth).toEqual(now); + + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCMonth(now.getUTCMonth() - 3); + now.setUTCDate(1); + expect(lastQuarter).toEqual(now); + + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCFullYear(now.getUTCFullYear() - 1); + now.setUTCDate(1); + expect(lastYear).toEqual(now); }); it('should handle previous calendar units', () => { - const previousWeek = parseDttmToMoment('previous calendar week'); - const previousMonth = parseDttmToMoment('previous calendar month'); - const previousYear = parseDttmToMoment('previous calendar year'); - expect(previousWeek.format()).toEqual( - moment().utc().subtract(1, 'weeks').startOf('isoWeek').format(), - ); - expect(previousMonth.format()).toEqual( - moment().utc().subtract(1, 'months').startOf('month').format(), - ); - expect(previousYear.format()).toEqual( - moment().utc().subtract(1, 'years').startOf('year').format(), - ); + const previousWeek = parseDttmToDate('previous calendar week'); + const previousMonth = parseDttmToDate('previous calendar month'); + const previousYear = parseDttmToDate('previous calendar year'); + let now = new Date(); + now.setUTCDate(now.getUTCDate() - (now.getUTCDay() || 7)); + now.setUTCHours(0, 0, 0, 0); + expect(previousWeek).toEqual(now); + + now = new Date(); + now.setUTCMonth(now.getUTCMonth() - 1, 1); + now.setUTCHours(0, 0, 0, 0); + expect(previousMonth).toEqual(now); + + now = new Date(); + now.setUTCFullYear(now.getUTCFullYear() - 1, 0, 1); + now.setUTCHours(0, 0, 0, 0); + expect(previousYear).toEqual(now); }); it('should handle dynamic "ago" times', () => { - const fiveDaysAgo = parseDttmToMoment('5 days ago'); - expect(fiveDaysAgo.format()).toEqual( - moment().utc().subtract(5, 'days').format(), - ); + const fiveDaysAgo = parseDttmToDate('5 days ago'); + const now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCDate(now.getUTCDate() - 5); + expect(fiveDaysAgo).toEqual(now); }); it('should parse valid moment strings', () => { - const specificDate = '2023-01-01'; - const parsedDate = parseDttmToMoment(specificDate); - expect(parsedDate.format()).toEqual(moment(specificDate).format()); + const specificDate = new Date('2023-01-01'); + specificDate.setUTCHours(0, 0, 0, 0); + const parsedDate = parseDttmToDate('2023-01-01'); + expect(parsedDate).toEqual(specificDate); }); }); diff --git a/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx b/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx index 4c08fb3fffcdd..1ce99e550f5b3 100644 --- a/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx +++ b/superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx @@ -20,7 +20,7 @@ import React, { ReactNode } from 'react'; import { isEqual } from 'lodash'; import moment, { Moment } from 'moment'; import { - parseDttmToMoment, + parseDttmToDate, BinaryAdhocFilter, SimpleAdhocFilter, css, @@ -62,9 +62,9 @@ export default function TimeOffsetControls({ const startDate = currentTimeRangeFilters[0]?.comparator.split(' : ')[0]; - const formatedDate = parseDttmToMoment(startDate); + const formatedDate = moment(parseDttmToDate(startDate)); const disabledDate: RangePickerProps['disabledDate'] = current => - current && current >= formatedDate; + current && current > formatedDate; return (
From 669a5bffc3cc6f47b28434cdfd3eb52eed6aa72b Mon Sep 17 00:00:00 2001 From: lilykuang Date: Fri, 26 Apr 2024 18:02:02 -0700 Subject: [PATCH 19/27] update tests --- .../src/time-comparison/getTimeOffset.ts | 6 +-- .../time-comparison/getTimeOffset.test.ts | 42 ++++++++++++++++--- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts index 0838e2ea3e4c6..1df28318f0564 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts @@ -46,7 +46,7 @@ export const parseDttmToDate = (dttm: string): Date => { return now; } if (dttm === 'previous calendar week') { - now.setUTCDate(now.getUTCDate() - (now.getUTCDay() || 7)); + now.setUTCDate(now.getUTCDate() - now.getUTCDay()); return now; } if (dttm === 'previous calendar month') { @@ -61,6 +61,7 @@ export const parseDttmToDate = (dttm: string): Date => { const parts = dttm.split(' '); const amount = parseInt(parts[0], 10); const unit = parts[1]; + switch (unit) { case 'day': case 'days': @@ -79,9 +80,8 @@ export const parseDttmToDate = (dttm: string): Date => { now.setUTCFullYear(now.getUTCFullYear() - amount); break; default: - throw new Error('Unsupported time unit'); + break; } - now.setUTCHours(0, 0, 0, 0); return now; } const parsed = new Date(dttm); diff --git a/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts b/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts index 3d681a7d933b9..c2f6c9511b504 100644 --- a/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts @@ -65,31 +65,63 @@ describe('parseDttmToDate', () => { }); it('should handle previous calendar units', () => { - const previousWeek = parseDttmToDate('previous calendar week'); - const previousMonth = parseDttmToDate('previous calendar month'); - const previousYear = parseDttmToDate('previous calendar year'); let now = new Date(); - now.setUTCDate(now.getUTCDate() - (now.getUTCDay() || 7)); now.setUTCHours(0, 0, 0, 0); + now.setUTCDate(now.getUTCDate() - now.getUTCDay()); + const previousWeek = parseDttmToDate('previous calendar week'); expect(previousWeek).toEqual(now); now = new Date(); now.setUTCMonth(now.getUTCMonth() - 1, 1); now.setUTCHours(0, 0, 0, 0); + const previousMonth = parseDttmToDate('previous calendar month'); expect(previousMonth).toEqual(now); now = new Date(); now.setUTCFullYear(now.getUTCFullYear() - 1, 0, 1); now.setUTCHours(0, 0, 0, 0); + const previousYear = parseDttmToDate('previous calendar year'); expect(previousYear).toEqual(now); }); it('should handle dynamic "ago" times', () => { const fiveDaysAgo = parseDttmToDate('5 days ago'); - const now = new Date(); + const fiveDayAgo = parseDttmToDate('5 day ago'); + let now = new Date(); now.setUTCHours(0, 0, 0, 0); now.setUTCDate(now.getUTCDate() - 5); expect(fiveDaysAgo).toEqual(now); + expect(fiveDayAgo).toEqual(now); + + const weeksAgo = parseDttmToDate('7 weeks ago'); + const weekAgo = parseDttmToDate('7 week ago'); + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCDate(now.getUTCDate() - 7 * 7); + expect(weeksAgo).toEqual(now); + expect(weekAgo).toEqual(now); + + const fiveMonthsAgo = parseDttmToDate('5 months ago'); + const fiveMonthAgo = parseDttmToDate('5 month ago'); + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCMonth(now.getUTCMonth() - 5); + expect(fiveMonthsAgo).toEqual(now); + expect(fiveMonthAgo).toEqual(now); + + const fiveYearsAgo = parseDttmToDate('5 years ago'); + const fiveYearAgo = parseDttmToDate('5 year ago'); + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCFullYear(now.getUTCFullYear() - 5); + expect(fiveYearsAgo).toEqual(now); + expect(fiveYearAgo).toEqual(now); + + // default case + const fiveHoursAgo = parseDttmToDate('5 hours ago'); + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + expect(fiveHoursAgo).toEqual(now); }); it('should parse valid moment strings', () => { From f5f534a94fb2c71e64c7e1cba3d69fee48289808 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Tue, 30 Apr 2024 10:48:13 +0200 Subject: [PATCH 20/27] BigNumber with Time Comparison: - Update lock file so we remove moment - Stop displaying Calculation Type since this viz only uses Actual Values (postporocessing: []) - Handle nullish dttm --- superset-frontend/package-lock.json | 2 -- .../src/sections/timeComparison.tsx | 7 ++++++- .../superset-ui-core/src/time-comparison/getTimeOffset.ts | 2 +- .../BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index 039d7a6c0bb2f..deaa3748f5983 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -67531,7 +67531,6 @@ "jed": "^1.1.1", "lodash": "^4.17.11", "math-expression-evaluator": "^1.3.8", - "moment": "^2.30.1", "pretty-ms": "^7.0.0", "react-error-boundary": "^1.2.5", "react-markdown": "^8.0.3", @@ -86239,7 +86238,6 @@ "jest-mock-console": "^1.0.0", "lodash": "^4.17.11", "math-expression-evaluator": "^1.3.8", - "moment": "^2.30.1", "pretty-ms": "^7.0.0", "react-error-boundary": "^1.2.5", "react-markdown": "^8.0.3", diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx index 6fe8a8152aa8e..087752f5ecd9c 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx @@ -22,7 +22,11 @@ import { ControlPanelSectionConfig } from '../types'; export const timeComparisonControls: ( multi?: boolean, -) => ControlPanelSectionConfig = (multi = true) => ({ + show_calculation_type?: boolean, +) => ControlPanelSectionConfig = ( + multi = true, + show_calculation_type = true, +) => ({ label: t('Time Comparison'), tabOverride: 'data', description: t( @@ -92,6 +96,7 @@ export const timeComparisonControls: ( 'difference between the main time series and each time shift; ' + 'as the percentage change; or as the ratio between series and time shifts.', ), + visibility: () => Boolean(show_calculation_type), }, }, ], diff --git a/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts b/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts index 1df28318f0564..aea7c6bd4a2d5 100644 --- a/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts +++ b/superset-frontend/packages/superset-ui-core/src/time-comparison/getTimeOffset.ts @@ -57,7 +57,7 @@ export const parseDttmToDate = (dttm: string): Date => { now.setUTCFullYear(now.getUTCFullYear() - 1, 0, 1); return now; } - if (dttm.includes('ago')) { + if (dttm?.includes('ago')) { const parts = dttm.split(' '); const amount = parseInt(parts[0], 10); const unit = parts[1]; diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts index 3507f6566dbbb..12db15a1eb978 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts @@ -108,7 +108,7 @@ const config: ControlPanelConfig = { ], ], }, - sections.timeComparisonControls(false), + sections.timeComparisonControls(false, false), ], controlOverrides: { y_axis_format: { From 9e59a97b65313d54315bc50dce74d4ec4ad250b0 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Tue, 7 May 2024 17:23:20 +0200 Subject: [PATCH 21/27] BigNumber with Time Comparison: - Add new prop so we can show full list of choices or just a reduced amount based on the boolean value - Update the copy so it better reflects and help the user --- .../src/sections/timeComparison.tsx | 69 ++++++++++++------- .../BigNumberPeriodOverPeriod/controlPanel.ts | 2 +- 2 files changed, 44 insertions(+), 27 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx index 087752f5ecd9c..d8a862a671a01 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx @@ -20,21 +20,46 @@ import { t, ComparisonType } from '@superset-ui/core'; import { ControlPanelSectionConfig } from '../types'; +const fullChoices = [ + ['1 day ago', t('1 day ago')], + ['1 week ago', t('1 week ago')], + ['28 days ago', t('28 days ago')], + ['30 days ago', t('30 days ago')], + ['1 month ago', t('1 month ago')], + ['52 weeks ago', t('52 weeks ago')], + ['1 year ago', t('1 year ago')], + ['104 weeks ago', t('104 weeks ago')], + ['2 years ago', t('2 years ago')], + ['156 weeks ago', t('156 weeks ago')], + ['3 years ago', t('3 years ago')], + ['custom', t('Custom date')], + ['inherit', t('Inherit range from time filter')], +]; + +const reducedKeys = new Set([ + '1 day ago', + '1 week ago', + '1 month ago', + '1 year ago', + 'custom', + 'inherit', +]); + +// Filter fullChoices to get only the entries whose keys are in reducedKeys +const reducedChoices = fullChoices.filter(choice => reducedKeys.has(choice[0])); + export const timeComparisonControls: ( multi?: boolean, - show_calculation_type?: boolean, + showCalculationType?: boolean, + showFullChoices?: boolean, ) => ControlPanelSectionConfig = ( multi = true, - show_calculation_type = true, + showCalculationType = true, + showFullChoices = true, ) => ({ label: t('Time Comparison'), tabOverride: 'data', - description: t( - 'This section contains options ' + - 'that allow for time comparison ' + - 'of query results using some portions of the ' + - 'existing advanced analytics section', - ), + description: t('Compare results with other time periods.'), controlSetRows: [ [ { @@ -43,26 +68,18 @@ export const timeComparisonControls: ( type: 'SelectControl', multi, freeForm: true, + placeholder: t('Select or type a custom value...'), label: t('Time shift'), - choices: [ - ['1 day ago', t('1 day ago')], - ['1 week ago', t('1 week ago')], - ['28 days ago', t('28 days ago')], - ['30 days ago', t('30 days ago')], - ['52 weeks ago', t('52 weeks ago')], - ['1 year ago', t('1 year ago')], - ['104 weeks ago', t('104 weeks ago')], - ['2 years ago', t('2 years ago')], - ['156 weeks ago', t('156 weeks ago')], - ['3 years ago', t('3 years ago')], - ['custom', t('Custom')], - ['inherit', t('Inherit')], - ], + choices: showFullChoices ? fullChoices : reducedChoices, description: t( - 'Overlay one or more timeseries from a ' + - 'relative time period. Expects relative time deltas ' + + 'Overlay results from a relative time period. ' + + 'Expects relative time deltas ' + 'in natural language (example: 24 hours, 7 days, ' + - '52 weeks, 365 days). Free text is supported.', + '52 weeks, 365 days). Free text is supported. ' + + 'Use "Inherit range from time filters" ' + + 'to shift the comparison time range ' + + 'by the same length as your time range ' + + 'and use "Custom" to set a custom comparison range.', ), }, }, @@ -96,7 +113,7 @@ export const timeComparisonControls: ( 'difference between the main time series and each time shift; ' + 'as the percentage change; or as the ratio between series and time shifts.', ), - visibility: () => Boolean(show_calculation_type), + visibility: () => Boolean(showCalculationType), }, }, ], diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts index 12db15a1eb978..1e75d1884ffde 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts @@ -108,7 +108,7 @@ const config: ControlPanelConfig = { ], ], }, - sections.timeComparisonControls(false, false), + sections.timeComparisonControls(false, false, false), ], controlOverrides: { y_axis_format: { From 9d66fc3abcd172bbe06fd7cf0d2adbab7aeb10d2 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Mon, 13 May 2024 13:00:58 +0200 Subject: [PATCH 22/27] Time Comparison: - Add migration to port from old time comparison params to new approach - Add tests for migration file --- ..._update_charts_with_old_time_comparison.py | 212 ++++++++++++ ...e_charts_with_old_time_comparison__test.py | 315 ++++++++++++++++++ 2 files changed, 527 insertions(+) create mode 100644 superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py create mode 100644 tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py diff --git a/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py b/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py new file mode 100644 index 0000000000000..00a200fafc740 --- /dev/null +++ b/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py @@ -0,0 +1,212 @@ +# 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. +"""Update charts with old time comparison controls + +Revision ID: f84fde59123a +Revises: 4081be5b6b74 +Create Date: 2024-05-10 18:02:38.891060 + +""" + +import json +import logging +from copy import deepcopy +from datetime import datetime, timedelta +from hashlib import md5 +from typing import Any + +from alembic import op +from sqlalchemy import Column, Integer, or_, String, Text +from sqlalchemy.ext.declarative import declarative_base + +from superset import db +from superset.migrations.shared.utils import paginated_update +from superset.utils.date_parser import get_since_until + +# revision identifiers, used by Alembic. +revision = "f84fde59123a" +down_revision = "4081be5b6b74" + +logger = logging.getLogger(__name__) +Base = declarative_base() + + +class Slice(Base): + __tablename__ = "slices" + + id = Column(Integer, primary_key=True) + params = Column(Text) + viz_type = Column(String(250)) + + +time_map = { + "r": "inherit", + "y": "1 year ago", + "m": "1 month ago", + "w": "1 week ago", + "c": "custom", +} + + +def upgrade_comparison_params(slice_params: dict[str, Any]) -> dict[str, Any]: + params = deepcopy(slice_params) + + if "enable_time_comparison" in params: + # Remove enable_time_comparison + del params["enable_time_comparison"] + + # Update time_comparison to time_compare + if "time_comparison" in params: + time_comp = params.pop("time_comparison") + params["time_compare"] = time_map.get( + time_comp, "inherit" + ) # Default to 'inherit' if not found + + # Add comparison_type + params["comparison_type"] = "values" + + # Adjust adhoc_custom + if "adhoc_custom" in params and params["adhoc_custom"]: + adhoc = params["adhoc_custom"][0] # As there's always only one element + if adhoc["comparator"] != "No filter": + # Set start_date_offset in params, not in adhoc + start_date_offset, _ = get_since_until(adhoc["comparator"]) + params["start_date_offset"] = start_date_offset.strftime("%Y-%m-%d") + # delete adhoc_custom + del params["adhoc_custom"] + + return params + + +def upgrade(): + bind = op.get_bind() + session = db.Session(bind=bind) + + for slc in paginated_update( + session.query(Slice).filter( + or_(Slice.viz_type == "pop_kpi", Slice.viz_type == "table") + ) + ): + try: + params = json.loads(slc.params) + updated_slice_params = upgrade_comparison_params(params) + slc.params = json.dumps(updated_slice_params) + except Exception as ex: + session.rollback() + logger.exception( + f"An error occurred: Upgrading params for slice {slc.id} failed." + f"You need to fix it before upgrading your DB." + ) + raise Exception(f"An error occurred while upgrading slice: {ex}") + + session.commit() + session.close() + + +def downgrade_comparison_params(slice_params: dict[str, Any]) -> dict[str, Any]: + params = deepcopy(slice_params) + + reverse_time_map = { + v: k for k, v in time_map.items() + } # Reverse the map from the upgrade function + + # Add enable_time_comparison + params["enable_time_comparison"] = True + + # Revert time_compare to time_comparison + if "time_compare" in params: + time_comp = params.pop("time_compare") + params["time_comparison"] = reverse_time_map.get( + time_comp, "r" + ) # Default to 'r' if not found + + # Remove comparison_type + if "comparison_type" in params: + del params["comparison_type"] + + # Default adhoc_custom + adhoc_custom = [ + { + "clause": "WHERE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "No filter", + "expressionType": "SIMPLE", + } + ] + + # Handle start_date_offset and adjust adhoc_custom if necessary + if "start_date_offset" in params: + start_date_offset = datetime.strptime( + params.pop("start_date_offset"), "%Y-%m-%d" + ) + adhoc_filters = params.get("adhoc_filters", []) + temporal_range_filter = next( + (f for f in adhoc_filters if f["operator"] == "TEMPORAL_RANGE"), None + ) + + if temporal_range_filter: + since, until = get_since_until(temporal_range_filter["comparator"]) + delta_days = (until - since).days + new_until_date = start_date_offset + timedelta(days=delta_days - 1) + comparator_str = f"{start_date_offset.strftime('%Y-%m-%d')} : {new_until_date.strftime('%Y-%m-%d')}" + + # Generate filterOptionName + random_string = md5(comparator_str.encode("utf-8")).hexdigest() + filter_option_name = f"filter_{random_string}" + + adhoc_custom[0] = { + "expressionType": "SIMPLE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": comparator_str, + "clause": "WHERE", + "sqlExpression": None, + "isExtra": False, + "isNew": False, + "datasourceWarning": False, + "filterOptionName": filter_option_name, + } + + params["adhoc_custom"] = adhoc_custom + + return params + + +def downgrade(): + bind = op.get_bind() + session = db.Session(bind=bind) + + for slc in paginated_update( + session.query(Slice).filter( + Slice.viz_type == "pop_kpi" or Slice.viz_type == "table" + ) + ): + try: + params = json.loads(slc.params) + updated_slice_params = downgrade_comparison_params(params) + slc.params = json.dumps(updated_slice_params) + except Exception as ex: + session.rollback() + logger.exception( + f"An error occurred: Downgrading params for slice {slc.id} failed." + f"You need to fix it before downgrading your DB." + ) + raise Exception(f"An error occurred while downgrading slice: {ex}") + + session.commit() + session.close() diff --git a/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py new file mode 100644 index 0000000000000..5f7fe505c03b1 --- /dev/null +++ b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py @@ -0,0 +1,315 @@ +# 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. +from copy import deepcopy +from importlib import import_module +from typing import Any + +migrate_time_comparison_to_new_format = import_module( + "superset.migrations.versions." + "2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison", +) +downgrade_comparison_params = ( + migrate_time_comparison_to_new_format.downgrade_comparison_params +) +upgrade_comparison_params = ( + migrate_time_comparison_to_new_format.upgrade_comparison_params +) + +params_v1_with_custom: dict[str, Any] = { + "datasource": "2__table", + "viz_type": "pop_kpi", + "metric": { + "expressionType": "SIMPLE", + "column": { + "advanced_data_type": None, + "certification_details": None, + "certified_by": None, + "column_name": "num_boys", + "description": None, + "expression": None, + "filterable": True, + "groupby": True, + "id": 334, + "is_certified": False, + "is_dttm": False, + "python_date_format": None, + "type": "BIGINT", + "type_generic": 0, + "verbose_name": None, + "warning_markdown": None, + }, + "aggregate": "SUM", + "sqlExpression": None, + "datasourceWarning": False, + "hasCustomLabel": False, + "label": "SUM(num_boys)", + "optionName": "metric_o6rj1h6jty_3t6mrruogfv", + }, + "adhoc_filters": [ + { + "expressionType": "SIMPLE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "1984 : 1986", + "clause": "WHERE", + "sqlExpression": None, + "isExtra": False, + "isNew": False, + "datasourceWarning": False, + "filterOptionName": "filter_p50i4xw50d_8x8e4ypwjs8", + } + ], + "row_limit": 10000, + "y_axis_format": "SMART_NUMBER", + "percentDifferenceFormat": "SMART_NUMBER", + "header_font_size": 0.2, + "subheader_font_size": 0.125, + "comparison_color_scheme": "Green", + "extra_form_data": {}, + "dashboards": [], + "time_comparison": "c", + "enable_time_comparison": True, + "adhoc_custom": [ + { + "expressionType": "SIMPLE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "1981-01-01 : 1983-01-01", + "clause": "WHERE", + "sqlExpression": None, + "isExtra": False, + "isNew": False, + "datasourceWarning": False, + } + ], +} +params_v1_other_than_custom: dict[str, Any] = { + "datasource": "2__table", + "viz_type": "pop_kpi", + "metric": { + "expressionType": "SIMPLE", + "column": { + "advanced_data_type": None, + "certification_details": None, + "certified_by": None, + "column_name": "num_boys", + "description": None, + "expression": None, + "filterable": True, + "groupby": True, + "id": 334, + "is_certified": False, + "is_dttm": False, + "python_date_format": None, + "type": "BIGINT", + "type_generic": 0, + "verbose_name": None, + "warning_markdown": None, + }, + "aggregate": "SUM", + "sqlExpression": None, + "datasourceWarning": False, + "hasCustomLabel": False, + "label": "SUM(num_boys)", + "optionName": "metric_96s7b8iypsr_4wrlgm0i7il", + }, + "adhoc_filters": [ + { + "expressionType": "SIMPLE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "1984 : 2000", + "clause": "WHERE", + "sqlExpression": None, + "isExtra": False, + "isNew": False, + "datasourceWarning": False, + "filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6", + } + ], + "row_limit": 10000, + "y_axis_format": "SMART_NUMBER", + "percentDifferenceFormat": "SMART_NUMBER", + "header_font_size": 0.2, + "subheader_font_size": 0.125, + "comparison_color_scheme": "Green", + "extra_form_data": {}, + "dashboards": [], + "time_comparison": "r", + "enable_time_comparison": True, + "adhoc_custom": [ + { + "clause": "WHERE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "No filter", + "expressionType": "SIMPLE", + } + ], +} +params_v2_with_custom: dict[str, Any] = { + "datasource": "2__table", + "viz_type": "pop_kpi", + "metric": { + "expressionType": "SIMPLE", + "column": { + "advanced_data_type": None, + "certification_details": None, + "certified_by": None, + "column_name": "num_boys", + "description": None, + "expression": None, + "filterable": True, + "groupby": True, + "id": 334, + "is_certified": False, + "is_dttm": False, + "python_date_format": None, + "type": "BIGINT", + "type_generic": 0, + "verbose_name": None, + "warning_markdown": None, + }, + "aggregate": "SUM", + "sqlExpression": None, + "datasourceWarning": False, + "hasCustomLabel": False, + "label": "SUM(num_boys)", + "optionName": "metric_o6rj1h6jty_3t6mrruogfv", + }, + "adhoc_filters": [ + { + "expressionType": "SIMPLE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "1984 : 1986", + "clause": "WHERE", + "sqlExpression": None, + "isExtra": False, + "isNew": False, + "datasourceWarning": False, + "filterOptionName": "filter_p50i4xw50d_8x8e4ypwjs8", + } + ], + "row_limit": 10000, + "y_axis_format": "SMART_NUMBER", + "percentDifferenceFormat": "SMART_NUMBER", + "header_font_size": 0.2, + "subheader_font_size": 0.125, + "comparison_color_scheme": "Green", + "extra_form_data": {}, + "dashboards": [], + "time_compare": "custom", + "comparison_type": "values", + "start_date_offset": "1981-01-01", +} +params_v2_other_than_custom: dict[str, Any] = { + "datasource": "2__table", + "viz_type": "pop_kpi", + "metric": { + "expressionType": "SIMPLE", + "column": { + "advanced_data_type": None, + "certification_details": None, + "certified_by": None, + "column_name": "num_boys", + "description": None, + "expression": None, + "filterable": True, + "groupby": True, + "id": 334, + "is_certified": False, + "is_dttm": False, + "python_date_format": None, + "type": "BIGINT", + "type_generic": 0, + "verbose_name": None, + "warning_markdown": None, + }, + "aggregate": "SUM", + "sqlExpression": None, + "datasourceWarning": False, + "hasCustomLabel": False, + "label": "SUM(num_boys)", + "optionName": "metric_96s7b8iypsr_4wrlgm0i7il", + }, + "adhoc_filters": [ + { + "expressionType": "SIMPLE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "1984 : 2000", + "clause": "WHERE", + "sqlExpression": None, + "isExtra": False, + "isNew": False, + "datasourceWarning": False, + "filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6", + } + ], + "row_limit": 10000, + "y_axis_format": "SMART_NUMBER", + "percentDifferenceFormat": "SMART_NUMBER", + "header_font_size": 0.2, + "subheader_font_size": 0.125, + "comparison_color_scheme": "Green", + "extra_form_data": {}, + "dashboards": [], + "time_compare": "inherit", + "comparison_type": "values", +} + + +def test_upgrade_chart_params_with_custom(): + """ + ensure that the new time comparison params are added + """ + original_params = deepcopy(params_v1_with_custom) + upgraded_params = upgrade_comparison_params(original_params) + assert upgraded_params == params_v2_with_custom + + +def test_downgrade_chart_params_with_custom(): + """ + ensure that the params downgrade operation produces an almost identical dict + as the original value + """ + original_params = deepcopy(params_v2_with_custom) + downgraded_params = downgrade_comparison_params(original_params) + # Ignore any property called filterOptionName simce that uses a random hash + for adhoc_custom in downgraded_params["adhoc_custom"]: + adhoc_custom.pop("filterOptionName", None) + assert downgraded_params == params_v1_with_custom + + +def test_upgrade_chart_params_other_than_custom(): + """ + ensure that the new time comparison params are added + """ + original_params = deepcopy(params_v1_other_than_custom) + upgraded_params = upgrade_comparison_params(original_params) + assert upgraded_params == params_v2_other_than_custom + + +def test_downgrade_chart_params_other_than_custom(): + """ + ensure that the params downgrade operation produces an almost identical dict + as the original value + """ + original_params = deepcopy(params_v2_other_than_custom) + downgraded_params = downgrade_comparison_params(original_params) + assert downgraded_params == params_v1_other_than_custom From 10c716f83a8de47afc340415f8d5fedbbe69f2b1 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Tue, 14 May 2024 18:35:54 +0200 Subject: [PATCH 23/27] Time Comparison: - Fix delta calculation for shifts - Add tests for new changes --- superset/utils/date_parser.py | 7 ++++--- tests/unit_tests/utils/date_parser_tests.py | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/superset/utils/date_parser.py b/superset/utils/date_parser.py index 9a7c135f5c159..bffe50c6242c3 100644 --- a/superset/utils/date_parser.py +++ b/superset/utils/date_parser.py @@ -263,9 +263,10 @@ def get_since_until( # pylint: disable=too-many-arguments,too-many-locals,too-m ) if time_shift: - time_delta = parse_past_timedelta(time_shift) - _since = _since if _since is None else (_since - time_delta) - _until = _until if _until is None else (_until - time_delta) + time_delta_since = parse_past_timedelta(time_shift, _since) + time_delta_until = parse_past_timedelta(time_shift, _until) + _since = _since if _since is None else (_since - time_delta_since) + _until = _until if _until is None else (_until - time_delta_until) if instant_time_comparison_range: # This is only set using the new time comparison controls diff --git a/tests/unit_tests/utils/date_parser_tests.py b/tests/unit_tests/utils/date_parser_tests.py index b031e54a664c8..e007c17e829c5 100644 --- a/tests/unit_tests/utils/date_parser_tests.py +++ b/tests/unit_tests/utils/date_parser_tests.py @@ -189,6 +189,27 @@ def test_get_since_until() -> None: expected = datetime(2000, 1, 1), datetime(2018, 1, 1) assert result == expected + result = get_since_until( + time_range="2000-01-01T00:00:00 : 2018-01-01T00:00:00", + time_shift="1 year ago", + ) + expected = datetime(1999, 1, 1), datetime(2017, 1, 1) + assert result == expected + + result = get_since_until( + time_range="2000-01-01T00:00:00 : 2018-01-01T00:00:00", + time_shift="1 month ago", + ) + expected = datetime(1999, 12, 1), datetime(2017, 12, 1) + assert result == expected + + result = get_since_until( + time_range="2000-01-01T00:00:00 : 2018-01-01T00:00:00", + time_shift="1 week ago", + ) + expected = datetime(1999, 12, 25), datetime(2017, 12, 25) + assert result == expected + with pytest.raises(ValueError): get_since_until(time_range="tomorrow : yesterday") From 336fc9aa39af6e59ea7dbe0fc333fe6480e3f25a Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Wed, 15 May 2024 16:33:37 +0200 Subject: [PATCH 24/27] Time Comparison: - Fix lint error - Fix tests --- superset/common/query_context_processor.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index ba1a43bc73075..29f52816dd98c 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -553,18 +553,17 @@ def join_offset_dfs( join_column_producer or time_grain in AGGREGATED_JOIN_GRAINS ) - if use_aggregated_join_column: - if not time_grain: - raise QueryObjectValidationError( - _("Time Grain must be specified when using Time Shift.") - ) + if use_aggregated_join_column and not time_grain: + raise QueryObjectValidationError( + _("Time Grain must be specified when using Time Shift.") + ) # iterate on offset_dfs, left join each with df for offset, offset_df in offset_dfs.items(): # defines a column name for the offset join column column_name = OFFSET_JOIN_COLUMN_SUFFIX + offset - if use_aggregated_join_column: + if time_grain: # add offset join column to df self.add_offset_join_column( df, column_name, time_grain, offset, join_column_producer @@ -593,7 +592,7 @@ def join_offset_dfs( rsuffix=R_SUFFIX, ) - if use_aggregated_join_column: + if time_grain: # move the temporal column to the first column in df col = df.pop(join_keys[0]) df.insert(0, col.name, col) From 661adcdb5337fa45689b5c9015981fbd8d14cb8f Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Wed, 15 May 2024 17:48:39 +0200 Subject: [PATCH 25/27] Time Comparison: - Pass a config object instead of raw bollean flags to the time comparison control so the intention is clear when using it - Split getTimeOffset tests so parseDttmToDate has its own file - use inlined test instead of blocks with describe and it --- .../src/sections/timeComparison.tsx | 17 +- .../time-comparison/fetchTimeRange.test.ts | 14 +- .../time-comparison/getTimeOffset.test.ts | 156 +++--------------- .../time-comparison/parseDttmToDate.test.ts | 131 +++++++++++++++ .../BigNumberPeriodOverPeriod/controlPanel.ts | 6 +- 5 files changed, 174 insertions(+), 150 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-core/test/time-comparison/parseDttmToDate.test.ts diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx index d8a862a671a01..86c7eb9f9db93 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/timeComparison.tsx @@ -48,15 +48,20 @@ const reducedKeys = new Set([ // Filter fullChoices to get only the entries whose keys are in reducedKeys const reducedChoices = fullChoices.filter(choice => reducedKeys.has(choice[0])); -export const timeComparisonControls: ( - multi?: boolean, - showCalculationType?: boolean, - showFullChoices?: boolean, -) => ControlPanelSectionConfig = ( +type TimeComparisonControlsType = { + multi?: boolean; + showCalculationType?: boolean; + showFullChoices?: boolean; +}; +export const timeComparisonControls: ({ + multi, + showCalculationType, + showFullChoices, +}: TimeComparisonControlsType) => ControlPanelSectionConfig = ({ multi = true, showCalculationType = true, showFullChoices = true, -) => ({ +}) => ({ label: t('Time Comparison'), tabOverride: 'data', description: t('Compare results with other time periods.'), diff --git a/superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts b/superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts index 84644564f66c0..964761f6b954b 100644 --- a/superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts @@ -27,7 +27,7 @@ import { afterEach(fetchMock.restore); -it('generates proper time range string', () => { +test('generates proper time range string', () => { expect( buildTimeRangeString('2010-07-30T00:00:00', '2020-07-30T00:00:00'), ).toBe('2010-07-30T00:00:00 : 2020-07-30T00:00:00'); @@ -37,7 +37,7 @@ it('generates proper time range string', () => { expect(buildTimeRangeString('', '')).toBe(' : '); }); -it('generates a readable time range', () => { +test('generates a readable time range', () => { expect(formatTimeRange('Last 7 days')).toBe('Last 7 days'); expect(formatTimeRange('No filter')).toBe('No filter'); expect(formatTimeRange('Yesterday : Tomorrow')).toBe( @@ -54,7 +54,7 @@ it('generates a readable time range', () => { ); }); -it('returns a formatted time range from response', async () => { +test('returns a formatted time range from response', async () => { fetchMock.get("glob:*/api/v1/time_range/?q='Last+day'", { result: [ { @@ -71,7 +71,7 @@ it('returns a formatted time range from response', async () => { }); }); -it('returns a formatted time range from empty response', async () => { +test('returns a formatted time range from empty response', async () => { fetchMock.get("glob:*/api/v1/time_range/?q='Last+day'", { result: [], }); @@ -82,7 +82,7 @@ it('returns a formatted time range from empty response', async () => { }); }); -it('returns a formatted error message from response', async () => { +test('returns a formatted error message from response', async () => { fetchMock.getOnce("glob:*/api/v1/time_range/?q='Last+day'", { throws: new Response(JSON.stringify({ message: 'Network error' })), }); @@ -118,7 +118,7 @@ it('returns a formatted error message from response', async () => { }); }); -it('fetchTimeRange with shift', async () => { +test('fetchTimeRange with shift', async () => { fetchMock.getOnce( "glob:*/api/v1/time_range/?q=!((timeRange:'Last+day'),(shift%3A'last%20month'%2CtimeRange%3A'Last%20day'))", { @@ -150,7 +150,7 @@ it('fetchTimeRange with shift', async () => { }); }); -it('formatTimeRangeComparison', () => { +test('formatTimeRangeComparison', () => { expect( formatTimeRangeComparison( '2021-04-13T00:00:00 : 2021-04-14T00:00:00', diff --git a/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts b/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts index c2f6c9511b504..9b14e0062c8e4 100644 --- a/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/time-comparison/getTimeOffset.test.ts @@ -16,147 +16,31 @@ * specific language governing permissions and limitations * under the License. */ -import { getTimeOffset, parseDttmToDate } from '@superset-ui/core'; +import { getTimeOffset } from '@superset-ui/core'; -describe('parseDttmToDate', () => { - it('should handle "now"', () => { - const now = parseDttmToDate('now'); - const expected = new Date(); - expected.setUTCHours(0, 0, 0, 0); - expect(expected).toEqual(now); - }); +test('handles custom shifts', () => { + const shifts = ['custom']; + const startDate = '2023-01-01'; + const timeRangeFilter = { comparator: '2023-01-03 : 2023-01-10' }; - it('should handle "today" and "No filter"', () => { - const today = parseDttmToDate('today'); - const noFilter = parseDttmToDate('No filter'); - const expected = new Date(); - expected.setUTCHours(0, 0, 0, 0); - expect(today).toEqual(expected); - expect(noFilter).toEqual(expected); - }); - - it('should handle relative time strings', () => { - const lastWeek = parseDttmToDate('Last week'); - const lastMonth = parseDttmToDate('Last month'); - const lastQuarter = parseDttmToDate('Last quarter'); - const lastYear = parseDttmToDate('Last year'); - let now = new Date(); - now.setUTCHours(0, 0, 0, 0); - now.setUTCDate(now.getUTCDate() - 7); - expect(lastWeek).toEqual(now); - - now = new Date(); - now.setUTCHours(0, 0, 0, 0); - now.setUTCMonth(now.getUTCMonth() - 1); - now.setUTCDate(1); - expect(lastMonth).toEqual(now); - - now = new Date(); - now.setUTCHours(0, 0, 0, 0); - now.setUTCMonth(now.getUTCMonth() - 3); - now.setUTCDate(1); - expect(lastQuarter).toEqual(now); - - now = new Date(); - now.setUTCHours(0, 0, 0, 0); - now.setUTCFullYear(now.getUTCFullYear() - 1); - now.setUTCDate(1); - expect(lastYear).toEqual(now); - }); - - it('should handle previous calendar units', () => { - let now = new Date(); - now.setUTCHours(0, 0, 0, 0); - now.setUTCDate(now.getUTCDate() - now.getUTCDay()); - const previousWeek = parseDttmToDate('previous calendar week'); - expect(previousWeek).toEqual(now); - - now = new Date(); - now.setUTCMonth(now.getUTCMonth() - 1, 1); - now.setUTCHours(0, 0, 0, 0); - const previousMonth = parseDttmToDate('previous calendar month'); - expect(previousMonth).toEqual(now); - - now = new Date(); - now.setUTCFullYear(now.getUTCFullYear() - 1, 0, 1); - now.setUTCHours(0, 0, 0, 0); - const previousYear = parseDttmToDate('previous calendar year'); - expect(previousYear).toEqual(now); - }); - - it('should handle dynamic "ago" times', () => { - const fiveDaysAgo = parseDttmToDate('5 days ago'); - const fiveDayAgo = parseDttmToDate('5 day ago'); - let now = new Date(); - now.setUTCHours(0, 0, 0, 0); - now.setUTCDate(now.getUTCDate() - 5); - expect(fiveDaysAgo).toEqual(now); - expect(fiveDayAgo).toEqual(now); - - const weeksAgo = parseDttmToDate('7 weeks ago'); - const weekAgo = parseDttmToDate('7 week ago'); - now = new Date(); - now.setUTCHours(0, 0, 0, 0); - now.setUTCDate(now.getUTCDate() - 7 * 7); - expect(weeksAgo).toEqual(now); - expect(weekAgo).toEqual(now); - - const fiveMonthsAgo = parseDttmToDate('5 months ago'); - const fiveMonthAgo = parseDttmToDate('5 month ago'); - now = new Date(); - now.setUTCHours(0, 0, 0, 0); - now.setUTCMonth(now.getUTCMonth() - 5); - expect(fiveMonthsAgo).toEqual(now); - expect(fiveMonthAgo).toEqual(now); - - const fiveYearsAgo = parseDttmToDate('5 years ago'); - const fiveYearAgo = parseDttmToDate('5 year ago'); - now = new Date(); - now.setUTCHours(0, 0, 0, 0); - now.setUTCFullYear(now.getUTCFullYear() - 5); - expect(fiveYearsAgo).toEqual(now); - expect(fiveYearAgo).toEqual(now); - - // default case - const fiveHoursAgo = parseDttmToDate('5 hours ago'); - now = new Date(); - now.setUTCHours(0, 0, 0, 0); - expect(fiveHoursAgo).toEqual(now); - }); - - it('should parse valid moment strings', () => { - const specificDate = new Date('2023-01-01'); - specificDate.setUTCHours(0, 0, 0, 0); - const parsedDate = parseDttmToDate('2023-01-01'); - expect(parsedDate).toEqual(specificDate); - }); + const result = getTimeOffset(timeRangeFilter, shifts, startDate); + expect(result).toEqual(['2 days ago']); }); -describe('getTimeOffset', () => { - it('handles custom shifts', () => { - const shifts = ['custom']; - const startDate = '2023-01-01'; - const timeRangeFilter = { comparator: '2023-01-03 : 2023-01-10' }; - - const result = getTimeOffset(timeRangeFilter, shifts, startDate); - expect(result).toEqual(['2 days ago']); - }); +test('handles inherit shifts', () => { + const shifts = ['inherit']; + const startDate = ''; + const timeRangeFilter = { comparator: '2023-01-03 : 2023-01-10' }; - it('handles inherit shifts', () => { - const shifts = ['inherit']; - const startDate = ''; - const timeRangeFilter = { comparator: '2023-01-03 : 2023-01-10' }; - - const result = getTimeOffset(timeRangeFilter, shifts, startDate); - expect(result).toEqual(['7 days ago']); - }); + const result = getTimeOffset(timeRangeFilter, shifts, startDate); + expect(result).toEqual(['7 days ago']); +}); - it('handles no custom or inherit shifts', () => { - const shifts = ['1 week ago']; - const startDate = ''; - const timeRangeFilter = { comparator: '2023-01-03 : 2023-01-10' }; +test('handles no custom or inherit shifts', () => { + const shifts = ['1 week ago']; + const startDate = ''; + const timeRangeFilter = { comparator: '2023-01-03 : 2023-01-10' }; - const result = getTimeOffset(timeRangeFilter, shifts, startDate); - expect(result).toEqual(['1 week ago']); - }); + const result = getTimeOffset(timeRangeFilter, shifts, startDate); + expect(result).toEqual(['1 week ago']); }); diff --git a/superset-frontend/packages/superset-ui-core/test/time-comparison/parseDttmToDate.test.ts b/superset-frontend/packages/superset-ui-core/test/time-comparison/parseDttmToDate.test.ts new file mode 100644 index 0000000000000..30c5d285bc5de --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/test/time-comparison/parseDttmToDate.test.ts @@ -0,0 +1,131 @@ +/** + * 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 { parseDttmToDate } from '@superset-ui/core'; + +test('should handle "now"', () => { + const now = parseDttmToDate('now'); + const expected = new Date(); + expected.setUTCHours(0, 0, 0, 0); + expect(expected).toEqual(now); +}); + +test('should handle "today" and "No filter"', () => { + const today = parseDttmToDate('today'); + const noFilter = parseDttmToDate('No filter'); + const expected = new Date(); + expected.setUTCHours(0, 0, 0, 0); + expect(today).toEqual(expected); + expect(noFilter).toEqual(expected); +}); + +test('should handle relative time strings', () => { + const lastWeek = parseDttmToDate('Last week'); + const lastMonth = parseDttmToDate('Last month'); + const lastQuarter = parseDttmToDate('Last quarter'); + const lastYear = parseDttmToDate('Last year'); + let now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCDate(now.getUTCDate() - 7); + expect(lastWeek).toEqual(now); + + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCMonth(now.getUTCMonth() - 1); + now.setUTCDate(1); + expect(lastMonth).toEqual(now); + + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCMonth(now.getUTCMonth() - 3); + now.setUTCDate(1); + expect(lastQuarter).toEqual(now); + + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCFullYear(now.getUTCFullYear() - 1); + now.setUTCDate(1); + expect(lastYear).toEqual(now); +}); + +test('should handle previous calendar units', () => { + let now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCDate(now.getUTCDate() - now.getUTCDay()); + const previousWeek = parseDttmToDate('previous calendar week'); + expect(previousWeek).toEqual(now); + + now = new Date(); + now.setUTCMonth(now.getUTCMonth() - 1, 1); + now.setUTCHours(0, 0, 0, 0); + const previousMonth = parseDttmToDate('previous calendar month'); + expect(previousMonth).toEqual(now); + + now = new Date(); + now.setUTCFullYear(now.getUTCFullYear() - 1, 0, 1); + now.setUTCHours(0, 0, 0, 0); + const previousYear = parseDttmToDate('previous calendar year'); + expect(previousYear).toEqual(now); +}); + +test('should handle dynamic "ago" times', () => { + const fiveDaysAgo = parseDttmToDate('5 days ago'); + const fiveDayAgo = parseDttmToDate('5 day ago'); + let now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCDate(now.getUTCDate() - 5); + expect(fiveDaysAgo).toEqual(now); + expect(fiveDayAgo).toEqual(now); + + const weeksAgo = parseDttmToDate('7 weeks ago'); + const weekAgo = parseDttmToDate('7 week ago'); + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCDate(now.getUTCDate() - 7 * 7); + expect(weeksAgo).toEqual(now); + expect(weekAgo).toEqual(now); + + const fiveMonthsAgo = parseDttmToDate('5 months ago'); + const fiveMonthAgo = parseDttmToDate('5 month ago'); + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCMonth(now.getUTCMonth() - 5); + expect(fiveMonthsAgo).toEqual(now); + expect(fiveMonthAgo).toEqual(now); + + const fiveYearsAgo = parseDttmToDate('5 years ago'); + const fiveYearAgo = parseDttmToDate('5 year ago'); + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + now.setUTCFullYear(now.getUTCFullYear() - 5); + expect(fiveYearsAgo).toEqual(now); + expect(fiveYearAgo).toEqual(now); + + // default case + const fiveHoursAgo = parseDttmToDate('5 hours ago'); + now = new Date(); + now.setUTCHours(0, 0, 0, 0); + expect(fiveHoursAgo).toEqual(now); +}); + +test('should parse valid moment strings', () => { + const specificDate = new Date('2023-01-01'); + specificDate.setUTCHours(0, 0, 0, 0); + const parsedDate = parseDttmToDate('2023-01-01'); + expect(parsedDate).toEqual(specificDate); +}); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts index 1e75d1884ffde..ce934c43360b6 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts @@ -108,7 +108,11 @@ const config: ControlPanelConfig = { ], ], }, - sections.timeComparisonControls(false, false, false), + sections.timeComparisonControls({ + multi: false, + showCalculationType: false, + showFullChoices: false, + }), ], controlOverrides: { y_axis_format: { From 5a6b7705010b5de34e6f1dd43ba7b4e312a62637 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Thu, 16 May 2024 15:38:03 +0200 Subject: [PATCH 26/27] Time Comparison: - Fix how we separate the time_grain scenario with join keys - Simplify when we raise time grain required exception --- superset/common/query_context_processor.py | 30 ++++++++++------------ 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index 29f52816dd98c..75b3e4e976c7a 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -549,21 +549,19 @@ def join_offset_dfs( time_grain ) - use_aggregated_join_column = ( - join_column_producer or time_grain in AGGREGATED_JOIN_GRAINS - ) - - if use_aggregated_join_column and not time_grain: + if join_column_producer and not time_grain: raise QueryObjectValidationError( _("Time Grain must be specified when using Time Shift.") ) # iterate on offset_dfs, left join each with df for offset, offset_df in offset_dfs.items(): - # defines a column name for the offset join column - column_name = OFFSET_JOIN_COLUMN_SUFFIX + offset + actual_join_keys = join_keys if time_grain: + # defines a column name for the offset join column + column_name = OFFSET_JOIN_COLUMN_SUFFIX + offset + # add offset join column to df self.add_offset_join_column( df, column_name, time_grain, offset, join_column_producer @@ -574,9 +572,9 @@ def join_offset_dfs( offset_df, column_name, time_grain, None, join_column_producer ) - # the temporal column is the first column in the join keys - # so we use the join column instead of the temporal column - actual_join_keys = [column_name, *join_keys[1:]] + # the temporal column is the first column in the join keys + # so we use the join column instead of the temporal column + actual_join_keys = [column_name, *join_keys[1:]] if join_keys: df = dataframe_utils.left_join_df( @@ -597,12 +595,12 @@ def join_offset_dfs( col = df.pop(join_keys[0]) df.insert(0, col.name, col) - # removes columns created only for join purposes - df.drop( - list(df.filter(regex=f"{OFFSET_JOIN_COLUMN_SUFFIX}|{R_SUFFIX}")), - axis=1, - inplace=True, - ) + # removes columns created only for join purposes + df.drop( + list(df.filter(regex=f"{OFFSET_JOIN_COLUMN_SUFFIX}|{R_SUFFIX}")), + axis=1, + inplace=True, + ) return df @staticmethod From 85a749458be381b0ef68250f11de4d0d4bde073d Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Thu, 16 May 2024 15:52:05 +0200 Subject: [PATCH 27/27] - Fix migration order --- ...-02_f84fde59123a_update_charts_with_old_time_comparison.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py b/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py index 00a200fafc740..c349344f566b5 100644 --- a/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py +++ b/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py @@ -17,7 +17,7 @@ """Update charts with old time comparison controls Revision ID: f84fde59123a -Revises: 4081be5b6b74 +Revises: 9621c6d56ffb Create Date: 2024-05-10 18:02:38.891060 """ @@ -39,7 +39,7 @@ # revision identifiers, used by Alembic. revision = "f84fde59123a" -down_revision = "4081be5b6b74" +down_revision = "9621c6d56ffb" logger = logging.getLogger(__name__) Base = declarative_base()