fix(utils): fix off-by-one error in how rolling window's min_periods truncates dataframe

[sfirke]

SUMMARY

In Advanced Analytics, if the Min Periods value is 5, the first row in the resulting df_rolling with a valid result will be row 4 (since counting starts at 0). Right now the line df_rolling = df_rolling[min_periods:] truncates rows 0-4 -- that is, five rows -- throwing out the first rolling value that satisfies the min_periods requirement.

Currently the tooltip for Min Periods is misleading, this will make it true.

SCREENSHOTS

Here in Superset 3.1.1 you can see that the 5th bar should be eligible for a rolling average since Min Periods is 5. But it's only beginning on the 6th bar that the line appears.

When I tell Superset to use 5-Period averages for the rolling window and min of 4 periods, this truncates the first 4 rows (due to the buggy off-by-one I'm fixing) and serves what I wanted. 27.4 is indeed the average of the first 5 periods and a valid result.

TESTING INSTRUCTIONS

Build a chart with rolling average in an ephemeral environment.

[sfirke]

I see I will have to update some tests. This is my first time working with testing in Python so may take me a bit. I still think this is the correct PR. Update: I changed the test. It no longer tests for emptiness, but tests that the right number of rows are truncated which I believe is the underlying thing that the test was checking.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.71%. Comparing base (d2f7dec) to head (a30b7ce).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27388      +/-   ##
==========================================
+ Coverage   67.34%   69.71%   +2.37%     
==========================================
  Files        1909     1909              
  Lines       74592    74592              
  Branches     8320     8320              
==========================================
+ Hits        50235    52003    +1768     
+ Misses      22305    20537    -1768     
  Partials     2052     2052              

Flag	Coverage Δ
hive	53.77% <0.00%> (?)
mysql	77.96% <0.00%> (-0.03%)	⬇️
postgres	78.08% <0.00%> (ø)
presto	53.72% <0.00%> (?)
python	83.13% <100.00%> (+4.91%)	⬆️
sqlite	77.60% <0.00%> (ø)
unit	56.51% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sfirke]

@michael-s-molina maybe a candidate for 4.0.0rc2 if there's time? It is definitely a fix IMO, but it will change how certain charts display. If people were adapted to the buggy behavior and were specifying N-1 for min periods as a workaround to actually get N to display, it could cause an unintended data point to display. So bundling with a major release would be convenient.

Not critical though and IMO could be merged into master after 4.0.0 without waiting for a breaking window.

[rusackas]

Merging, as I think this is a fix. If people start complaining, we can revert it. If anyone disagrees, I can revert it now.

CC @sadpandajoe @Vitor-Avila FYI