Support collect_set on rolling window by sperlingxx · Pull Request #7881 · NVIDIA/cudf

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

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

…lling

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.

…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

+                                                     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

 * @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/cudf-spark#2062

Closed

21 tasks

ttnghia requested changes Apr 7, 2021

View reviewed changes

Comment thread cpp/src/rolling/rolling_detail.cuh Outdated

mythrocks reviewed Apr 7, 2021

View reviewed changes

Comment thread cpp/tests/rolling/collect_ops_test.cpp

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 mythrocks and ttnghia 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 NVIDIA:branch-21.06 May 26, 2021

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

Uh oh!

Conversation

sperlingxx commented Apr 7, 2021

Uh oh!

Uh oh!

mythrocks commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sperlingxx commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mythrocks commented May 10, 2021

Uh oh!

ttnghia commented May 25, 2021

Uh oh!

ttnghia May 25, 2021

Choose a reason for hiding this comment

Uh oh!

sperlingxx May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sperlingxx May 26, 2021

Choose a reason for hiding this comment

Uh oh!

ttnghia May 26, 2021

Choose a reason for hiding this comment

Uh oh!

ttnghia May 26, 2021

Choose a reason for hiding this comment

Uh oh!

sperlingxx May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttnghia May 26, 2021

Choose a reason for hiding this comment

Uh oh!

sperlingxx May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sperlingxx May 26, 2021

Choose a reason for hiding this comment

Uh oh!

ttnghia May 26, 2021

Choose a reason for hiding this comment

Uh oh!

ttnghia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sperlingxx commented May 26, 2021

Uh oh!

ttnghia commented May 26, 2021

Uh oh!

ttnghia commented May 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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