Skip to content

Commit 0964a8b

Browse files
mistercrunchmistercrunch
authored
fix(big number with trendline): running 2 identical queries for no good reason (#34296)
1 parent 8de8f95 commit 0964a8b

File tree

11 files changed

+304
-212
lines changed

11 files changed

+304
-212
lines changed

‎superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx

Lines changed: 52 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,53 @@ import {
4141
import { checkColumnType } from '../utils/checkColumnType';
4242
import { isSortable } from '../utils/isSortable';
4343

44+
// Aggregation choices with computation methods for plugins and controls
45+
export const aggregationChoices = {
46+
raw: {
47+
label: 'Overall value',
48+
compute: (data: number[]) => {
49+
if (!data.length) return null;
50+
return data[0];
51+
},
52+
},
53+
LAST_VALUE: {
54+
label: 'Last Value',
55+
compute: (data: number[]) => {
56+
if (!data.length) return null;
57+
return data[0];
58+
},
59+
},
60+
sum: {
61+
label: 'Total (Sum)',
62+
compute: (data: number[]) =>
63+
data.length ? data.reduce((a, b) => a + b, 0) : null,
64+
},
65+
mean: {
66+
label: 'Average (Mean)',
67+
compute: (data: number[]) =>
68+
data.length ? data.reduce((a, b) => a + b, 0) / data.length : null,
69+
},
70+
min: {
71+
label: 'Minimum',
72+
compute: (data: number[]) => (data.length ? Math.min(...data) : null),
73+
},
74+
max: {
75+
label: 'Maximum',
76+
compute: (data: number[]) => (data.length ? Math.max(...data) : null),
77+
},
78+
median: {
79+
label: 'Median',
80+
compute: (data: number[]) => {
81+
if (!data.length) return null;
82+
const sorted = [...data].sort((a, b) => a - b);
83+
const mid = Math.floor(sorted.length / 2);
84+
return sorted.length % 2 === 0
85+
? (sorted[mid - 1] + sorted[mid]) / 2
86+
: sorted[mid];
87+
},
88+
},
89+
} as const;
90+
4491
export const contributionModeControl = {
4592
name: 'contributionMode',
4693
config: {
@@ -69,17 +116,12 @@ export const aggregationControl = {
69116
default: 'LAST_VALUE',
70117
clearable: false,
71118
renderTrigger: false,
72-
choices: [
73-
['raw', t('None')],
74-
['LAST_VALUE', t('Last Value')],
75-
['sum', t('Total (Sum)')],
76-
['mean', t('Average (Mean)')],
77-
['min', t('Minimum')],
78-
['max', t('Maximum')],
79-
['median', t('Median')],
80-
],
119+
choices: Object.entries(aggregationChoices).map(([value, { label }]) => [
120+
value,
121+
t(label),
122+
]),
81123
description: t(
82-
'Aggregation method used to compute the Big Number from the Trendline.For non-additive metrics like ratios, averages, distinct counts, etc use NONE.',
124+
'Method to compute the displayed value. "Overall value" calculates a single metric across the entire filtered time period, ideal for non-additive metrics like ratios, averages, or distinct counts. Other methods operate over the time series data points.',
83125
),
84126
provideFormDataToProps: true,
85127
mapStateToProps: ({ form_data }: ControlPanelState) => ({

‎superset-frontend/packages/superset-ui-core/src/components/UnsavedChangesModal/index.tsx

Lines changed: 29 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,8 @@
1616
* specific language governing permissions and limitations
1717
* under the License.
1818
*/
19-
import { t, css, useTheme } from '@superset-ui/core';
20-
import {
21-
Icons,
22-
Modal,
23-
Typography,
24-
Button,
25-
Flex,
26-
} from '@superset-ui/core/components';
19+
import { t } from '@superset-ui/core';
20+
import { Icons, Modal, Typography, Button } from '@superset-ui/core/components';
2721
import type { FC, ReactElement } from 'react';
2822

2923
export type UnsavedChangesModalProps = {
@@ -42,66 +36,30 @@ export const UnsavedChangesModal: FC<UnsavedChangesModalProps> = ({
4236
onConfirmNavigation,
4337
title = 'Unsaved Changes',
4438
body = "If you don't save, changes will be lost.",
45-
}): ReactElement => {
46-
const theme = useTheme();
47-
48-
return (
49-
<Modal
50-
name={title}
51-
centered
52-
responsive
53-
onHide={onHide}
54-
show={showModal}
55-
width="444px"
56-
title={
57-
<Flex>
58-
<Icons.WarningOutlined
59-
iconColor={theme.colorWarning}
60-
css={css`
61-
margin-right: ${theme.sizeUnit * 2}px;
62-
`}
63-
iconSize="l"
64-
/>
65-
<Typography.Title
66-
css={css`
67-
&& {
68-
margin: 0;
69-
margin-bottom: 0;
70-
}
71-
`}
72-
level={5}
73-
>
74-
{title}
75-
</Typography.Title>
76-
</Flex>
77-
}
78-
footer={
79-
<Flex
80-
justify="flex-end"
81-
css={css`
82-
width: 100%;
83-
`}
84-
>
85-
<Button
86-
htmlType="button"
87-
buttonSize="small"
88-
buttonStyle="secondary"
89-
onClick={onConfirmNavigation}
90-
>
91-
{t('Discard')}
92-
</Button>
93-
<Button
94-
htmlType="button"
95-
buttonSize="small"
96-
buttonStyle="primary"
97-
onClick={handleSave}
98-
>
99-
{t('Save')}
100-
</Button>
101-
</Flex>
102-
}
103-
>
104-
<Typography.Text>{body}</Typography.Text>
105-
</Modal>
106-
);
107-
};
39+
}: UnsavedChangesModalProps): ReactElement => (
40+
<Modal
41+
centered
42+
responsive
43+
onHide={onHide}
44+
show={showModal}
45+
width="444px"
46+
title={
47+
<>
48+
<Icons.WarningOutlined iconSize="m" style={{ marginRight: 8 }} />
49+
{title}
50+
</>
51+
}
52+
footer={
53+
<>
54+
<Button buttonStyle="secondary" onClick={onConfirmNavigation}>
55+
{t('Discard')}
56+
</Button>
57+
<Button buttonStyle="primary" onClick={handleSave}>
58+
{t('Save')}
59+
</Button>
60+
</>
61+
}
62+
>
63+
<Typography.Text>{body}</Typography.Text>
64+
</Modal>
65+
);

‎superset-frontend/plugins/legacy-plugin-chart-parallel-coordinates/src/vendor/parcoords/d3.parcoords.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1385,15 +1385,15 @@ export default function (config) {
13851385
p[0] = p[0] - __.margin.left;
13861386
p[1] = p[1] - __.margin.top;
13871387

1388-
(dims = dimensionsForPoint(p)),
1388+
((dims = dimensionsForPoint(p)),
13891389
(strum = {
13901390
p1: p,
13911391
dims: dims,
13921392
minX: xscale(dims.left),
13931393
maxX: xscale(dims.right),
13941394
minY: 0,
13951395
maxY: h(),
1396-
});
1396+
}));
13971397

13981398
strums[dims.i] = strum;
13991399
strums.active = dims.i;
@@ -1942,7 +1942,7 @@ export default function (config) {
19421942
p[0] = p[0] - __.margin.left;
19431943
p[1] = p[1] - __.margin.top;
19441944

1945-
(dims = dimensionsForPoint(p)),
1945+
((dims = dimensionsForPoint(p)),
19461946
(arc = {
19471947
p1: p,
19481948
dims: dims,
@@ -1953,7 +1953,7 @@ export default function (config) {
19531953
startAngle: undefined,
19541954
endAngle: undefined,
19551955
arc: d3.svg.arc().innerRadius(0),
1956-
});
1956+
}));
19571957

19581958
arcs[dims.i] = arc;
19591959
arcs.active = dims.i;

‎superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.test.ts

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -49,38 +49,53 @@ describe('BigNumberWithTrendline buildQuery', () => {
4949
aggregation: null,
5050
};
5151

52-
it('creates raw metric query when aggregation is null', () => {
53-
const queryContext = buildQuery({ ...baseFormData });
52+
it('creates raw metric query when aggregation is "raw"', () => {
53+
const queryContext = buildQuery({ ...baseFormData, aggregation: 'raw' });
5454
const bigNumberQuery = queryContext.queries[1];
5555

56-
expect(bigNumberQuery.post_processing).toEqual([{ operation: 'pivot' }]);
57-
expect(bigNumberQuery.is_timeseries).toBe(true);
56+
expect(bigNumberQuery.post_processing).toEqual([]);
57+
expect(bigNumberQuery.is_timeseries).toBe(false);
58+
expect(bigNumberQuery.columns).toEqual([]);
5859
});
5960

60-
it('adds aggregation operator when aggregation is "sum"', () => {
61+
it('returns single query for aggregation methods that can be computed client-side', () => {
6162
const queryContext = buildQuery({ ...baseFormData, aggregation: 'sum' });
62-
const bigNumberQuery = queryContext.queries[1];
6363

64-
expect(bigNumberQuery.post_processing).toEqual([
64+
expect(queryContext.queries.length).toBe(1);
65+
expect(queryContext.queries[0].post_processing).toEqual([
6566
{ operation: 'pivot' },
66-
{ operation: 'aggregation', options: { operator: 'sum' } },
67+
{ operation: 'rolling' },
68+
{ operation: 'resample' },
69+
{ operation: 'flatten' },
6770
]);
68-
expect(bigNumberQuery.is_timeseries).toBe(true);
6971
});
7072

71-
it('skips aggregation when aggregation is LAST_VALUE', () => {
73+
it('returns single query for LAST_VALUE aggregation', () => {
7274
const queryContext = buildQuery({
7375
...baseFormData,
7476
aggregation: 'LAST_VALUE',
7577
});
76-
const bigNumberQuery = queryContext.queries[1];
7778

78-
expect(bigNumberQuery.post_processing).toEqual([{ operation: 'pivot' }]);
79-
expect(bigNumberQuery.is_timeseries).toBe(true);
79+
expect(queryContext.queries.length).toBe(1);
80+
expect(queryContext.queries[0].post_processing).toEqual([
81+
{ operation: 'pivot' },
82+
{ operation: 'rolling' },
83+
{ operation: 'resample' },
84+
{ operation: 'flatten' },
85+
]);
8086
});
8187

82-
it('always returns two queries', () => {
83-
const queryContext = buildQuery({ ...baseFormData });
88+
it('returns two queries only for raw aggregation', () => {
89+
const queryContext = buildQuery({ ...baseFormData, aggregation: 'raw' });
8490
expect(queryContext.queries.length).toBe(2);
91+
92+
const queryContextLastValue = buildQuery({
93+
...baseFormData,
94+
aggregation: 'LAST_VALUE',
95+
});
96+
expect(queryContextLastValue.queries.length).toBe(1);
97+
98+
const queryContextSum = buildQuery({ ...baseFormData, aggregation: 'sum' });
99+
expect(queryContextSum.queries.length).toBe(1);
85100
});
86101
});

‎superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts

Lines changed: 33 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -39,28 +39,37 @@ export default function buildQuery(formData: QueryFormData) {
3939
? ensureIsArray(getXAxisColumn(formData))
4040
: [];
4141

42-
return buildQueryContext(formData, baseQueryObject => [
43-
{
44-
...baseQueryObject,
45-
columns: [...timeColumn],
46-
...(timeColumn.length ? {} : { is_timeseries: true }),
47-
post_processing: [
48-
pivotOperator(formData, baseQueryObject),
49-
rollingWindowOperator(formData, baseQueryObject),
50-
resampleOperator(formData, baseQueryObject),
51-
flattenOperator(formData, baseQueryObject),
52-
],
53-
},
54-
{
55-
...baseQueryObject,
56-
columns: [...(isRawMetric ? [] : timeColumn)],
57-
is_timeseries: !isRawMetric,
58-
post_processing: isRawMetric
59-
? []
60-
: [
61-
pivotOperator(formData, baseQueryObject),
62-
aggregationOperator(formData, baseQueryObject),
63-
],
64-
},
65-
]);
42+
return buildQueryContext(formData, baseQueryObject => {
43+
const queries = [
44+
{
45+
...baseQueryObject,
46+
columns: [...timeColumn],
47+
...(timeColumn.length ? {} : { is_timeseries: true }),
48+
post_processing: [
49+
pivotOperator(formData, baseQueryObject),
50+
rollingWindowOperator(formData, baseQueryObject),
51+
resampleOperator(formData, baseQueryObject),
52+
flattenOperator(formData, baseQueryObject),
53+
].filter(Boolean),
54+
},
55+
];
56+
57+
// Only add second query for raw metrics which need different query structure
58+
// All other aggregations (sum, mean, min, max, median, LAST_VALUE) can be computed client-side from trendline data
59+
if (formData.aggregation === 'raw') {
60+
queries.push({
61+
...baseQueryObject,
62+
columns: [...(isRawMetric ? [] : timeColumn)],
63+
is_timeseries: !isRawMetric,
64+
post_processing: isRawMetric
65+
? []
66+
: ([
67+
pivotOperator(formData, baseQueryObject),
68+
aggregationOperator(formData, baseQueryObject),
69+
].filter(Boolean) as any[]),
70+
});
71+
}
72+
73+
return queries;
74+
});
6675
}

0 commit comments

Comments
 (0)