Skip to content

Fix misleading Javadoc for Aggregation.nth(int)#22486

Open
piotrrzysko wants to merge 1 commit into
rapidsai:mainfrom
piotrrzysko:fix-nth-javadoc
Open

Fix misleading Javadoc for Aggregation.nth(int)#22486
piotrrzysko wants to merge 1 commit into
rapidsai:mainfrom
piotrrzysko:fix-nth-javadoc

Conversation

@piotrrzysko
Copy link
Copy Markdown

@piotrrzysko piotrrzysko commented May 13, 2026

Description

The comment stated "non-null element" but the method defaults to NullPolicy.INCLUDE, meaning NULL values can be returned.

Checklist

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

@piotrrzysko piotrrzysko requested a review from a team as a code owner May 13, 2026 06:40
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 13, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the Java Affects Java cuDF API. label May 13, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 13, 2026 •
edited
Loading

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 63b60c4c-b00e-4094-8397-cb151048b63d

📥 Commits

Reviewing files that changed from the base of the PR and between 262ae67 and 9c2ccdc.

📒 Files selected for processing (1)
  • java/src/main/java/ai/rapids/cudf/Aggregation.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • java/src/main/java/ai/rapids/cudf/Aggregation.java
📝 Walkthrough

Summary by CodeRabbit

  • Documentation
    • Clarified in the docs that selecting the nth element treats NULLs as part of the sequence by default, and added guidance pointing to the alternative API to control NULL handling.
    • This change is documentation-only; no runtime behavior or public API signatures were modified.

Walkthrough

This PR updates the Javadoc for Aggregation.nth(int) to state NULLs are included by default and directs callers to nth(int, NullPolicy) to control NULL handling.

Changes

nth Aggregation Documentation Update

Layer / File(s) Summary
nth aggregation NULL handling documentation
java/src/main/java/ai/rapids/cudf/Aggregation.java		 Updated file header year and Javadoc for the nth aggregation to explicitly state that NULL values are included by default and to reference nth(int, NullPolicy) for controlling NULL handling.

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing misleading Javadoc for the Aggregation.nth(int) method to reflect its actual NULL handling behavior.
Description check ✅ Passed The description clearly explains why the change was needed: the Javadoc incorrectly stated 'non-null element' when the method defaults to NullPolicy.INCLUDE, allowing NULL values.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

Comment @coderabbitai help to get the list of available commands and usage tips.

findepi
findepi approved these changes May 13, 2026
View reviewed changes
Comment thread java/src/main/java/ai/rapids/cudf/Aggregation.java Outdated
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team doc Documentation non-breaking Non-breaking change labels May 13, 2026
@rapidsai rapidsai deleted a comment from copy-pr-bot Bot May 13, 2026
@davidwendt
Copy link
Copy Markdown
Contributor

davidwendt commented May 13, 2026

/ok to test 262ae67

@piotrrzysko
Copy link
Copy Markdown
Author

piotrrzysko commented May 14, 2026

@davidwendt thanks for taking a look at this PR!

CI fails with:

verify-copyright-cudf....................................................Failed
- hook id: verify-copyright
- exit code: 1
- files were modified by this hook

In file java/src/main/java/ai/rapids/cudf/Aggregation.java:3:43:
  *  SPDX-FileCopyrightText: Copyright (c) 2020-2025, NVIDIA CORPORATION.
warning: copyright is out of date

In file java/src/main/java/ai/rapids/cudf/Aggregation.java:3:29:
- *  SPDX-FileCopyrightText: Copyright (c) 2020-2025, NVIDIA CORPORATION.
+ *  SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION.

This seems unrelated to the PR. Should I fix it in this PR?

@davidwendt
Copy link
Copy Markdown
Contributor

davidwendt commented May 14, 2026

@davidwendt thanks for taking a look at this PR!

CI fails with:

verify-copyright-cudf....................................................Failed
....
This seems unrelated to the PR. Should I fix it in this PR?

@piotrrzysko Yes. If you modify a file you are expected to update the copyright year at the top of that file.
https://github.com/rapidsai/cudf/blob/main/cpp/doxygen/developer_guide/DOCUMENTATION.md#copyright-license

@paul-aiyedun
Copy link
Copy Markdown

paul-aiyedun commented May 14, 2026

@davidwendt thanks for taking a look at this PR!
CI fails with:

verify-copyright-cudf....................................................Failed
....
This seems unrelated to the PR. Should I fix it in this PR?

@piotrrzysko Yes. If you modify a file you are expected to update the copyright year at the top of that file. https://github.com/rapidsai/cudf/blob/main/cpp/doxygen/developer_guide/DOCUMENTATION.md#copyright-license

@piotrrzysko Please execute pre-commit run --all-files and that will apply updates like this automatically. See https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md#using-pre-commit-hooks for more details about using pre-commit.

@piotrrzysko
Fix misleading Javadoc for Aggregation.nth(int)
9c2ccdc 
The comment stated "non-null element" but the method defaults to
NullPolicy.INCLUDE, meaning NULL values can be returned.
@davidwendt
Copy link
Copy Markdown
Contributor

davidwendt commented May 14, 2026

/ok to test 9c2ccdc

@davidwendt
Copy link
Copy Markdown
Contributor

davidwendt commented May 15, 2026

Builds are failing. We just need to wait until they are working again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@findepi findepi findepi approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@piotrrzysko piotrrzysko

Labels

3 - Ready for Review Ready for review by team doc Documentation 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.

4 participants

@piotrrzysko @davidwendt @paul-aiyedun @findepi