betodealmeida commented on code in PR #32767:
URL: https://github.com/apache/superset/pull/32767#discussion_r2006291294
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -107,13 +108,54 @@ export default function transformProps(
// sort in time descending order
.sort((a, b) => (a[0] !== null && b[0] !== null ? b[0] - a[0] : 0));
- bigNumber = sortedData[0][1];
timestamp = sortedData[0][0];
- if (bigNumber === null) {
- bigNumberFallback = sortedData.find(d => d[1] !== null);
- bigNumber = bigNumberFallback ? bigNumberFallback[1] : null;
- timestamp = bigNumberFallback ? bigNumberFallback[0] : null;
+ const metricValues: number[] = sortedData
+ .map(d => d[1])
+ .filter((value): value is number => value !== null);
+
+ if (metricValues.length > 0) {
+ const sortedValues: number[] = [...metricValues].sort((a, b) => a - b);
+ const totalSum: number = metricValues.reduce(
+ (sum, value) => sum + value,
+ 0,
+ );
+ const mid = Math.floor(sortedValues.length / 2);
+
+ switch (aggregation) {
+ case 'SUM':
+ bigNumber = totalSum;
+ break;
+ case 'AVG':
+ bigNumber = totalSum / metricValues.length;
+ break;
+ case 'MIN':
+ bigNumber =
+ metricValues.length > 0 ? Math.min(...metricValues) : null;
Review Comment:
```suggestion
Math.min(...metricValues);
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -107,13 +108,54 @@ export default function transformProps(
// sort in time descending order
.sort((a, b) => (a[0] !== null && b[0] !== null ? b[0] - a[0] : 0));
- bigNumber = sortedData[0][1];
timestamp = sortedData[0][0];
- if (bigNumber === null) {
- bigNumberFallback = sortedData.find(d => d[1] !== null);
- bigNumber = bigNumberFallback ? bigNumberFallback[1] : null;
- timestamp = bigNumberFallback ? bigNumberFallback[0] : null;
+ const metricValues: number[] = sortedData
+ .map(d => d[1])
+ .filter((value): value is number => value !== null);
+
+ if (metricValues.length > 0) {
+ const sortedValues: number[] = [...metricValues].sort((a, b) => a - b);
+ const totalSum: number = metricValues.reduce(
+ (sum, value) => sum + value,
+ 0,
+ );
+ const mid = Math.floor(sortedValues.length / 2);
Review Comment:
It's better to move this inside `case 'MEDIAN':`, so it's only computed if
needed.
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -107,13 +108,54 @@ export default function transformProps(
// sort in time descending order
.sort((a, b) => (a[0] !== null && b[0] !== null ? b[0] - a[0] : 0));
- bigNumber = sortedData[0][1];
timestamp = sortedData[0][0];
- if (bigNumber === null) {
- bigNumberFallback = sortedData.find(d => d[1] !== null);
- bigNumber = bigNumberFallback ? bigNumberFallback[1] : null;
- timestamp = bigNumberFallback ? bigNumberFallback[0] : null;
+ const metricValues: number[] = sortedData
+ .map(d => d[1])
+ .filter((value): value is number => value !== null);
+
+ if (metricValues.length > 0) {
+ const sortedValues: number[] = [...metricValues].sort((a, b) => a - b);
+ const totalSum: number = metricValues.reduce(
+ (sum, value) => sum + value,
+ 0,
+ );
+ const mid = Math.floor(sortedValues.length / 2);
+
+ switch (aggregation) {
+ case 'SUM':
+ bigNumber = totalSum;
+ break;
+ case 'AVG':
+ bigNumber = totalSum / metricValues.length;
+ break;
+ case 'MIN':
+ bigNumber =
+ metricValues.length > 0 ? Math.min(...metricValues) : null;
+ break;
+ case 'MAX':
+ bigNumber =
+ metricValues.length > 0 ? Math.max(...metricValues) : null;
+ break;
+ case 'MEDIAN':
+ bigNumber =
+ sortedValues.length % 2 === 0
+ ? ((sortedValues[mid - 1] ?? 0) + (sortedValues[mid] ?? 0)) / 2
+ : (sortedValues[mid] ?? 0);
+ break;
+ case 'LAST_VALUE':
+ default:
+ bigNumber = sortedData.length > 0 ? (sortedData[0][1] ?? null) :
null;
+
+ if (bigNumber === null) {
+ bigNumberFallback = sortedData.find(d => d[1] !== null) ?? null;
+ bigNumber = bigNumberFallback ? bigNumberFallback[1] : null;
+ timestamp = bigNumberFallback ? bigNumberFallback[0] : null;
+ }
Review Comment:
This should be moved to line 160, so that it also applies when `bigNumber`
is null because of line 158.
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -107,13 +108,54 @@
// sort in time descending order
.sort((a, b) => (a[0] !== null && b[0] !== null ? b[0] - a[0] : 0));
- bigNumber = sortedData[0][1];
timestamp = sortedData[0][0];
- if (bigNumber === null) {
- bigNumberFallback = sortedData.find(d => d[1] !== null);
- bigNumber = bigNumberFallback ? bigNumberFallback[1] : null;
- timestamp = bigNumberFallback ? bigNumberFallback[0] : null;
+ const metricValues: number[] = sortedData
+ .map(d => d[1])
+ .filter((value): value is number => value !== null);
Review Comment:
Legibility counts.
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -107,13 +108,54 @@ export default function transformProps(
// sort in time descending order
.sort((a, b) => (a[0] !== null && b[0] !== null ? b[0] - a[0] : 0));
- bigNumber = sortedData[0][1];
timestamp = sortedData[0][0];
- if (bigNumber === null) {
- bigNumberFallback = sortedData.find(d => d[1] !== null);
- bigNumber = bigNumberFallback ? bigNumberFallback[1] : null;
- timestamp = bigNumberFallback ? bigNumberFallback[0] : null;
+ const metricValues: number[] = sortedData
+ .map(d => d[1])
+ .filter((value): value is number => value !== null);
+
+ if (metricValues.length > 0) {
+ const sortedValues: number[] = [...metricValues].sort((a, b) => a - b);
+ const totalSum: number = metricValues.reduce(
+ (sum, value) => sum + value,
+ 0,
+ );
+ const mid = Math.floor(sortedValues.length / 2);
+
+ switch (aggregation) {
+ case 'SUM':
+ bigNumber = totalSum;
+ break;
+ case 'AVG':
+ bigNumber = totalSum / metricValues.length;
+ break;
+ case 'MIN':
+ bigNumber =
+ metricValues.length > 0 ? Math.min(...metricValues) : null;
+ break;
+ case 'MAX':
+ bigNumber =
+ metricValues.length > 0 ? Math.max(...metricValues) : null;
+ break;
+ case 'MEDIAN':
+ bigNumber =
+ sortedValues.length % 2 === 0
+ ? ((sortedValues[mid - 1] ?? 0) + (sortedValues[mid] ?? 0)) / 2
+ : (sortedValues[mid] ?? 0);
+ break;
+ case 'LAST_VALUE':
+ default:
+ bigNumber = sortedData.length > 0 ? (sortedData[0][1] ?? null) :
null;
Review Comment:
You already check that `metricValues` is not empty, so you can just:
```suggestion
bigNumber = sortedData[0][1];
```
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -61,6 +61,28 @@ const xAxisSortVisibility = ({ controls }: { controls:
ControlStateMapping }) =>
ensureIsArray(controls?.groupby?.value).length === 0 &&
ensureIsArray(controls?.metrics?.value).length === 1;
+export const aggregationControl = {
+ name: 'aggregation',
+ config: {
+ type: 'SelectControl',
+ label: t('Aggregation Method'),
+ choices: [
+ ['LAST_VALUE', t('Last Value')],
+ ['SUM', t('Total(Sum)')],
+ ['AVG', t('Average(Mean)')],
+ ['MIN', t('Minimum')],
+ ['MAX', t('Maximum')],
+ ['MEDIAN', t('Median')],
+ ],
+ description: t('Select an aggregation method to apply to the metric.'),
+ renderTrigger: true,
+ provideFormDataToProps: true,
+ mapStateToProps: ({ form_data }: ControlPanelState) => ({
+ value: form_data.aggregation,
Review Comment:
@korbit-ai seems correct here. What happens with existing charts?
```suggestion
value: form_data.aggregation || 'LAST_VALUE',
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -107,13 +108,54 @@ export default function transformProps(
// sort in time descending order
.sort((a, b) => (a[0] !== null && b[0] !== null ? b[0] - a[0] : 0));
- bigNumber = sortedData[0][1];
timestamp = sortedData[0][0];
- if (bigNumber === null) {
- bigNumberFallback = sortedData.find(d => d[1] !== null);
- bigNumber = bigNumberFallback ? bigNumberFallback[1] : null;
- timestamp = bigNumberFallback ? bigNumberFallback[0] : null;
+ const metricValues: number[] = sortedData
+ .map(d => d[1])
+ .filter((value): value is number => value !== null);
+
+ if (metricValues.length > 0) {
+ const sortedValues: number[] = [...metricValues].sort((a, b) => a - b);
+ const totalSum: number = metricValues.reduce(
+ (sum, value) => sum + value,
+ 0,
+ );
+ const mid = Math.floor(sortedValues.length / 2);
+
+ switch (aggregation) {
+ case 'SUM':
+ bigNumber = totalSum;
+ break;
+ case 'AVG':
+ bigNumber = totalSum / metricValues.length;
+ break;
+ case 'MIN':
+ bigNumber =
+ metricValues.length > 0 ? Math.min(...metricValues) : null;
+ break;
+ case 'MAX':
+ bigNumber =
+ metricValues.length > 0 ? Math.max(...metricValues) : null;
Review Comment:
```suggestion
Math.max(...metricValues);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
