You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2025/07/24 05:12:10 UTC
[PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
mistercrunch opened a new pull request, #34296:
URL: https://github.com/apache/superset/pull/34296
## Summary
Fixes duplicate query issue in BigNumber with Trendline charts and improves the View Query modal UX. PR #33407 introduced duplicate identical queries for `LAST_VALUE` aggregation (the default), causing unnecessary database load and
confusing query inspection.
<img width="427" height="181" alt="Screenshot 2025-07-23 at 9 14 05 PM" src="https://github.com/user-attachments/assets/6f0ab29f-5b09-4644-ada2-d638343bd039" />
## Changes
**Query Optimization:**
- **`LAST_VALUE`**: Now uses 1 query (most recent trendline value)
- **`None`**: Preserves 2 queries (trendline + raw server aggregation)
- **Math aggregations**: Uses client-side computation from trendline data
**UI Improvements:**
- Wrapped each query in AntD Cards for better visual separation
- Moved action buttons below SQL code with secondary styling
- Improved spacing and layout using theme units
- Enhanced aggregation tooltip explaining cross-timeseries behavior
## Background
PR #33407 by @LevisNgigi always generated 2 queries regardless of aggregation type:
```typescript
// Before: Always 2 queries even when identical
return buildQueryContext(formData, baseQueryObject => [
{ /* trendline query */ },
{ /* aggregation query */ } // Duplicate for LAST_VALUE
]);
For LAST_VALUE (default), both queries were functionally identical since the big number is just the most recent trendline point.
Query Logic
- LAST_VALUE: 1 query → big number from most recent trendline value
- None: 2 queries → trendline + raw metric over entire time period
- sum/mean/min/max/median: 1 query → client-side aggregation from trendline
Why "None" Needs 2 Queries
For non-additive metrics like COUNT(DISTINCT user_id):
- Query 1: Daily counts [100, 120, 95] for trendline
- Query 2: Total distinct users 250 across period (can't sum daily counts)
Testing
- ✅ Updated tests to match new query behavior
- ✅ All aggregation methods produce correct results
- ✅ View Query modal shows clean card layout
Fixes performance regression from #33407 while preserving correct behavior for legitimate multi-query scenarios.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #34296:
URL: https://github.com/apache/superset/pull/34296#issuecomment-3112840268
Thanks for the detailed breakdown! I hadn’t noticed that the previous implementation was generating two queries for all aggregations as I was working to achieve that for the None option. Good catch on optimizing that.
For client-side aggregation, I had initially gone that route as well, but later shifted to backend post-processing based on earlier feedback. That said, this new approach makes a lot of sense, especially with the performance improvements and reduced query load. Appreciate the clarity
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34296:
URL: https://github.com/apache/superset/pull/34296#issuecomment-3119866723
@mistercrunch Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/16528728741). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #34296:
URL: https://github.com/apache/superset/pull/34296#issuecomment-3114797856
I know there's a bit of history going back and forth on going client/server side, seems single-timeseries aggregation are well handled on the client side. I can see it either way on my side...
Also, making `None` more descriptive calling it `Force server-side aggregation`
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34296:
URL: https://github.com/apache/superset/pull/34296#issuecomment-3119899267
@mistercrunch Ephemeral environment spinning up at http://54.191.189.47:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #34296:
URL: https://github.com/apache/superset/pull/34296#issuecomment-3114238901
> The two queries were identical, and from my understanding aggregations for everything but `None` were done client-side. Is that what we intended? Is this PR right for sure?
Aggregations for everything was done server-side but for none we would just skip the aggregation similar to last value? I agree with the duplicate query issue though.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #34296:
URL: https://github.com/apache/superset/pull/34296#issuecomment-3116531858
> > Aggregations for everything was done server-side
>
> but the query was wrong it seems, as one of the queries should have NOT included the time dimension (?)
Yes there was no need for duplicate queries, previously for something like sum this was the query, [{'operation': 'pivot',
'options': {'aggregates': {'count': {'operator': 'mean'}},
'columns': [],
'drop_missing_columns': True,
'index': ['order_date']}},
{'operation': 'aggregate',
'options': {'aggregates': {'count': {'column': 'count', 'operator': 'sum'}},
'groupby': []}}]
but with client side it is this [{'operation': 'pivot',
'options': {'aggregates': {'count': {'operator': 'mean'}},
'columns': [],
'drop_missing_columns': True,
'index': ['order_date']}},
{'operation': 'flatten'}] but even client processing works fine as well.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #34296:
URL: https://github.com/apache/superset/pull/34296
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #34296:
URL: https://github.com/apache/superset/pull/34296#issuecomment-3114223106
The two queries were identical, and from my understanding aggregations for everything but `None` were done client-side. Is that what we intended? Is this PR right for sure?
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34296:
URL: https://github.com/apache/superset/pull/34296#discussion_r2237425408
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -41,6 +41,53 @@ import {
import { checkColumnType } from '../utils/checkColumnType';
import { isSortable } from '../utils/isSortable';
+// Aggregation choices with computation methods for plugins and controls
+export const aggregationChoices = {
+ raw: {
+ label: 'Force server-side aggregation',
Review Comment:
@yousoph
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34296:
URL: https://github.com/apache/superset/pull/34296#discussion_r2240691972
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -41,6 +41,53 @@ import {
import { checkColumnType } from '../utils/checkColumnType';
import { isSortable } from '../utils/isSortable';
+// Aggregation choices with computation methods for plugins and controls
+export const aggregationChoices = {
+ raw: {
+ label: 'Force server-side aggregation',
Review Comment:
also updated the control description:
'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.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34296:
URL: https://github.com/apache/superset/pull/34296#discussion_r2240666208
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -41,6 +41,53 @@ import {
import { checkColumnType } from '../utils/checkColumnType';
import { isSortable } from '../utils/isSortable';
+// Aggregation choices with computation methods for plugins and controls
+export const aggregationChoices = {
+ raw: {
+ label: 'Force server-side aggregation',
Review Comment:
Done!
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #34296:
URL: https://github.com/apache/superset/pull/34296#issuecomment-3114757233
> Aggregations for everything was done server-side
but the query was wrong it seems, as one of the queries should have NOT included the time dimension (?)
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34296:
URL: https://github.com/apache/superset/pull/34296#discussion_r2232010776
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -41,6 +41,53 @@ import {
import { checkColumnType } from '../utils/checkColumnType';
import { isSortable } from '../utils/isSortable';
+// Aggregation choices with computation methods for plugins and controls
+export const aggregationChoices = {
+ raw: {
+ label: 'Force server-side aggregation',
Review Comment:
I'm not sure if this is clearer to the user... "server-side" is an implementation detail.
My understanding is that when this option is selected we ignore the time grain when computing the big number. If the grain is monthly, and the metric is `AVG(price)`, we would have:
- The "trendline" (which is not a trendline) would show a timeseries of `AVG(price)` per month.
- The big number would show the overall `AVG(price)`, from start to end of the data.
So I think renaming this to "Overall value" would make it clearer.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34296:
URL: https://github.com/apache/superset/pull/34296#discussion_r2232218830
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -41,6 +41,53 @@ import {
import { checkColumnType } from '../utils/checkColumnType';
import { isSortable } from '../utils/isSortable';
+// Aggregation choices with computation methods for plugins and controls
+export const aggregationChoices = {
+ raw: {
+ label: 'Force server-side aggregation',
Review Comment:
yeah though call on naming this properly... clearly we can expand on the tooltip and put much more information and can be a bit more technical there too. The feature is funky for a variety of reasons, one is that you have to pick 2 aggregations (one one the time grain, and one across time - sometimes across the raw data, sometimes on the time series itself), and many combinations might not make sense. LAST_VALUE is the most intuitive to me (and the default).
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34296:
URL: https://github.com/apache/superset/pull/34296#discussion_r2240179337
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -41,6 +41,53 @@ import {
import { checkColumnType } from '../utils/checkColumnType';
import { isSortable } from '../utils/isSortable';
+// Aggregation choices with computation methods for plugins and controls
+export const aggregationChoices = {
+ raw: {
+ label: 'Force server-side aggregation',
Review Comment:
I would strongly prefer something else here, since the user shouldn't care if the aggregation is done on the server or on the client. In fact, one might argue that if the chart is slow a user might want to choose this "to make things faster since it forces the aggregation to run on the server".
A few suggestions:
- "Overall" or "overall value"
- "Whole time period"
- "Whole series"
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(big number with trendline): running 2 identical queries for no good reason [superset]
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on code in PR #34296:
URL: https://github.com/apache/superset/pull/34296#discussion_r2240543441
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -41,6 +41,53 @@ import {
import { checkColumnType } from '../utils/checkColumnType';
import { isSortable } from '../utils/isSortable';
+// Aggregation choices with computation methods for plugins and controls
+export const aggregationChoices = {
+ raw: {
+ label: 'Force server-side aggregation',
Review Comment:
I think "Overall value" is the clearest label to me
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org