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

Add java bindings for distinct count #13573

Merged
merged 2 commits into from
Jun 14, 2023

Conversation

revans2
Copy link
Contributor

@revans2 revans2 commented Jun 14, 2023

Description

This adds in java bindings for cudf::distinct_count for both table and column APIs.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@revans2 revans2 added 3 - Ready for Review Ready for review by team Java Affects Java cuDF API. Spark Functionality that helps Spark RAPIDS 4 - Needs cuDF (Java) Reviewer improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 14, 2023
@revans2 revans2 requested a review from a team as a code owner June 14, 2023 16:29
@revans2 revans2 self-assigned this Jun 14, 2023
Copy link
Contributor

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 3971 to 3980
* Count how many rows in the column are distinct from one another.
* @param nullPolicy if nulls should be included or not.
*/
public int distinctCount(NullPolicy nullPolicy) {
return distinctCount(getNativeView(), nullPolicy.includeNulls, false);
}

/**
* Count how many rows in the column are distinct from one another.
* Nulls are included.
Copy link
Contributor

@ttnghia ttnghia Jun 14, 2023

Choose a reason for hiding this comment

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

Please also clarify the last parameter in the doc. What does false mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What parameter? The false in the native code?

@@ -3999,6 +4015,8 @@ static DeviceMemoryBufferView getOffsetsBuffer(long viewHandle) {
}

// Native Methods
private static native int distinctCount(long handle, boolean nullsIncluded, boolean nansAreNulls);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this parameter? I don't see any public Java API calling it. If we are not using it then should we just remove it completely from Java code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it thinking that there was a java enum that corresponded to it, but never got around to adding in the enum. I can remove it if we want to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please remove it.

Comment on lines 4753 to 4754


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Too many empty lines.

@revans2
Copy link
Contributor Author

revans2 commented Jun 14, 2023

/merge

@rapids-bot rapids-bot bot merged commit 649cf5e into rapidsai:branch-23.08 Jun 14, 2023
@revans2 revans2 deleted the jni_distinct_count branch June 14, 2023 19:21
@vyasr vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuDF (Java) Reviewer labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants