korbit-ai[bot] commented on code in PR #32767:
URL: https://github.com/apache/superset/pull/32767#discussion_r2006212086
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.ts:
##########
@@ -19,6 +19,7 @@
export { default as sharedControls } from './sharedControls';
// React control components
export { default as sharedControlComponents } from './components';
+export { aggregationControl } from './customControls';
export * from './components';
export * from './customControls';
Review Comment:
### Inconsistent and redundant export patterns <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The module has inconsistent export patterns and redundant exports. It mixes
named exports, default exports, and wildcard exports from the same modules.
###### Why this matters
Inconsistent export patterns make the module's API less predictable and can
lead to confusion about what is actually being exported. The redundant exports
from the same modules can lead to naming conflicts and make it harder to track
dependencies.
###### Suggested change ∙ *Feature Preview*
Standardize the export pattern by either using named exports consistently or
consolidating exports from the same module:
```typescript
export { sharedControls } from './sharedControls';
export { sharedControlComponents, ... } from './components';
export { aggregationControl, ... } from './customControls';
export * from './mixins';
export * from './dndControls';
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb3f8e5c-4559-4ea8-96ef-728cd7382d7d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb3f8e5c-4559-4ea8-96ef-728cd7382d7d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb3f8e5c-4559-4ea8-96ef-728cd7382d7d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb3f8e5c-4559-4ea8-96ef-728cd7382d7d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb3f8e5c-4559-4ea8-96ef-728cd7382d7d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:dfc6ed47-95fa-4782-abdf-f5a652550ec0 -->
[](dfc6ed47-95fa-4782-abdf-f5a652550ec0)
##########
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:
### Inefficient Array Operations <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating a new array with .map() followed by .filter() causes unnecessary
array iterations and memory allocations.
###### Why this matters
Performing separate map and filter operations requires two full array
traversals and creates intermediate arrays in memory, impacting performance
especially with large datasets.
###### Suggested change ∙ *Feature Preview*
Combine map and filter into a single reduce operation to process the array
once:
```typescript
const metricValues: number[] = sortedData.reduce((acc: number[], d) => {
if (d[1] !== null) acc.push(d[1]);
return acc;
}, []);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c025d304-3958-46c2-9841-71ca02900c9d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c025d304-3958-46c2-9841-71ca02900c9d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c025d304-3958-46c2-9841-71ca02900c9d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c025d304-3958-46c2-9841-71ca02900c9d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c025d304-3958-46c2-9841-71ca02900c9d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f18e9526-7788-4d14-8f33-8e3d8d6009af -->
[](f18e9526-7788-4d14-8f33-8e3d8d6009af)
##########
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;
Review Comment:
### Redundant Array Length Check <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Redundant length check in MIN and MAX cases as metricValues is already
filtered for non-null values and this code block only executes if
metricValues.length > 0
###### Why this matters
The unnecessary checks add complexity and could mislead developers about the
actual state of metricValues at this point in the code execution.
###### Suggested change ∙ *Feature Preview*
```typescript
case 'MIN':
bigNumber = Math.min(...metricValues);
break;
case 'MAX':
bigNumber = Math.max(...metricValues);
break;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/563363ef-873c-4471-8523-0bde6dd7ef8f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/563363ef-873c-4471-8523-0bde6dd7ef8f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/563363ef-873c-4471-8523-0bde6dd7ef8f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/563363ef-873c-4471-8523-0bde6dd7ef8f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/563363ef-873c-4471-8523-0bde6dd7ef8f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:722ee9b3-901b-4911-8248-6b246984d95d -->
[](722ee9b3-901b-4911-8248-6b246984d95d)
##########
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);
+
+ if (metricValues.length > 0) {
+ const sortedValues: number[] = [...metricValues].sort((a, b) => a - b);
Review Comment:
### Unnecessary Array Copy <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating a new array with spread operator [...metricValues] before sorting
creates an unnecessary copy of the array.
###### Why this matters
Memory consumption is doubled needlessly as the spread operator creates a
complete copy of the array in memory.
###### Suggested change ∙ *Feature Preview*
If the original array order is not needed later, sort it directly:
```typescript
const sortedValues = metricValues.sort((a, b) => a - b);
```
If original order must be preserved, use the existing copy but document the
requirement.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b1031b7-e1b1-4429-83cd-a29c7dd0b306/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b1031b7-e1b1-4429-83cd-a29c7dd0b306?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b1031b7-e1b1-4429-83cd-a29c7dd0b306?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b1031b7-e1b1-4429-83cd-a29c7dd0b306?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b1031b7-e1b1-4429-83cd-a29c7dd0b306)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2c9eb18d-de14-4be2-9148-569e55608b23 -->
[](2c9eb18d-de14-4be2-9148-569e55608b23)
##########
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:
### Unhandled Undefined State in mapStateToProps <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The mapStateToProps function doesn't handle the case where
form_data.aggregation is undefined, which could occur with legacy data or new
chart instances.
###### Why this matters
This could lead to the control showing an empty/undefined state even when
there should be a valid default value displayed.
###### Suggested change ∙ *Feature Preview*
Add a fallback value when form_data.aggregation is undefined:
```typescript
mapStateToProps: ({ form_data }: ControlPanelState) => ({
value: form_data.aggregation || 'LAST_VALUE',
}),
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ce72f9-7c35-4daa-a6b4-9e005c86dc2b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ce72f9-7c35-4daa-a6b4-9e005c86dc2b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ce72f9-7c35-4daa-a6b4-9e005c86dc2b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ce72f9-7c35-4daa-a6b4-9e005c86dc2b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ce72f9-7c35-4daa-a6b4-9e005c86dc2b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4b5deb2f-c5fa-4a50-9d76-252adf5061c5 -->
[](4b5deb2f-c5fa-4a50-9d76-252adf5061c5)
##########
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);
+
+ 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;
Review Comment:
### Redundant Null Coalescing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Unnecessary null coalescing operator in bigNumberFallback assignment as
find() already returns undefined when no element is found
###### Why this matters
The redundant null coalescing adds unnecessary complexity and doesn't
provide any additional safety or functionality.
###### Suggested change ∙ *Feature Preview*
```typescript
bigNumberFallback = sortedData.find(d => d[1] !== null);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecf52347-c247-46cf-a053-b641bc9b5b27/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecf52347-c247-46cf-a053-b641bc9b5b27?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecf52347-c247-46cf-a053-b641bc9b5b27?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecf52347-c247-46cf-a053-b641bc9b5b27?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecf52347-c247-46cf-a053-b641bc9b5b27)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8d49603f-f8a7-4123-af31-4a261331ab18 -->
[](8d49603f-f8a7-4123-af31-4a261331ab18)
##########
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);
+
+ 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:
### Unnecessary Null Checks <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Unnecessary null coalescing and length check in LAST_VALUE case as
sortedData is already guaranteed to have elements when this code block executes
###### Why this matters
The redundant checks make the code more complex than necessary and could
impact readability and maintenance.
###### Suggested change ∙ *Feature Preview*
```typescript
bigNumber = sortedData[0][1];
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/186d3889-0611-4937-a901-0c37feb66672/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/186d3889-0611-4937-a901-0c37feb66672?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/186d3889-0611-4937-a901-0c37feb66672?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/186d3889-0611-4937-a901-0c37feb66672?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/186d3889-0611-4937-a901-0c37feb66672)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d453747b-8d96-405b-895c-2ddc0cc20ae9 -->
[](d453747b-8d96-405b-895c-2ddc0cc20ae9)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/controlPanel.tsx:
##########
@@ -35,6 +36,7 @@ const config: ControlPanelConfig = {
controlSetRows: [
['x_axis'],
['time_grain_sqla'],
+ [aggregationControl],
['metric'],
['adhoc_filters'],
],
Review Comment:
### Non-intuitive Query Control Ordering <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The query controls are arranged in a non-intuitive sequence. Generally,
filters should come before metrics and aggregations as they define the data
scope.
###### Why this matters
Improper control sequencing can lead to a confusing user experience and make
it harder to understand the data flow in the visualization configuration.
###### Suggested change ∙ *Feature Preview*
Reorder the controls to follow a more logical data flow sequence:
```typescript
controlSetRows: [
['x_axis'],
['adhoc_filters'],
['time_grain_sqla'],
[aggregationControl],
['metric'],
]
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1cc6fe7d-8798-40ee-9711-96c6a8d0fbd0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1cc6fe7d-8798-40ee-9711-96c6a8d0fbd0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1cc6fe7d-8798-40ee-9711-96c6a8d0fbd0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1cc6fe7d-8798-40ee-9711-96c6a8d0fbd0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1cc6fe7d-8798-40ee-9711-96c6a8d0fbd0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:513d425f-dfb1-43bf-8073-0f985db06aa6 -->
[](513d425f-dfb1-43bf-8073-0f985db06aa6)
--
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]
