Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix: rolling and cum operator in multiple series chart #1386

Merged

Conversation

zhaoyongjie
Copy link
Contributor

@zhaoyongjie zhaoyongjie commented Oct 2, 2021

🐛 Bug Fix

Currently, Rolling Window calculation results are incorrect on multiple series.

Backendcode at: apache/superset#16945

@vercel
Copy link

vercel bot commented Oct 2, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/ACUSShdiPhDGegDuz6J1m6yU5hXZ
✅ Preview: https://superset-ui-git-fork-zhaoyongjie-fixrollingmult-f4a5c9-superset.vercel.app

@zhaoyongjie zhaoyongjie changed the title fix: rolling and cum operator in multiple series chart [WIP]fix: rolling and cum operator in multiple series chart Oct 2, 2021
@zhaoyongjie zhaoyongjie changed the title [WIP]fix: rolling and cum operator in multiple series chart fix: rolling and cum operator in multiple series chart Oct 2, 2021
@codecov
Copy link

codecov bot commented Oct 3, 2021

Codecov Report

Merging #1386 (1d1b705) into master (63c9ca6) will increase coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1386   +/-   ##
=======================================
  Coverage   30.34%   30.35%           
=======================================
  Files         497      497           
  Lines        9979     9983    +4     
  Branches     1680     1681    +1     
=======================================
+ Hits         3028     3030    +2     
- Misses       6706     6707    +1     
- Partials      245      246    +1     
Impacted Files Coverage Δ
...rt-controls/src/operators/rollingWindowOperator.ts 100.00% <ø> (ø)
.../plugin-chart-echarts/src/Timeseries/buildQuery.ts 50.00% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63c9ca6...1d1b705. Read the comment docs.

@zhaoyongjie zhaoyongjie marked this pull request as ready for review October 3, 2021 13:02
@zhaoyongjie zhaoyongjie requested a review from a team as a code owner October 3, 2021 13:02
Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@villebro
Copy link
Contributor

villebro commented Oct 6, 2021

One minor improvement proposal if you have the time: it would be nice to set isVisible to false on the the rolling_periods control when rolling_type is null. But this is totally optional - maybe we can do a separate improvement PR to do a similar thing to all advanced analytics controls.

@zhaoyongjie
Copy link
Contributor Author

One minor improvement proposal if you have the time: it would be nice to set isVisible to false on the the rolling_periods control when rolling_type is null. But this is totally optional - maybe we can do a separate improvement PR to do a similar thing to all advanced analytics controls.

This is necessary. I will create a separate PR.

@zhaoyongjie zhaoyongjie merged commit b572988 into apache-superset:master Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants