fix(charts): apply resample before rolling window in post-processing pipeline#37987
fix(charts): apply resample before rolling window in post-processing pipeline#37987veeceey wants to merge 2 commits intoapache:masterfrom
Conversation
Code Review Agent Run #b46558Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
alexandrusoare
left a comment
There was a problem hiding this comment.
LGTM - can you also provide a screenshot of the After? I am curious of how it looks
|
Sure thing @alexandrusoare! I'll add an After screenshot shortly. |
|
Thanks for the approval @alexandrusoare! I don't currently have a local Superset dev environment set up to grab a screenshot since the changes are in the query builder layer (not visual). The fix ensures that when If you'd still like a visual, I can spin up the dev environment and show the SQL output before/after — let me know! |
Code Review Agent Run #3c327bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
…pipeline When both resampling and rolling window are used on time-series charts, the rolling calculation was applied before resampling. This meant that imputed/zero-filled values from resampling were not included in the rolling window, producing incorrect stepped graphs instead of smooth curves. Reorder the post-processing pipeline in Timeseries, MixedTimeseries, and BigNumberWithTrendline chart types so that resample runs before rolling window, matching the mathematically correct sequence. Closes apache#23072
… lint errors Replace `formulaSeries?.data` with `formulaSeries!.data` in type assertion contexts where optional chaining produces unsafe undefined values. The `formulaSeries` variable is already validated via `expect().toBeDefined()` before these lines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
21ad31a to
6c6d5da
Compare
Code Review Agent Run #65500dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
Good question! Since this is a backend fix to the post-processing pipeline order (applying resample before rolling window instead of after), the chart visually looks the same—it's the underlying computed values that change. The Before showed incorrect aggregation because rolling was applied to un-resampled data, and the After shows correct values with proper resampling applied first. The data values in the table should be different, but the chart rendering itself is identical. Happy to add a data value comparison if that would be helpful! |
SUMMARY
When using both resampling (e.g., "1 calendar day frequency" with zero imputation) and a rolling window function (e.g., rolling mean) on time-series charts, the results come out wrong — you get a stepped graph alternating between the original values and zero instead of a smooth rolling curve.
The root cause is that the rolling window operator was being applied before the resample operator in the post-processing pipeline. This means the rolling calculation runs on the sparse original data, and then resampling fills in zeros afterward — which defeats the purpose. The correct order is to resample first (filling gaps with zeros or other imputation), and then apply the rolling window over that complete series.
This swaps the order in all three affected chart types:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before (from issue report): stepped graph with values jumping between 5 and 0

Expected (smooth rolling mean curve):

TESTING INSTRUCTIONS
ADDITIONAL INFORMATION