Skip to content

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

Merged
rapids-bot[bot] merged 4 commits into
NVIDIA:mainfrom
piotrrzysko:fix-nth-javadoc
May 18, 2026
Merged

Fix misleading Javadoc for Aggregation.nth(int)#22486
rapids-bot[bot] merged 4 commits into
NVIDIA:mainfrom
piotrrzysko:fix-nth-javadoc

Conversation

@piotrrzysko

Copy link
Copy Markdown
Contributor

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-pr-bot Bot commented May 13, 2026

Copy link
Copy Markdown

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

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown

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: 765e8161-daed-4fdc-ac7a-5e8a57c80c44

📥 Commits

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

📒 Files selected for processing (1)
  • java/src/main/java/ai/rapids/cudf/Aggregation.java
✅ Files skipped from review due to trivial changes (1)
  • java/src/main/java/ai/rapids/cudf/Aggregation.java

📝 Walkthrough

Summary by CodeRabbit

  • Documentation
    • Clarified that selecting the nth element includes NULLs by default and added guidance pointing to the alternative API to control NULL handling.
    • Documentation-only update; no runtime behavior or public API signatures were changed.

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
Replaced the nth(int) Javadoc to explicitly state 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 describes the main change—fixing misleading Javadoc documentation for the Aggregation.nth(int) method.
Description check ✅ Passed The description is directly related to the changeset, explaining that the Javadoc comment incorrectly stated 'non-null element' when the method defaults to NullPolicy.INCLUDE.
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

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

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
@NVIDIA NVIDIA deleted a comment from copy-pr-bot Bot May 13, 2026
@davidwendt

Copy link
Copy Markdown
Contributor

/ok to test 262ae67

@piotrrzysko

Copy link
Copy Markdown
Contributor Author

@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 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
Contributor

@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.

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

/ok to test 9c2ccdc

@davidwendt

Copy link
Copy Markdown
Contributor

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

@davidwendt davidwendt removed their assignment May 18, 2026
@davidwendt

Copy link
Copy Markdown
Contributor

/ok to test b5cde3f

@davidwendt davidwendt removed their assignment May 18, 2026
@davidwendt

Copy link
Copy Markdown
Contributor

/ok to test bb6f0b8

@revans2

revans2 commented May 18, 2026

Copy link
Copy Markdown
Contributor

Thanks for fixing my mistake.

@davidwendt

Copy link
Copy Markdown
Contributor

Now waiting on #22549 to merge this

@davidwendt davidwendt removed their assignment May 18, 2026
@davidwendt

Copy link
Copy Markdown
Contributor

/ok to test 351312f

@davidwendt

Copy link
Copy Markdown
Contributor

This PR is cursed.

@davidwendt

Copy link
Copy Markdown
Contributor

/merge

@rapids-bot
rapids-bot Bot merged commit c084936 into NVIDIA:main May 18, 2026
253 of 259 checks passed
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 doc Documentation Java Affects Java cuDF API. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants