Support collect_set on rolling window #7881

sperlingxx · 2021-04-07T10:13:49Z

This pull request is to support collect_set on rolling window, which is required in #7809.

…lling

cpp/src/rolling/rolling_detail.cuh

mythrocks · 2021-04-07T15:18:49Z

Hey, there, @sperlingxx. This PR should be against branch-0.20, not branch-0.19. You have the option of picking these commits into a branch-0.20 checkout, and overwriting this branch.

Also, pardon me for pointing this out: The reason we postponed making this change was that we are piling on if-else checks in the SFINAE code. @nvdbaranec is in the middle of sorting out that refactor.

cpp/tests/rolling/collect_ops_test.cpp

…lling

codecov · 2021-04-19T14:47:36Z

Codecov Report

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

❗ Current head e146913 differs from pull request most recent head dca2bb6. Consider uploading reports for the commit dca2bb6 to get more accurate results

@@               Coverage Diff               @@
##             branch-21.06    #7881   +/-   ##
===============================================
  Coverage                ?   82.89%           
===============================================
  Files                   ?      105           
  Lines                   ?    17875           
  Branches                ?        0           
===============================================
  Hits                    ?    14817           
  Misses                  ?     3058           
  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 dd5eecd...dca2bb6. Read the comment docs.

…lling

Signed-off-by: sperlingxx <lovedreamf@gmail.com>

sperlingxx · 2021-05-10T05:02:28Z

Hi @ttnghia @mythrocks, I apologize for triggering re-requesting review by mistake. I didn't realize the refactor involving multiple PRs until I found #8158.

mythrocks · 2021-05-10T06:22:49Z

No worries, sir. We will prioritize this as soon as #8158 is in.

…olling

Signed-off-by: sperlingxx <lovedreamf@gmail.com>

ttnghia · 2021-05-25T23:20:51Z

Rerun tests.

ttnghia · 2021-05-25T23:58:58Z

cpp/src/rolling/rolling_detail.cuh

+                                                     min_periods,
+                                                     agg._null_handling,
+                                                     stream,
+                                                     mr);


collect_list will not be returned, thus we will not use mr. Instead, please use rmm::mr::get_current_device_resource().

Oh, my bad! I always forget that. Shall we keep the stream, or replacing it with rmm::cuda_stream_default?

I made this change.

All func calls should use the same stream so we will not change that.

ttnghia · 2021-05-26T01:04:20Z

cpp/include/cudf/detail/aggregation/aggregation.hpp

@@ -581,7 +581,7 @@ class collect_list_aggregation final : public rolling_aggregation {
 /**
 * @brief Derived aggregation class for specifying COLLECT_SET aggregation
 */
-class collect_set_aggregation final : public aggregation {
+class collect_set_aggregation final : public rolling_aggregation {


Why do we inherit collect set from rolling_aggregation instead of aggregation? We need it for both rolling window and groupby, don't we?

Because rolling_aggregation is virtually inherited from aggregation. I just followed corresponding codes for collect_list.

Maybe both are wrong, as collect_set_aggregation and collect_set_aggregation are used not only in rolling window but in groupby. Can you change to:

class collect_list_aggregation final : public aggregation ... class collect_set_aggregation final : public aggregation

and test if they can compile and unit tests all pass please?

Hi @ttnghia, I tried on replacing rolling_aggregation with aggregation. And I got compiling error on src/aggregation/aggregation.cpp:454:

/home/alfredxu/workspace/codes/cudf/cpp/src/aggregation/aggregation.cpp:454:74: error: could not convert ‘std::make_unique(_Args&& ...) [with _Tp = cudf::detail::collect_list_aggregation; _Args = {cudf::null_policy&}; typename std::_MakeUniq<_Tp>::__single_object = std::unique_ptr<cudf::detail::collect_list_aggregation, std::default_delete<cudf::detail::collect_list_aggregation> >]()’ from ‘unique_ptr<cudf::detail::collect_list_aggregation,default_delete<cudf::detail::collect_list_aggregation>>’ to ‘unique_ptr<cudf::rolling_aggregation,default_delete<cudf::rolling_aggregation>>’

/// Factory to create a COLLECT_LIST aggregation template <typename Base = aggregation> std::unique_ptr<Base> make_collect_list_aggregation(null_policy null_handling) { return std::make_unique<detail::collect_list_aggregation>(null_handling); } template std::unique_ptr<aggregation> make_collect_list_aggregation<aggregation>( null_policy null_handling); template std::unique_ptr<rolling_aggregation> make_collect_list_aggregation<rolling_aggregation>( null_policy null_handling);

I think it is because we can not return std::unique_ptr<rolling_aggregation> after we made the replacement.

I see. Thanks 😄

Signed-off-by: sperlingxx <lovedreamf@gmail.com>

ttnghia

LGTM. Now we have other blocker PRs (#8342 and rapidsai/dask-cuda#623). Wait for them merged first before we can rerun tests.

sperlingxx · 2021-05-26T13:30:14Z

rerun tests

ttnghia · 2021-05-26T16:53:34Z

Rerun tests.

ttnghia · 2021-05-26T19:22:01Z

@gpucibot merge

sperlingxx added 2 commits April 7, 2021 18:05

support collect_set on rolling

48745d4

Merge remote-tracking branch 'origin/branch-0.19' into collect_set_ro…

8946233

…lling

sperlingxx requested review from mythrocks and ttnghia April 7, 2021 10:13

sperlingxx requested a review from a team as a code owner April 7, 2021 10:13

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Apr 7, 2021

sperlingxx added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. labels Apr 7, 2021

fix clang style

34b5a28

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Apr 7, 2021

sperlingxx mentioned this pull request Apr 7, 2021

[FEA] support collect aggregations NVIDIA/spark-rapids#2062

Closed

21 tasks

ttnghia requested changes Apr 7, 2021

View reviewed changes

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

mythrocks reviewed Apr 7, 2021

View reviewed changes

cpp/tests/rolling/collect_ops_test.cpp Show resolved Hide resolved

sperlingxx added 2 commits April 12, 2021 17:16

Merge remote-tracking branch 'origin/branch-0.19' into collect_set_ro…

79cc6a0

…lling

some updates

055bf3a

sperlingxx requested a review from a team as a code owner April 19, 2021 10:07

github-actions bot added the CMake CMake build issue label Apr 19, 2021

sperlingxx changed the base branch from branch-0.19 to branch-0.20 April 19, 2021 10:08

robertmaynard approved these changes Apr 28, 2021

View reviewed changes

sperlingxx added 2 commits May 10, 2021 11:45

Merge remote-tracking branch 'origin/branch-0.20' into collect_set_ro…

c26ba70

…lling

some fix after merging with latest master

874732b

Signed-off-by: sperlingxx <lovedreamf@gmail.com>

sperlingxx requested review from ttnghia and mythrocks May 10, 2021 04:22

fix

8933345

sperlingxx added 5 commits May 25, 2021 16:02

Merge remote-tracking branch 'origin/branch-21.06' into collect_set_r…

9acbd15

…olling

adapt rolling_window refactor

50c1dea

Signed-off-by: sperlingxx <lovedreamf@gmail.com>

fix

747f0f9

Signed-off-by: sperlingxx <lovedreamf@gmail.com>

fix

5b5f452

Signed-off-by: sperlingxx <lovedreamf@gmail.com>

fix

3b2fdd5

Signed-off-by: sperlingxx <lovedreamf@gmail.com>

ttnghia requested changes May 26, 2021

View reviewed changes

small fix

dca2bb6

Signed-off-by: sperlingxx <lovedreamf@gmail.com>

ttnghia approved these changes May 26, 2021

View reviewed changes

rapids-bot bot merged commit be05a00 into rapidsai:branch-21.06 May 26, 2021

sperlingxx deleted the collect_set_rolling branch May 26, 2021 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support collect_set on rolling window #7881

Support collect_set on rolling window #7881

sperlingxx commented Apr 7, 2021

mythrocks commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 19, 2021 •

edited

Loading

sperlingxx commented May 10, 2021 •

edited

Loading

mythrocks commented May 10, 2021

ttnghia commented May 25, 2021

ttnghia May 25, 2021

sperlingxx May 26, 2021 •

edited

Loading

sperlingxx May 26, 2021

ttnghia May 26, 2021

ttnghia May 26, 2021

sperlingxx May 26, 2021 •

edited

Loading

ttnghia May 26, 2021

sperlingxx May 26, 2021 •

edited

Loading

sperlingxx May 26, 2021

ttnghia May 26, 2021

ttnghia left a comment •

edited

Loading

sperlingxx commented May 26, 2021

ttnghia commented May 26, 2021

ttnghia commented May 26, 2021

Support collect_set on rolling window #7881

Support collect_set on rolling window #7881

Conversation

sperlingxx commented Apr 7, 2021

mythrocks commented Apr 7, 2021 • edited Loading

codecov bot commented Apr 19, 2021 • edited Loading

Codecov Report

sperlingxx commented May 10, 2021 • edited Loading

mythrocks commented May 10, 2021

ttnghia commented May 25, 2021

ttnghia May 25, 2021

Choose a reason for hiding this comment

sperlingxx May 26, 2021 • edited Loading

Choose a reason for hiding this comment

sperlingxx May 26, 2021

Choose a reason for hiding this comment

ttnghia May 26, 2021

Choose a reason for hiding this comment

ttnghia May 26, 2021

Choose a reason for hiding this comment

sperlingxx May 26, 2021 • edited Loading

Choose a reason for hiding this comment

ttnghia May 26, 2021

Choose a reason for hiding this comment

sperlingxx May 26, 2021 • edited Loading

Choose a reason for hiding this comment

sperlingxx May 26, 2021

Choose a reason for hiding this comment

ttnghia May 26, 2021

Choose a reason for hiding this comment

ttnghia left a comment • edited Loading

Choose a reason for hiding this comment

sperlingxx commented May 26, 2021

ttnghia commented May 26, 2021

ttnghia commented May 26, 2021

mythrocks commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 19, 2021 •

edited

Loading

sperlingxx commented May 10, 2021 •

edited

Loading

sperlingxx May 26, 2021 •

edited

Loading

sperlingxx May 26, 2021 •

edited

Loading

sperlingxx May 26, 2021 •

edited

Loading

ttnghia left a comment •

edited

Loading