Implement COLLECT rolling window aggregation #7189
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No min-periods check yet.
Also, fixed null/empty list representation.
... at the beginning of the output.
mythrocks
added
3 - Ready for Review
|
There are now 4 fairly large tests that pertain to
I intended to combine them all under the |
mythrocks
commented
Jan 22, 2021
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #7189 +/- ##
===============================================
+ Coverage 82.09% 82.19% +0.09%
===============================================
Files 97 99 +2
Lines 16474 16813 +339
===============================================
+ Hits 13524 13819 +295
- Misses 2950 2994 +44
Continue to review full report at Codecov.
|
12 tasks
vuule
approved these changes
Jan 29, 2021
rgsl888prabhu
approved these changes
Jan 29, 2021
mythrocks
added
the
5 - Ready to Merge
|
Thank you for the reviews, @vuule, @rgsl888prabhu! 👏 |
kkraus14
removed
3 - Ready for Review
|
@gpucibot merge |
|
I have filed #7258 for optional null-filtering, to support Spark use-cases. |
6 tasks
This was referenced Feb 1, 2021
rapids-bot bot
pushed a commit
that referenced
this pull request
Feb 4, 2021
Fixes #7265. `cudf::detail::get_num_child_rows()` is currently defined in `cudf/lists/detail/utilities.cuh`. The build pipelines for #7189 are fine, but there seem to be build failures in dependent projects such as `spark-rapids`: ``` [2021-01-31T08:12:10.611Z] /.../workspace/spark/cudf18_nightly/cpp/include/cudf/lists/detail/utilities.cuh:31:18: error: 'cudf::size_type cudf::detail::get_num_child_rows(const cudf::column_view&, rmm::cuda_stream_view)' defined but not used [-Werror=unused-function] [2021-01-31T08:12:10.611Z] static cudf::size_type get_num_child_rows(cudf::column_view const& list_offsets, [2021-01-31T08:12:10.611Z] ^~~~~~~~~~~~~~~~~~ [2021-01-31T08:12:11.981Z] cc1plus: all warnings being treated as errors [2021-01-31T08:12:12.238Z] make[2]: *** [CMakeFiles/cudf_hash.dir/build.make:82: CMakeFiles/cudf_hash.dir/src/hash/hashing.cu.o] Error 1 [2021-01-31T08:12:12.238Z] make[1]: *** [CMakeFiles/Makefile2:220: CMakeFiles/cudf_hash.dir/all] Error 2 ``` In any case, it is less than ideal for the function to be completely defined in the header, especially given that the likes of `hashing.cu` are exposed to it (by way of `scatter.cuh`). This commit moves the function definition to a separate translation unit, without changing implementation or interface. Authors: - MithunR (@mythrocks) Approvers: - @nvdbaranec - Mike Wilson (@hyperbolic2346) - David (@davidwendt) URL: #7266
rapids-bot bot
pushed a commit
that referenced
this pull request
Feb 4, 2021
Add unit tests for aggregate 'collect' with windowing. This PR depends on the PR #7189 . Signed-off-by: Liangcai Li <liangcail@nvidia.com> Authors: - Liangcai Li (@firestarman) Approvers: - MithunR (@mythrocks) - Robert (Bobby) Evans (@revans2) URL: #7121
rapids-bot bot
pushed a commit
that referenced
this pull request
Feb 18, 2021
Closes #7258. #7189 implements `COLLECT` aggregations to be done from window functions. The semantics of how null input rows are handled are consistent with CUDF semantics. E.g. ```c++ auto input_col = fixed_width_column_wrapper<int32_t>{70, ∅, 72, 73, 74}; auto output_col = cudf::rolling_window(input_col, 2, 1, 1, collect_aggr); // == [ [70,∅], [70,∅,72], [∅,72,73], [72,73,74], [73,74] ] ``` Note that the null element (`∅`) is replicated in the first 3 rows of the output. SparkSQL (and Hive, and other big data SQL systems) have different semantics, in that all null elements are purged. The output for the same operation should yield the following: ```c++ auto sparkish_output_col = cudf::rolling_window(input_col, 2, 1, 1, collect_aggr); // == [ [70], [70,72], [72,73], [72,73,74], [73,74] ] ``` CUDF should allow the `COLLECT` aggregation to be constructed with an optional `null_policy` argument (with default `INCLUDE`). The `COLLECT` window function should check the policy, and filter out null list-elements _a posteriori_. Authors: - MithunR (@mythrocks) Approvers: - Ram (Ramakrishna Prabhu) (@rgsl888prabhu) - AJ Schmidt (@ajschmidt8) - Vukasin Milovanovic (@vuule) - Jake Hemstad (@jrhemstad) URL: #7264
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #7133.
This is an implementation of the
COLLECTaggregation in the context of rolling window functions. This enables the collection of rows (of typeT) within specified window boundaries into a list column (containing elements of typeT). In this context, one list row would be generated per input row. E.g. Consider the following example:Calling
rolling_window()withpreceding=2,following=1,min_periods=1produces the following:COLLECTis supported withrolling_window(),grouped_rolling_window(), andgrouped_time_range_rolling_window(), across primitive types and arbitrarily nested lists and structs.min_periodsis also honoured: If the number of observations is fewer than min_periods, the resulting list row is null.