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.

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

JNI: Support skipping nulls for collect aggregation [skip ci] #7457

Merged
merged 3 commits into from
Mar 5, 2021

Conversation

firestarman
Copy link
Contributor

@firestarman firestarman commented Feb 26, 2021

This PR is to support skipping nulls for collect aggregation in JVM by creating a new class CollectAggregation who accepts a NullPolicy argument indicating whether to include nulls.

Skipping nulls has already been supported by collect aggregation with rolling in native (#7264), so this PR just exposes the feaure in JVM.

This PR also introduces NullPolicy and updates the related aggregates.

Signed-off-by: firestarman [email protected]

Add a parameter controlling whether to skip nulls when creating a collect aggregation.

Signed-off-by: Liangcai Li <[email protected]>
@firestarman firestarman requested a review from a team as a code owner February 26, 2021 05:14
@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@github-actions github-actions bot added the Java Affects Java cuDF API. label Feb 26, 2021
@firestarman
Copy link
Contributor Author

@mythrocks Could you help review ?

@firestarman firestarman changed the title JVM: Support skipping nulls for collect aggregation JVM: Support skipping nulls for collect aggregation [skip-ci] Feb 26, 2021
@firestarman firestarman changed the title JVM: Support skipping nulls for collect aggregation [skip-ci] JVM: Support skipping nulls for collect aggregation [skip ci] Feb 26, 2021
firestarman added a commit to firestarman/spark-rapids that referenced this pull request Mar 1, 2021
Since cudf supports skipping null values by PRs
  rapidsai/cudf#7264, and
  rapidsai/cudf#7457.

Signed-off-by: Firestarman <[email protected]>
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

Looking very good. One minor suggestion.

java/src/main/java/ai/rapids/cudf/Aggregation.java Outdated Show resolved Hide resolved
Since ullPolicy is more readable than a bare boolean.

Signed-off-by: Firestarman <[email protected]>
@revans2 revans2 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 3, 2021
and depreacted the APIs using booleans to have the APIs
been consistent.

Signed-off-by: Firestarman <[email protected]>
@firestarman firestarman requested a review from revans2 March 4, 2021 08:49
Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Looks great thanks for doing this

@mythrocks
Copy link
Contributor

mythrocks commented Mar 4, 2021

@firestarman, thank you for making NullPolicy consistent! 🔥 ⭐ 👨 👍

@firestarman
Copy link
Contributor Author

@gpucibot merge

@firestarman
Copy link
Contributor Author

rerun tests

@firestarman
Copy link
Contributor Author

run tests

@sperlingxx
Copy link
Contributor

@gpucibot merge

@sperlingxx
Copy link
Contributor

rerun tests

@mythrocks mythrocks changed the title JVM: Support skipping nulls for collect aggregation [skip ci] JNI: Support skipping nulls for collect aggregation Mar 5, 2021
@mythrocks
Copy link
Contributor

@gpucibot merge

@firestarman
Copy link
Contributor Author

rerun tests

@firestarman
Copy link
Contributor Author

ok to test

@firestarman
Copy link
Contributor Author

rerun tests

1 similar comment
@sperlingxx
Copy link
Contributor

rerun tests

@kkraus14 kkraus14 changed the title JNI: Support skipping nulls for collect aggregation JNI: Support skipping nulls for collect aggregation [skip ci] Mar 5, 2021
@kkraus14
Copy link
Collaborator

kkraus14 commented Mar 5, 2021

ok to test

@rapids-bot rapids-bot bot merged commit cc104ef into rapidsai:branch-0.19 Mar 5, 2021
@firestarman firestarman deleted the jni-collect branch March 8, 2021 01:29
firestarman added a commit to firestarman/spark-rapids that referenced this pull request Mar 8, 2021
Since cudf supports skipping null values by PRs
  rapidsai/cudf#7264, and
  rapidsai/cudf#7457.

Signed-off-by: Firestarman <[email protected]>
firestarman added a commit to NVIDIA/spark-rapids that referenced this pull request Mar 9, 2021
Since cudf supports skipping null values by PRs
  rapidsai/cudf#7264, and
  rapidsai/cudf#7457.

Signed-off-by: Firestarman <[email protected]>
hyperbolic2346 pushed a commit to hyperbolic2346/cudf that referenced this pull request Mar 25, 2021
This PR is to support skipping nulls for `collect ` aggregation in JVM by creating a new class `CollectAggregation` who accepts a `NullPolicy ` argument indicating whether to include nulls. 

Skipping nulls has already been supported by `collect ` aggregation with rolling in native (rapidsai#7264), so this PR just exposes the feaure in JVM.

This PR also introduces `NullPolicy ` and updates the related aggregates.

Signed-off-by: firestarman <[email protected]>

Authors:
  - Liangcai Li (@firestarman)

Approvers:
  - Robert (Bobby) Evans (@revans2)
  - MithunR (@mythrocks)

URL: rapidsai#7457
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
Since cudf supports skipping null values by PRs
  rapidsai/cudf#7264, and
  rapidsai/cudf#7457.

Signed-off-by: Firestarman <[email protected]>
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
Since cudf supports skipping null values by PRs
  rapidsai/cudf#7264, and
  rapidsai/cudf#7457.

Signed-off-by: Firestarman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants