Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor AggregationJni to support collectSet [skip ci] #8057

Merged
merged 9 commits into from
May 5, 2021
Merged

Refactor AggregationJni to support collectSet [skip ci] #8057

merged 9 commits into from
May 5, 2021

Conversation

sperlingxx
Copy link
Contributor

This pull request refactored AggregationJni to support COLLECT_SET as a kind of Aggregation (as well as COLLECT_LIST).

@sperlingxx
refactor AggregationJni to suppport collectSet as well as collectList
a4d2ae2 
Signed-off-by: sperlingxx <lovedreamf@gmail.com>
@sperlingxx sperlingxx requested a review from a team as a code owner April 26, 2021 08:03
@github-actions github-actions bot added the Java Affects Java cuDF API. label Apr 26, 2021
@sperlingxx sperlingxx added the improvement Improvement / enhancement to an existing function label Apr 26, 2021
@sperlingxx sperlingxx changed the title Refactor AggregationJni to support collectSet Refactor AggregationJni to support collectSet [skip ci] Apr 26, 2021
@sperlingxx sperlingxx added the non-breaking Non-breaking change label Apr 26, 2021
@sperlingxx sperlingxx mentioned this pull request Apr 26, 2021
Closed
21 tasks
@codecov
Copy link

codecov bot commented Apr 26, 2021 •
edited
Loading

Codecov Report

Merging #8057 (3a86fa1) into branch-0.20 (51336df) will decrease coverage by 0.45%.
The diff coverage is 84.29%.

❗ Current head 3a86fa1 differs from pull request most recent head e1774ce. Consider uploading reports for the commit e1774ce to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.20    #8057      +/-   ##
===============================================
- Coverage        82.88%   82.43%   -0.46%     
===============================================
  Files              103      103              
  Lines            17668    17422     -246     
===============================================
- Hits             14645    14361     -284     
- Misses            3023     3061      +38
Impacted Files Coverage Δ
python/cudf/cudf/core/column/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/io/orc.py 86.80% <ø> (-0.10%) ⬇️
python/cudf/cudf/utils/cudautils.py 55.04% <25.00%> (-2.72%) ⬇️
python/cudf/cudf/utils/dtypes.py 81.87% <41.66%> (-1.57%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.41% <70.00%> (-0.02%) ⬇️
python/cudf/cudf/core/tools/datetimes.py 80.00% <74.07%> (-4.54%) ⬇️
python/cudf/cudf/core/column/column.py 88.48% <75.00%> (-0.17%) ⬇️
python/cudf/cudf/core/groupby/groupby.py 91.39% <76.92%> (-0.06%) ⬇️
python/cudf/cudf/core/column/decimal.py 90.08% <78.26%> (-2.85%) ⬇️
python/dask_cudf/dask_cudf/backends.py 89.20% <80.00%> (-0.38%) ⬇️
... and 63 more

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 44f21b3...e1774ce. Read the comment docs.

sperlingxx and others added 2 commits April 27, 2021 17:07
@sperlingxx @ttnghia
Update java/src/main/java/ai/rapids/cudf/Aggregation.java
2944c47 
Co-authored-by: Nghia Truong <ttnghia@users.noreply.github.com>
@sperlingxx
some refinements
fc62d71 
Signed-off-by: sperlingxx <lovedreamf@gmail.com>
@sperlingxx sperlingxx added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Apr 29, 2021
@sperlingxx
Copy link
Contributor Author

@jlowe Could you help to review this PR? It introduces a change on the mapping of AggregationKind, which suggested by Nghia.

@sperlingxx sperlingxx requested a review from jlowe April 29, 2021 01:29
jlowe
jlowe reviewed Apr 29, 2021
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

cc: @revans2 for visibility since this has some overlap with #8069

java/src/main/java/ai/rapids/cudf/Aggregation.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/Aggregation.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/Aggregation.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/Aggregation.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/Aggregation.java Outdated Show resolved Hide resolved
java/src/main/native/src/AggregationJni.cpp Outdated Show resolved Hide resolved
@sperlingxx
Revert "some refinements"
402003d 
This reverts commit fc62d71.
@sperlingxx
Merge remote-tracking branch 'origin/branch-0.20' into jni_collect_re…
63d49c2 
…factor
@sperlingxx
fix
c01275a 
Signed-off-by: sperlingxx <lovedreamf@gmail.com>
@sperlingxx
fix nit
c08c0ab 
Signed-off-by: sperlingxx <lovedreamf@gmail.com>
jlowe
jlowe reviewed Apr 30, 2021
View reviewed changes
java/src/main/native/src/AggregationJni.cpp
@@ -186,10 +186,10 @@ JNIEXPORT jlong JNICALL Java_ai_rapids_cudf_Aggregation_createLeadLagAgg(JNIEnv
std::unique_ptr<cudf::aggregation> ret;
Copy link
Member

Choose a reason for hiding this comment

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

There are some comments starting on line 84 of this file that should be updated per the new enumeration values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks for pointing out!

java/src/main/java/ai/rapids/cudf/Aggregation.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/Aggregation.java Outdated Show resolved Hide resolved
jlowe
jlowe approved these changes Apr 30, 2021
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Only small nits remain, otherwise this looks good to me.

@sperlingxx
fix nit
0d441b1 
Signed-off-by: sperlingxx <lovedreamf@gmail.com>
@sperlingxx
Merge remote-tracking branch 'origin/branch-0.20' into jni_collect_re…
e1774ce 
…factor
@sperlingxx
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4715c83 into rapidsai:branch-0.20 May 5, 2021
@sperlingxx sperlingxx deleted the jni_collect_refactor branch May 5, 2021 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ttnghia ttnghia ttnghia approved these changes

@jlowe jlowe jlowe approved these changes

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sperlingxx @jlowe @ttnghia