feat: Don't require data to be sorted by by column in rolling_*_by operations

[MarcoGorelli]

Needs rebasing onto #16247

In summary this splits rolling_apply_agg_window into:

• rolling_apply_agg_window_sorted (no difference to the current rolling_apply_agg_window, this is the fastpath when data is already known to be sorted by by)
• rolling_apply_agg_window: doesn't require the data to be sorted by by, which is why it needs an extra argument (sorting_indices) which is used to place the results in the right place of the out Vec
• deprecated warn_if_unsorted on the Python side, removed it from the Rust side

The gist of the change is rolling_apply_agg_window in crates/polars-time/src/chunkedarray/rolling_window/rolling_kernels/no_nulls.rs

Perf impact:

• for data already known to be sorted: none
• for data not known to be sorted, an extra arg_sort is performed in order to create sorting_indices. Once this has been created, it's just used to place the elements in the out Vec in the right place (rolling_apply_agg_window)

[codecov]

Codecov Report

Attention: Patch coverage is 86.84211% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 80.78%. Comparing base (98a2d9b) to head (7326da2).
Report is 10 commits behind head on main.

Files	Patch %	Lines
py-polars/polars/expr/expr.py	12.50%	7 Missing and 7 partials ⚠️
...s-time/src/chunkedarray/rolling_window/dispatch.rs	80.39%	10 Missing ⚠️
...edarray/rolling_window/rolling_kernels/no_nulls.rs	96.27%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16249      +/-   ##
==========================================
- Coverage   80.82%   80.78%   -0.04%     
==========================================
  Files        1393     1393              
  Lines      179341   179517     +176     
  Branches     2918     2929      +11     
==========================================
+ Hits       144955   145031      +76     
- Misses      33884    33976      +92     
- Partials      502      510       +8     

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

[MarcoGorelli]

Hi @orlp - I've tried using zeroed_vec here, and creating a validity mask separately. Any feedback on how I've done this would be appreciated if possible, thanks 🙏

[ritchie46]

Should this allocate this much?

[MarcoGorelli]

nope, just pushing an update

[MarcoGorelli]

Just chatted with Orson about this

The difficulty is that it's not known in advance (before starting the loop) whether there will be nulls
He's suggested making an Option<MutableBitmap>, which gets set on the first null

There is an extra if validity.is_none() check, but nulls produced by the rolling operation should be fairly rare anyway

I've pushed an update - thanks @orlp, much appreciated 🙏

[ritchie46]

Maybe put this in a closure as that ensure we have a single code block and call that closure twice.

I think the overhead of the function call is not too much here as you said, you don't expect many nulls.

[ritchie46]

Thanks for this one @MarcoGorelli. I do agree it is an improvement to answer the query. :)

[MarcoGorelli]

yay, thank you so much, and thanks @orlp for helping me - been wanting to see this happen for a while 🥳 I think users will definitely appreciate it