You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "LevisNgigi (via GitHub)" <gi...@apache.org> on 2025/05/12 09:14:42 UTC
[PR] fix(big number with trendline):Make aggregation optional. [superset]
LevisNgigi opened a new pull request, #33407:
URL: https://github.com/apache/superset/pull/33407
### SUMMARY
This PR improves the Big Number with Trendline chart by making the "Aggregation Method" optional. Previously, aggregation was always applied, even for non-additive metrics such as COUNT(DISTINCT ...) or ratio expressions, resulting in misleading or incorrect Big Numbers.
Now, when no aggregation method is selected, the Big Number query omits time-based grouping and computes the raw metric over the entire date range similar to the Big Number without trendline.
This allows correct visualization of non-additive metrics while retaining support for additive ones via the optional dropdown.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
### TESTING INSTRUCTIONS
1. Go to Explore and select the "Big Number with Trendline" chart.
2. Choose a non-additive metric (e.g. `COUNT(*) / COUNT(DISTINCT gender)`).
3. Set a time grain (e.g. monthly).
4. **Clear the "Aggregation Method" dropdown**.
5. Click "Run".
6. Verify that the Big Number displays a single raw value across the entire date range, and the trendline shows the correct per-period values.
Also verify:
- Additive metrics (e.g. SUM) work as before.
- Clearing the aggregation method disables post-processing and grouping.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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):Make aggregation optional. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #33407:
URL: https://github.com/apache/superset/pull/33407#discussion_r2084430777
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts:
##########
@@ -52,15 +52,19 @@
{
...baseQueryObject,
columns: [
- ...(isXAxisSet(formData)
- ? ensureIsArray(getXAxisColumn(formData))
+ ...(formData.aggregation
+ ? isXAxisSet(formData)
+ ? ensureIsArray(getXAxisColumn(formData))
+ : []
: []),
],
- ...(isXAxisSet(formData) ? {} : { is_timeseries: true }),
- post_processing: [
- pivotOperator(formData, baseQueryObject),
- aggregationOperator(formData, baseQueryObject),
- ],
+ is_timeseries: !!formData.aggregation,
+ post_processing: formData.aggregation
+ ? [
+ pivotOperator(formData, baseQueryObject),
+ aggregationOperator(formData, baseQueryObject),
+ ]
+ : [],
Review Comment:
Thanks! I tried separating pivotOperator, but without aggregation, the time column is removed, which causes pivot to fail. So I kept it conditional to avoid data errors when the time column isn’t present. Also, not all post-processing is skipped, only the aggregation step is skipped when no aggregation method is selected.
--
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): make aggregation optional. [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2892452171
Right. I'm not sure who else advised what before - and sorry for the confusion as we figure things out as we go and often navigate intricate tradeoffs. Maybe we tag whoever had a different view so we can hash it out here.
--
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): make aggregation optional. [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2887747079
Oh so there are some mechanics behind there that will run 2 queries? I know we do that in some special cases like maybe "show totals" in the Table visualization (?)
Now wondering if we should use this new option as the default of keep LAST_VALUE there (?) Seems generally users would expect LAST_VALUE, but I might not be the right person to decide since I probably built it the way it is today as it seems more intuitive 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
Re: [PR] fix(big number with trendline): make aggregation optional. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2887832628
> Oh so there are some mechanics behind there that will run 2 queries? I know we do that in some special cases like maybe "show totals" in the Table visualization (?)
>
> Now wondering if we should use this new option as the default of keep LAST_VALUE there (?) Seems generally users would expect LAST_VALUE, but I might not be the right person to decide since I probably built it the way it is today as it seems more intuitive to me...
Yes,this setup runs two queries: one for the trendline and another for the big number when no aggregation method is selected. With the new change making aggregation optional, we can't assign a different default like LAST_VALUE, since doing so would prevent users from clearing the selection and reverting to the raw metric. Leaving the control empty is now the explicit signal to compute the raw value across the entire time range.Yes,Last value was previously the default, I hope the new option is okay?
--
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): make aggregation optional. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2890608567
> > we can't assign a different default like LAST_VALUE
>
> is that right? seems it's totally doable to set anything as a default for the control, and use are able to clear the dropdown if they want. You'd have to pass `clearable` or whatever it's called to the component.
>
> Alternatively you could also introduce a new aggregation type for this new option you are creating. Not sure what exactly to call it, but giving it a clear name would make the new option and behavior more discoverable.
>
> I'm worried that changing the default to a new behavior is going to confuse users who are used to the current behavior (last_value).
Yes, I’ve been trying to set LAST_VALUE as the default while keeping the dropdown clearable, but there are still some odd behaviors when loading or saving charts with that setup,so I’m continuing to look into it.
I had actually introduced a "Raw Metric" option in the dropdown earlier, but was advised against it since it’s technically not an aggregation method. Including it there could be misleading, as selecting it means we skip aggregation entirely.But it worked well with the default value.
That said, I agree that discoverability is important, and the trade-off between clarity and correctness here is still something we’re balancing,especially since changing the default behavior from LAST_VALUE could be confusing to existing users.
--
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): make aggregation optional. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2893118697
> Right. I'm not sure who else advised what before - and sorry for the confusion as we figure things out as we go and often navigate intricate tradeoffs. Maybe we tag whoever had a different view so we can hash it out here.
No problem,I appreciate that we are navigating some intricate tradeoffs and I am happy with the discussion so far.Just to clarify, @betodealmeida had looked into the current behavior in depth and provided guidance on the most reliable and aligned way forward, based on that investigation.
--
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): make aggregation optional. [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2888576369
> we can't assign a different default like LAST_VALUE
is that right? seems it's totally doable to set anything as a default for the control, and use are able to clear the dropdown if they want.
Alternatively you could also introduce a new aggregation type for this new option you are creating. Not sure what exactly to call it, but giving it a clear name would make the new option and behavior more discoverable.
I'm worried that changing the default to a new behavior is going to confuse users who are used to the current behavior (last_value).
--
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): make aggregation optional. [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2899517514
@sadpandajoe Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/15174788733). 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): add None option to the aggregation method dropdown [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #33407:
URL: https://github.com/apache/superset/pull/33407
--
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):Make aggregation optional. [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33407:
URL: https://github.com/apache/superset/pull/33407#discussion_r2084224233
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts:
##########
@@ -52,15 +52,19 @@ export default function buildQuery(formData: QueryFormData) {
{
...baseQueryObject,
columns: [
- ...(isXAxisSet(formData)
- ? ensureIsArray(getXAxisColumn(formData))
+ ...(formData.aggregation
+ ? isXAxisSet(formData)
+ ? ensureIsArray(getXAxisColumn(formData))
+ : []
: []),
],
- ...(isXAxisSet(formData) ? {} : { is_timeseries: true }),
- post_processing: [
- pivotOperator(formData, baseQueryObject),
- aggregationOperator(formData, baseQueryObject),
- ],
+ is_timeseries: !!formData.aggregation,
Review Comment:
### Incorrect Time Series Flag Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The is_timeseries flag is incorrectly tied to the presence of aggregation, when it should be based on whether an X-axis is set (as in the first query object).
###### Why this matters
This could cause incorrect data processing for time series data when no aggregation is selected, potentially leading to visualization errors or data misrepresentation.
###### Suggested change ∙ *Feature Preview*
The is_timeseries flag should maintain the same logic as the first query object:
```typescript
...(isXAxisSet(formData) ? {} : { is_timeseries: true }),
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/526a4bc5-4e0c-409a-8916-83a32dae1c19/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/526a4bc5-4e0c-409a-8916-83a32dae1c19?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/526a4bc5-4e0c-409a-8916-83a32dae1c19?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/526a4bc5-4e0c-409a-8916-83a32dae1c19?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/526a4bc5-4e0c-409a-8916-83a32dae1c19)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:75dc8d09-d748-4e67-bf96-cce6394a188d -->
[](75dc8d09-d748-4e67-bf96-cce6394a188d)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts:
##########
@@ -52,15 +52,19 @@
{
...baseQueryObject,
columns: [
- ...(isXAxisSet(formData)
- ? ensureIsArray(getXAxisColumn(formData))
+ ...(formData.aggregation
+ ? isXAxisSet(formData)
+ ? ensureIsArray(getXAxisColumn(formData))
+ : []
: []),
],
- ...(isXAxisSet(formData) ? {} : { is_timeseries: true }),
- post_processing: [
- pivotOperator(formData, baseQueryObject),
- aggregationOperator(formData, baseQueryObject),
- ],
+ is_timeseries: !!formData.aggregation,
+ post_processing: formData.aggregation
+ ? [
+ pivotOperator(formData, baseQueryObject),
+ aggregationOperator(formData, baseQueryObject),
+ ]
+ : [],
Review Comment:
### Missing Essential Post-Processing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
When aggregation is disabled, all post-processing operations are skipped, including pivoting which might still be necessary for proper data transformation.
###### Why this matters
This could result in incorrectly structured data when pivoting is required but aggregation is not selected, leading to visualization failures.
###### Suggested change ∙ *Feature Preview*
Separate the pivot operation from the aggregation condition:
```typescript
post_processing: [
pivotOperator(formData, baseQueryObject),
...(formData.aggregation ? [aggregationOperator(formData, baseQueryObject)] : []),
],
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f1fe1b8-933f-4b7d-aad1-60a025293765/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f1fe1b8-933f-4b7d-aad1-60a025293765?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f1fe1b8-933f-4b7d-aad1-60a025293765?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f1fe1b8-933f-4b7d-aad1-60a025293765?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f1fe1b8-933f-4b7d-aad1-60a025293765)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:94b9b312-ed06-4d0d-9c84-4ddc8b957ff8 -->
[](94b9b312-ed06-4d0d-9c84-4ddc8b957ff8)
--
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): make aggregation optional. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2886104477
> Isn't this the same as `LAST_VALUE`? Trying to understand how it differs from it...
@mistercrunch LAST_VALUE returns the final value from a sorted result set. That’s fine, but for this we need the big number to display the raw metric value for non-additive metrics like ratios, or percentages.
In this case, we're not just interested in the last data point — we're trying to reflect the raw metric over the entire time range, as it exists in the source data.
--
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): make aggregation optional. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2887670269
> Oh so say for a `COUNT(DISTINCT user_id)`, if the length of the trendline is 30 days, it would show `DAU` on the trendline for overall count distinct over the period for the big number (MAU)? Doesn't that require running two queries? Wondering if/how that works on this PR as is (?)
As per my investigation and modification so far this PR follows that structure, so the trendline shows granular values (e.g., daily counts), and the big number reflects an aggregate (or raw) value across the selected period. It works as expected for non-additive metrics like COUNT(DISTINCT user_id) when no aggregation method is selected.
--
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): make aggregation optional. [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2887117734
Oh so say for a `COUNT(DISTINCT user_id)`, if the length of the trendline is 30 days, it would show `DAU` on the trendline for overall count distinct over the period for the big number (MAU)? Doesn't that require running two queries? Wondering if/how that works on this PR as is (?)
--
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):Make aggregation optional. [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33407:
URL: https://github.com/apache/superset/pull/33407#discussion_r2084431377
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts:
##########
@@ -52,15 +52,19 @@
{
...baseQueryObject,
columns: [
- ...(isXAxisSet(formData)
- ? ensureIsArray(getXAxisColumn(formData))
+ ...(formData.aggregation
+ ? isXAxisSet(formData)
+ ? ensureIsArray(getXAxisColumn(formData))
+ : []
: []),
],
- ...(isXAxisSet(formData) ? {} : { is_timeseries: true }),
- post_processing: [
- pivotOperator(formData, baseQueryObject),
- aggregationOperator(formData, baseQueryObject),
- ],
+ is_timeseries: !!formData.aggregation,
+ post_processing: formData.aggregation
+ ? [
+ pivotOperator(formData, baseQueryObject),
+ aggregationOperator(formData, baseQueryObject),
+ ]
+ : [],
Review Comment:
That makes sense - I understand now that pivot requires the time column which is only present with aggregation. Thanks for explaining the technical constraint.
--
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): make aggregation optional. [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2897231735
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/33407?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 65.73%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`767f893`)](https://app.codecov.io/gh/apache/superset/commit/767f89357f220cb9d32020fa6c757473a3b9b06d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1880 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #33407 +/- ##
==========================================
+ Coverage 60.48% 65.73% +5.24%
==========================================
Files 1931 553 -1378
Lines 76236 39998 -36238
Branches 8568 0 -8568
==========================================
- Hits 46114 26293 -19821
+ Misses 28017 13705 -14312
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/33407/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/33407/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.23% <ø> (-0.93%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/33407/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/33407/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.69% <ø> (-1.12%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/33407/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.73% <ø> (+2.23%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/33407/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.30% <ø> (+3.67%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/33407?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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): make aggregation optional. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2899006046
> > Right. I'm not sure who else advised what before - and sorry for the confusion as we figure things out as we go and often navigate intricate tradeoffs. Maybe we tag whoever had a different view so we can hash it out here.
>
> No problem,I appreciate that we are navigating some intricate tradeoffs and I am happy with the discussion so far.Just to clarify, @betodealmeida had looked into the current behavior in depth and provided guidance on the most reliable and aligned way forward, based on that investigation.
I am wondering if the option to add 'None' in the dropdwon would be the best case scenario with everything considered.
--
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): make aggregation optional. [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33407:
URL: https://github.com/apache/superset/pull/33407#issuecomment-2899531483
@sadpandajoe Ephemeral environment spinning up at http://54.189.163.207: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