Skip to content

Remove lists::drop_list_duplicates#11236

Merged
rapids-bot[bot] merged 51 commits intorapidsai:branch-22.08from
ttnghia:deprecate_drop_list_duplicates
Jul 22, 2022
Merged

Remove lists::drop_list_duplicates#11236
rapids-bot[bot] merged 51 commits intorapidsai:branch-22.08from
ttnghia:deprecate_drop_list_duplicates

Conversation

@ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Jul 11, 2022 •
edited
Loading

This PR completely removes cudf::lists::drop_list_duplicates. It is replaced by the new API cudf::list::distinct which has a simpler implementation but better performance. The replacements for internal cudf usage have all been merged before thus there is no side effect or breaking for the existing APIs in this work.

Closes #11114, #11093, #11053, #11034, and closes #9257.

Depends on:

ttnghia added 30 commits June 24, 2022 14:51
@ttnghia
Add new implementation and test files
1d7e8e0
@ttnghia
Fix compile error
51b80db
@ttnghia
Rename function
08a76ad
@ttnghia
Implement cudf::detail::stable_distinct and lists::distinct
16101f7
@ttnghia
Rewrite doxygen
5ec13d6
@ttnghia
Rename variable
6c5b738
@ttnghia
Rewrite comment
5b70eee
@ttnghia
Rename files
238248d
@ttnghia
Implement float tests
ba6bf6b
@ttnghia
Implement string tests
3845c95
@ttnghia
Implement tests for ListDistinctTypedTest
507c82d
@ttnghia
Complete the remaining tests
2cb8347
@ttnghia
Merge branch 'branch-22.08' into add_lists_distinct
7efdea0
@ttnghia
Rewrite doxygen
4388637
@ttnghia
Misc
4dd5e74
@ttnghia
Misc
3b0760c
@ttnghia
Rewrite test
9730b70
@ttnghia
Fix doxygen
9bd9b6f
@ttnghia
Fix header
790a482
@ttnghia
Rewrite doxygen
1c58baa
@ttnghia
Rewrite doxygen and fix headers
d493c4f
@ttnghia
Fix iterator type
d090d2a
@ttnghia
Rewrite doxygen
ee51822
@ttnghia
Add empty lines
ccdd6f0
@ttnghia
Merge branch 'branch-22.08' into add_lists_distinct
034ee2a
@ttnghia
Update default stream
b1231a2
@ttnghia
Merge branch 'branch-22.08' into add_lists_distinct
af91b80
@ttnghia
Merge branch 'branch-22.08' into add_lists_distinct
86c9ba8
@ttnghia
Handle empty input
99d70b1
@ttnghia
Merge branch 'add_lists_distinct' into refactor_collect_set
cf965f6
This was linked to issues Jul 14, 2022
[FEA] Unify lists/drop_list_duplicates.hpp header into lists/stream_compaction.hpp #11034
Closed
[FEA] Implement lists::drop_list_duplicates using cudf::distinct #11053
Closed
[FEA] Fully support nested types in lists::drop_list_duplicates #11093
Closed
@ttnghia
Merge branch 'branch-22.08' into deprecate_drop_list_duplicates
c1d05d9 
# Conflicts:
#	cpp/include/cudf/lists/drop_list_duplicates.hpp
#	cpp/src/groupby/sort/aggregate.cpp
#	cpp/src/reductions/collect_ops.cu
#	cpp/tests/groupby/collect_set_tests.cpp
#	cpp/tests/groupby/merge_sets_tests.cpp
#	cpp/tests/reductions/collect_ops_tests.cpp
#	java/src/main/native/src/ColumnViewJni.cpp
#	java/src/main/native/src/ColumnViewJni.cu
#	java/src/main/native/src/ColumnViewJni.hpp
@github-actions github-actions bot removed Java Affects Java cuDF API. Python Affects Python cuDF API. labels Jul 19, 2022
@ttnghia ttnghia added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress 0 - Blocked Cannot progress due to external reasons labels Jul 19, 2022
@ttnghia ttnghia marked this pull request as ready for review July 19, 2022 02:47
@ttnghia ttnghia requested review from a team as code owners July 19, 2022 02:47
@ttnghia ttnghia requested review from karthikeyann and vuule July 19, 2022 02:47
@codecov
Copy link

codecov bot commented Jul 19, 2022 •
edited
Loading

Codecov Report

Merging #11236 (c1d05d9) into branch-22.08 (b2dd1bf) will increase coverage by 0.03%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.08   #11236      +/-   ##
================================================
+ Coverage         86.34%   86.37%   +0.03%     
================================================
  Files               144      144              
  Lines             22826    22826              
================================================
+ Hits              19708    19715       +7     
+ Misses             3118     3111       -7
Impacted Files Coverage Δ
python/cudf/cudf/core/dataframe.py 93.57% <0.00%> (+0.04%) ⬆️
python/cudf/cudf/core/column/string.py 88.80% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.02% <0.00%> (+0.21%) ⬆️
python/cudf/cudf/core/column/numerical.py 96.19% <0.00%> (+0.29%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 91.70% <0.00%> (+0.97%) ⬆️

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 b2dd1bf...c1d05d9. Read the comment docs.

robertmaynard
robertmaynard approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMake changes approved

@vuule
Copy link
Contributor

vuule commented Jul 21, 2022

Should the PR be named "Remove ..."? Deprecation means that the API is still available.

@bdice bdice mentioned this pull request Jul 22, 2022
Closed
@bdice bdice changed the title Deprecate lists::drop_list_duplicates Remove lists::drop_list_duplicates Jul 22, 2022
bdice
bdice approved these changes Jul 22, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. -1,741 lines of code is a great feeling.

@bdice
Copy link
Contributor

bdice commented Jul 22, 2022 •
edited
Loading

@ttnghia I think this can be merged when you're ready. You're more familiar with the state of all the linked issues than I am -- please close or comment on all the related issues once this is merged.

edit: Also, I fixed the labels -- PRs removing public APIs are always breaking.

@bdice bdice added breaking Breaking change and removed non-breaking Non-breaking change labels Jul 22, 2022
@ttnghia
Copy link
Contributor Author

ttnghia commented Jul 22, 2022

Thanks all for helping on this. I'm merging.

@ttnghia
Copy link
Contributor Author

ttnghia commented Jul 22, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a541ffb into rapidsai:branch-22.08 Jul 22, 2022
@ttnghia ttnghia deleted the deprecate_drop_list_duplicates branch July 25, 2022 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robertmaynard robertmaynard robertmaynard approved these changes

@bdice bdice bdice approved these changes

@jjacobelli jjacobelli jjacobelli approved these changes

@vuule vuule vuule approved these changes

@karthikeyann karthikeyann Awaiting requested review from karthikeyann karthikeyann is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees

@ttnghia ttnghia

Labels

3 - Ready for Review Ready for review by team breaking Breaking change CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.

Projects

None yet

Milestone

No milestone

5 participants

@ttnghia @vuule @bdice @robertmaynard @jjacobelli