korbit-ai[bot] commented on code in PR #34296:
URL: https://github.com/apache/superset/pull/34296#discussion_r2227369508
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts:
##########
@@ -39,28 +39,37 @@ export default function buildQuery(formData: QueryFormData)
{
? ensureIsArray(getXAxisColumn(formData))
: [];
- return buildQueryContext(formData, baseQueryObject => [
- {
- ...baseQueryObject,
- columns: [...timeColumn],
- ...(timeColumn.length ? {} : { is_timeseries: true }),
- post_processing: [
- pivotOperator(formData, baseQueryObject),
- rollingWindowOperator(formData, baseQueryObject),
- resampleOperator(formData, baseQueryObject),
- flattenOperator(formData, baseQueryObject),
- ],
- },
- {
- ...baseQueryObject,
- columns: [...(isRawMetric ? [] : timeColumn)],
- is_timeseries: !isRawMetric,
- post_processing: isRawMetric
- ? []
- : [
- pivotOperator(formData, baseQueryObject),
- aggregationOperator(formData, baseQueryObject),
- ],
- },
- ]);
+ return buildQueryContext(formData, baseQueryObject => {
+ const queries = [
+ {
+ ...baseQueryObject,
+ columns: [...timeColumn],
+ ...(timeColumn.length ? {} : { is_timeseries: true }),
+ post_processing: [
+ pivotOperator(formData, baseQueryObject),
+ rollingWindowOperator(formData, baseQueryObject),
+ resampleOperator(formData, baseQueryObject),
+ flattenOperator(formData, baseQueryObject),
+ ].filter(Boolean),
+ },
+ ];
+
+ // Only add second query for raw metrics which need different query
structure
+ // All other aggregations (sum, mean, min, max, median, LAST_VALUE) can be
computed client-side from trendline data
+ if (formData.aggregation === 'raw') {
+ queries.push({
+ ...baseQueryObject,
+ columns: [...(isRawMetric ? [] : timeColumn)],
+ is_timeseries: !isRawMetric,
+ post_processing: isRawMetric
+ ? []
+ : ([
+ pivotOperator(formData, baseQueryObject),
+ aggregationOperator(formData, baseQueryObject),
+ ].filter(Boolean) as any[]),
+ });
+ }
Review Comment:
### Inconsistent Raw Metric Check <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The conditional logic for adding the second query uses both
formData.aggregation === 'raw' and isRawMetric, which is redundant since
isRawMetric is already defined as formData.aggregation === 'raw'
###### Why this matters
This redundancy could lead to confusion and potential bugs if one of these
conditions is updated without updating the other. The code flow is also less
clear due to the nested ternary using the same condition.
###### Suggested change ∙ *Feature Preview*
Simplify the logic by using only isRawMetric and removing the redundant
checks:
```typescript
if (isRawMetric) {
queries.push({
...baseQueryObject,
columns: [],
is_timeseries: false,
post_processing: [],
});
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/36e6a0f2-5c99-40b2-b3c1-5d769e48bcf2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/36e6a0f2-5c99-40b2-b3c1-5d769e48bcf2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/36e6a0f2-5c99-40b2-b3c1-5d769e48bcf2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/36e6a0f2-5c99-40b2-b3c1-5d769e48bcf2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/36e6a0f2-5c99-40b2-b3c1-5d769e48bcf2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6bbac259-79fd-45ae-8cf9-4aae253f7924 -->
[](6bbac259-79fd-45ae-8cf9-4aae253f7924)
##########
superset-frontend/src/explore/components/controls/ViewQuery.tsx:
##########
@@ -147,44 +140,57 @@ const ViewQuery: FC<ViewQueryProps> = props => {
}, [sql]);
return (
- <StyledSyntaxContainer key={sql}>
- <StyledHeaderMenuContainer>
- <StyledHeaderActionContainer>
- <CopyToClipboard
- text={currentSQL}
- shouldShowText={false}
- copyNode={
- <CopyButtonViewQuery
- buttonSize="small"
- icon={<Icons.CopyOutlined />}
- >
- {t('Copy')}
- </CopyButtonViewQuery>
- }
- />
- <Button onClick={navToSQLLab}>{t('View in SQL Lab')}</Button>
- </StyledHeaderActionContainer>
- <StyledHeaderActionContainer>
- <Switch
- id="formatSwitch"
- checked={!showFormatSQL}
- onChange={formatCurrentQuery}
- />
- <StyledLabel htmlFor="formatSwitch">
- {t('Show original SQL')}
- </StyledLabel>
- </StyledHeaderActionContainer>
- </StyledHeaderMenuContainer>
- {!formattedSQL && <Skeleton active />}
- {formattedSQL && (
- <StyledThemedSyntaxHighlighter
- language={language}
- customStyle={{ flex: 1 }}
+ <Card bodyStyle={{ padding: theme.sizeUnit * 4 }}>
+ <StyledSyntaxContainer key={sql}>
+ {!formattedSQL && <Skeleton active />}
+ {formattedSQL && (
+ <StyledThemedSyntaxHighlighter
+ language={language}
+ customStyle={{ flex: 1, marginBottom: 12 }}
+ >
+ {currentSQL}
+ </StyledThemedSyntaxHighlighter>
+ )}
+
+ <div
+ style={{
+ display: 'flex',
+ justifyContent: 'space-between',
+ alignItems: 'center',
+ }}
Review Comment:
### Inconsistent Styling Pattern <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Inline styles are used instead of styled-components, breaking the pattern
established earlier in the code where styled-components were used for styling.
###### Why this matters
Inconsistent styling patterns make the code harder to maintain and update.
Inline styles also lack the benefits of styled-components like theme support
and style reuse.
###### Suggested change ∙ *Feature Preview*
Extract inline styles into styled-components, following the existing pattern:
```typescript
const StyledActionContainer = styled.div`
display: flex;
justify-content: space-between;
align-items: center;
`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7de51f28-5fa2-438a-8431-8ca2369724e9/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7de51f28-5fa2-438a-8431-8ca2369724e9?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7de51f28-5fa2-438a-8431-8ca2369724e9?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7de51f28-5fa2-438a-8431-8ca2369724e9?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7de51f28-5fa2-438a-8431-8ca2369724e9)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:54c0c80d-15fd-4e24-acca-f048bb82d1e2 -->
[](54c0c80d-15fd-4e24-acca-f048bb82d1e2)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -43,6 +43,43 @@ const formatPercentChange = getNumberFormatter(
NumberFormats.PERCENT_SIGNED_1_POINT,
);
+// Client-side aggregation functions
+function computeClientSideAggregation(
+ data: [number | null, number | null][],
+ aggregation: string,
+): number | null {
Review Comment:
### Missing purpose in client-side aggregation function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function computeClientSideAggregation lacks documentation explaining the
purpose and assumptions behind the client-side aggregation strategy.
###### Why this matters
The function's purpose and the rationale for using client-side aggregation
vs server-side aggregation is not immediately clear, which could lead to misuse
or confusion during maintenance.
###### Suggested change ∙ *Feature Preview*
/**
* Performs client-side aggregation as a primary computation strategy,
falling back to server-side
* aggregation only when necessary. This approach ensures consistent
aggregation behavior
* across all data presentations.
*
* @param data - Array of timestamp-value pairs [timestamp, value]
* @param aggregation - Type of aggregation to perform ('sum', 'mean',
'min', 'max', 'median', 'raw', 'LAST_VALUE')
* @returns The aggregated value or null if no valid values exist
*/
function computeClientSideAggregation(
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee6bb66-5e61-491a-9733-73db8709bd04/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee6bb66-5e61-491a-9733-73db8709bd04?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee6bb66-5e61-491a-9733-73db8709bd04?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee6bb66-5e61-491a-9733-73db8709bd04?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee6bb66-5e61-491a-9733-73db8709bd04)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9276f1a6-0038-49a0-92c2-c1847581026f -->
[](9276f1a6-0038-49a0-92c2-c1847581026f)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -43,6 +43,43 @@
NumberFormats.PERCENT_SIGNED_1_POINT,
);
+// Client-side aggregation functions
+function computeClientSideAggregation(
+ data: [number | null, number | null][],
+ aggregation: string,
+): number | null {
+ if (!data.length) return null;
+
+ const validValues = data
+ .map(([, value]) => value)
+ .filter((value): value is number => value !== null);
+
+ if (!validValues.length) return null;
+
+ switch (aggregation) {
+ case 'sum':
+ return validValues.reduce((a, b) => a + b, 0);
Review Comment:
### Sum Aggregation Overflow Risk <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sum aggregation function may lead to arithmetic overflow with large
numbers due to using a simple reduce operation without safeguards.
###### Why this matters
When dealing with large numbers, summing them directly can exceed
JavaScript's Number.MAX_SAFE_INTEGER (2^53 - 1), leading to precision loss or
incorrect results.
###### Suggested change ∙ *Feature Preview*
Implement a safe sum operation with overflow checks:
```typescript
case 'sum': {
return validValues.reduce((a, b) => {
const sum = a + b;
if (!Number.isSafeInteger(sum)) {
console.warn('Sum operation exceeds safe integer bounds');
}
return sum;
}, 0);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb9e3fc2-ed2d-4323-bd7d-090ecbd65a0d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb9e3fc2-ed2d-4323-bd7d-090ecbd65a0d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb9e3fc2-ed2d-4323-bd7d-090ecbd65a0d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb9e3fc2-ed2d-4323-bd7d-090ecbd65a0d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb9e3fc2-ed2d-4323-bd7d-090ecbd65a0d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8b008133-0cce-4cdb-85ba-c49d6929db7c -->
[](8b008133-0cce-4cdb-85ba-c49d6929db7c)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -43,6 +43,43 @@
NumberFormats.PERCENT_SIGNED_1_POINT,
);
+// Client-side aggregation functions
+function computeClientSideAggregation(
+ data: [number | null, number | null][],
+ aggregation: string,
+): number | null {
+ if (!data.length) return null;
+
+ const validValues = data
+ .map(([, value]) => value)
+ .filter((value): value is number => value !== null);
+
+ if (!validValues.length) return null;
+
+ switch (aggregation) {
+ case 'sum':
+ return validValues.reduce((a, b) => a + b, 0);
+ case 'mean':
+ return validValues.reduce((a, b) => a + b, 0) / validValues.length;
+ case 'min':
+ return Math.min(...validValues);
+ case 'max':
+ return Math.max(...validValues);
+ case 'median': {
+ const sorted = [...validValues].sort((a, b) => a - b);
Review Comment:
### Inefficient Median Calculation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating a new array and sorting it for median calculation has O(n log n)
time complexity and O(n) space complexity.
###### Why this matters
For large datasets, this operation can be expensive and consume unnecessary
memory. A selection algorithm could find the median in O(n) time with O(1)
space.
###### Suggested change ∙ *Feature Preview*
Use QuickSelect algorithm for finding the median. Here's a basic
implementation:
```typescript
function quickSelect(arr: number[], k: number): number {
// QuickSelect implementation that modifies array in-place
// and returns kth smallest element in O(n) average time
}
case 'median': {
const k = Math.floor(validValues.length / 2);
return validValues.length % 2 === 0
? (quickSelect(validValues, k - 1) + quickSelect(validValues, k)) / 2
: quickSelect(validValues, k);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b273408-9bbf-4c57-936e-f485eefe9797/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b273408-9bbf-4c57-936e-f485eefe9797?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b273408-9bbf-4c57-936e-f485eefe9797?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b273408-9bbf-4c57-936e-f485eefe9797?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4b273408-9bbf-4c57-936e-f485eefe9797)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:bda31217-9e02-47fb-88b7-4cb7872ff334 -->
[](bda31217-9e02-47fb-88b7-4cb7872ff334)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -43,6 +43,43 @@
NumberFormats.PERCENT_SIGNED_1_POINT,
);
+// Client-side aggregation functions
+function computeClientSideAggregation(
+ data: [number | null, number | null][],
+ aggregation: string,
+): number | null {
+ if (!data.length) return null;
+
+ const validValues = data
+ .map(([, value]) => value)
+ .filter((value): value is number => value !== null);
+
+ if (!validValues.length) return null;
+
+ switch (aggregation) {
+ case 'sum':
+ return validValues.reduce((a, b) => a + b, 0);
+ case 'mean':
+ return validValues.reduce((a, b) => a + b, 0) / validValues.length;
+ case 'min':
+ return Math.min(...validValues);
+ case 'max':
+ return Math.max(...validValues);
+ case 'median': {
+ const sorted = [...validValues].sort((a, b) => a - b);
+ const mid = Math.floor(sorted.length / 2);
+ return sorted.length % 2 === 0
+ ? (sorted[mid - 1] + sorted[mid]) / 2
+ : sorted[mid];
+ }
Review Comment:
### Median Calculation Overflow Risk <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The median calculation doesn't handle potential numeric overflow when
averaging the two middle values for even-length arrays.
###### Why this matters
When calculating the median for even-length arrays, adding two potentially
large numbers before division could cause overflow, leading to incorrect
results.
###### Suggested change ∙ *Feature Preview*
Implement a safe median calculation with overflow protection:
```typescript
case 'median': {
const sorted = [...validValues].sort((a, b) => a - b);
const mid = Math.floor(sorted.length / 2);
if (sorted.length % 2 === 0) {
return sorted[mid - 1] / 2 + sorted[mid] / 2; // Divide first to prevent
overflow
}
return sorted[mid];
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d30b1c45-a590-4c55-8282-d4486d2880f1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d30b1c45-a590-4c55-8282-d4486d2880f1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d30b1c45-a590-4c55-8282-d4486d2880f1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d30b1c45-a590-4c55-8282-d4486d2880f1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d30b1c45-a590-4c55-8282-d4486d2880f1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d253eb02-64b9-4574-b39a-b494c1b7d243 -->
[](d253eb02-64b9-4574-b39a-b494c1b7d243)
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
