Support `nth_element` for window functions #11158

mythrocks · 2022-06-27T23:24:24Z

This is to address spark-rapids/issues/4005 and
spark-rapids/issues/5061.

This change adds support for NTH_ELEMENT window aggregations.
This should allow for the implementation of FIRST(), LAST(),
and NTH_VALUE() window functions in Spark SQL.

NTH_ELEMENT in window function returns the Nth element from the
specified window for each row in a column. N is deemed to be
zero based, so NTH_ELEMENT(0) translates to the first element
in a window. Similarly, NTH_ELEMENT(-1) translates to the last.

If for any window of size W, if the specified N falls outside
the range [ -W, W-1 ], a null element is returned for that row.

This is to address spark-rapids/issues/4005 and spark-rapids/issues/5061. This change adds support for `NTH_ELEMENT` window aggregations. This should allow for the implementation of `FIRST()`, `LAST()`, and `NTH_VALUE()` window functions in Spark SQL. `NTH_ELEMENT` in window function returns the Nth element from the specified window for each row in a column. `N` is deemed to be zero based, so `NTH_ELEMENT(0)` translates to the first element in a window. Similarly, `NTH_ELEMENT(-1)` translates to the last. If for any window of size `W`, if the specified `N` falls outside the range `[ -W, W-1 ]`, a null element is returned for that row.

codecov · 2022-06-28T02:09:59Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.08@b5a59cf). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.08   #11158   +/-   ##
===============================================
  Coverage                ?   86.30%           
===============================================
  Files                   ?      144           
  Lines                   ?    22698           
  Branches                ?        0           
===============================================
  Hits                    ?    19589           
  Misses                  ?     3109           
  Partials                ?        0

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 b5a59cf...53ef97f. Read the comment docs.

cpp/src/rolling/nth_element.cuh

cpp/src/rolling/rolling.cu

cpp/src/rolling/rolling_collect_list.cu

davidwendt · 2022-06-29T14:45:11Z

Looks like this more than doubles the compile time for rolling.cu.
https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci/job/cudf/job/prb/job/cudf-cpu-cuda-build/CUDA=11.5/10413/Build_20Metrics_20Report/
Could this be broken up into separate source files perhaps?

mythrocks · 2022-06-30T16:17:46Z

Could this be broken up into separate source files perhaps?

The snag is that this is a function template, whose type parameters are transform iterators. I'll take a crack at this.

More readable.

This commit adds the JNI bindings required to invoke `NTH_ELEMENT` aggregations as a window aggregation. Java tests are included. Depends on rapidsai#11158.

mythrocks · 2022-07-06T17:50:40Z

@davidwendt, @ttnghia, thank you for the prompt reviews. I think I've addressed your concerns here.
I'll give it a moment to rerun tests.

vyasr

CMake LGTM.

I was a bit taken aback by the _detail named files, but it looks like that's not unique to this PR. We should probably be more consistent about using a detail/ directory, even in src/, rather than putting it in the filename. But out of scope for this PR.

cpp/src/rolling/rolling_detail.hpp

cpp/src/rolling/nth_element.cuh

1. Using #pragma once. 2. Doxygen format. 3. Renamed gather map functor. 4. Materialized gather indices.

mythrocks · 2022-07-06T23:14:59Z

CMake LGTM.

I was a bit taken aback by the _detail named files, but it looks like that's not unique to this PR. We should probably be more consistent about using a detail/ directory, even in src/, rather than putting it in the filename. But out of scope for this PR.

I started with making this change in this PR, but I now agree that this is peripheral to the current change. I've filed #11211 to track this.

To clarify: The changes required here would end up moving some of the files with substantial changes, and distract from the main change here. I'll address this after the current change has been merged.

cpp/src/rolling/nth_element.cuh

ttnghia

Just one more minor issue. Otherwise it's good.

cpp/src/rolling/nth_element.cuh

Also, switched to use gathered.front().

mythrocks · 2022-07-07T21:40:39Z

Thank you for the reviews, @davidwendt, @ttnghia, @vyasr. I'll merge this now.

mythrocks · 2022-07-07T21:41:56Z

@gpucibot merge

Depends on #11158. This commit adds the JNI bindings required to invoke `NTH_ELEMENT` aggregations as a window aggregation. Java tests are included. Authors: - MithunR (https://github.com/mythrocks) Approvers: - Ryan Lee (https://github.com/rwlee) - Raza Jafri (https://github.com/razajafri) URL: #11201

mythrocks requested a review from a team as a code owner June 27, 2022 23:24

mythrocks requested review from cwharris and davidwendt June 27, 2022 23:24

github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Jun 27, 2022

mythrocks self-assigned this Jun 27, 2022

mythrocks added feature request New feature or request non-breaking Non-breaking change labels Jun 27, 2022

mythrocks force-pushed the window-first-last branch from b6295fe to 7d02895 Compare June 27, 2022 23:52

ttnghia reviewed Jun 28, 2022

View reviewed changes

cpp/src/rolling/nth_element.cuh Outdated Show resolved Hide resolved

ttnghia reviewed Jun 28, 2022

View reviewed changes

cpp/src/rolling/nth_element.cuh Show resolved Hide resolved

ttnghia reviewed Jun 28, 2022

View reviewed changes

cpp/src/rolling/nth_element.cuh Outdated Show resolved Hide resolved

ttnghia reviewed Jun 28, 2022

View reviewed changes

cpp/src/rolling/rolling.cu Outdated Show resolved Hide resolved

ttnghia reviewed Jun 28, 2022

View reviewed changes

cpp/src/rolling/rolling_collect_list.cu Outdated Show resolved Hide resolved

mythrocks added 3 commits July 1, 2022 16:15

Added newline at the end of the file.

e53a589

Moved detail functions to their own TUs.

5257a01

Switched to offset_type.

0a03036

mythrocks requested a review from a team as a code owner July 1, 2022 23:50

mythrocks added 2 commits July 5, 2022 08:44

Fixed copyright year.

f5d8e71

Switched to use functor.

29708e2

More readable.

mythrocks requested a review from ttnghia July 5, 2022 19:51

mythrocks added a commit to mythrocks/cudf that referenced this pull request Jul 5, 2022

JNI bindings for NTH_ELEMENT window agg

abd0fd2

This commit adds the JNI bindings required to invoke `NTH_ELEMENT` aggregations as a window aggregation. Java tests are included. Depends on rapidsai#11158.

mythrocks mentioned this pull request Jul 5, 2022

JNI bindings for NTH_ELEMENT window aggregation #11201

Merged

vyasr approved these changes Jul 6, 2022

View reviewed changes

davidwendt reviewed Jul 6, 2022

View reviewed changes

cpp/src/rolling/rolling_detail.hpp Show resolved Hide resolved

davidwendt reviewed Jul 6, 2022

View reviewed changes

cpp/src/rolling/nth_element.cuh Outdated Show resolved Hide resolved

ttnghia reviewed Jul 6, 2022

View reviewed changes

cpp/src/rolling/nth_element.cuh Outdated Show resolved Hide resolved

ttnghia reviewed Jul 6, 2022

View reviewed changes

cpp/src/rolling/nth_element.cuh Outdated Show resolved Hide resolved

Review changes:

8605b96

1. Using #pragma once. 2. Doxygen format. 3. Renamed gather map functor. 4. Materialized gather indices.

mythrocks requested review from davidwendt and ttnghia July 6, 2022 23:26

davidwendt approved these changes Jul 7, 2022

View reviewed changes

ttnghia reviewed Jul 7, 2022

View reviewed changes

cpp/src/rolling/nth_element.cuh Outdated Show resolved Hide resolved

ttnghia approved these changes Jul 7, 2022

View reviewed changes

davidwendt reviewed Jul 7, 2022

View reviewed changes

cpp/src/rolling/nth_element.cuh Outdated Show resolved Hide resolved

Removed unused mr parameter.

d664b92

davidwendt reviewed Jul 7, 2022

View reviewed changes

cpp/src/rolling/nth_element.cuh Outdated Show resolved Hide resolved

Switched to non-template detail::gather().

53ef97f

Also, switched to use gathered.front().

rapids-bot bot merged commit cdd4e03 into rapidsai:branch-22.08 Jul 7, 2022

vyasr mentioned this pull request Jul 7, 2022

[BUG] Move _detail in rolling/ to rolling/detail #11211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `nth_element` for window functions #11158

Support `nth_element` for window functions #11158

mythrocks commented Jun 27, 2022

codecov bot commented Jun 28, 2022 •

edited

Loading

davidwendt commented Jun 29, 2022

mythrocks commented Jun 30, 2022

mythrocks commented Jul 6, 2022

vyasr left a comment

mythrocks commented Jul 6, 2022 •

edited

Loading

ttnghia left a comment

mythrocks commented Jul 7, 2022

mythrocks commented Jul 7, 2022

Support nth_element for window functions #11158

Support nth_element for window functions #11158

Conversation

mythrocks commented Jun 27, 2022

codecov bot commented Jun 28, 2022 • edited Loading

Codecov Report

davidwendt commented Jun 29, 2022

mythrocks commented Jun 30, 2022

mythrocks commented Jul 6, 2022

vyasr left a comment

Choose a reason for hiding this comment

mythrocks commented Jul 6, 2022 • edited Loading

ttnghia left a comment

Choose a reason for hiding this comment

mythrocks commented Jul 7, 2022

mythrocks commented Jul 7, 2022

Support `nth_element` for window functions #11158

Support `nth_element` for window functions #11158

codecov bot commented Jun 28, 2022 •

edited

Loading

mythrocks commented Jul 6, 2022 •

edited

Loading