NVIDIA / NVIDIA/cudf

[QST] Changing `cudf::rolling_window` API to have exclusive `preceding` parameter

Open
#7,093 12 comments 0 reactions 0 assignees View on GitHub
libcudf question
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

**Should we change `cudf::rolling_window` API so that the `preceding` parameter is exclusive?**

This is the current API for `cudf::rolling_window`:
```cpp
std::unique_ptr rolling_window(
column_view const& input,
size_type preceding_window,
size_type following_window,
size_type min_periods,
std::unique_ptr const& agg,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());
```

The question is whether `preceding` should be exclusive or inclusive. Current examples of behaviour:
```cpp
// auto col = {1, 2, 3, 4, 5};
cudf::rolling_window(col, 2, 1, 1, ???); // gets you [1, 2], [1, 2, 3], [2, 3, 4], [3, 4, 5], [4, 5]
cudf::rolling_window(col, 2, 1, 1, min); // 1, 1, 2, 3, 4
cudf::rolling_window(col, 2, 1, 1, max); // 2, 3, 4, 5, 5
cudf::rolling_window(col, 2, 1, 1, sum); // 3, 6, 9, 12, 9

// auto col = {1, 2, 3, 4, 5};
cudf::rolling_window(col, 1, 1, 1, ???); // gets you [1, 2], [2, 3], [3, 4], [4, 5], [5]
cudf::rolling_window(col, 1, 1, 1, min); // 1, 2, 3, 4, 5
cudf::rolling_window(col, 1, 1, 1, max); // 2, 3, 4, 5, 5
cudf::rolling_window(col, 1, 1, 1, sum); // 3, 5, 7, 9, 5

// auto col = {1, 2, 3, 4, 5};
cudf::rolling_window(col, 1, 0, 1, ???); // gets you [1], [2], [3], [4], [5]
cudf::rolling_window(col, 1, 0, 1, min); // 1, 2, 3, 4, 5
cudf::rolling_window(col, 1, 0, 1, max); // 1, 2, 3, 4, 5
cudf::rolling_window(col, 1, 0, 1, sum); // 1, 2, 3, 4, 5
```
Recommendation would be that all of the `preceding` parameters we be "reduce by 1" and the current index would be included by default. Therefore window length would always be `preceding` + `following` + 1. However, need to consider the API that takes a column of window sizes and also the future changes @mythrocks will make.

**Previous discussion:**
* https://github.com/rapidsai/cudf/pull/3305#discussion_r352659894
* https://github.com/rapidsai/cudf/pull/3305#discussion_r353878021

**Relevant SQL Links:**
* https://docs.microsoft.com/en-us/sql/t-sql/queries/select-over-clause-transact-sql?view=sql-server-ver15
* https://www.red-gate.com/simple-talk/sql/learn-sql-server/window-functions-in-sql-server-part-2-the-frame/

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.