Skip to content

Fix percentile_approx for mixed empty TDigests - #24103

Closed
wjxiz1992 wants to merge 6 commits into
NVIDIA:mainfrom
wjxiz1992:codex/fix-percentile-empty-tdigest
Closed

wjxiz1992 wants to merge 6 commits into
NVIDIA:mainfrom
wjxiz1992:codex/fix-percentile-empty-tdigest

Conversation

@wjxiz1992

Copy link
Copy Markdown
Contributor

Description

Closes #24056.

percentile_approx currently marks rows from empty TDigests null while retaining
percentiles.size() child elements for those rows. Mixed empty and non-empty input
therefore produces non-empty null list rows, which downstream consumers reject.

This change:

  • gives empty TDigests zero-length list ranges and writes non-empty rows directly into
    a compact child column;
  • builds the parent validity mask without an additional host synchronization;
  • preserves the dense fast path when no TDigests are empty; and
  • bounds the percentile kernel launch and uses a grid-stride loop for sparse inputs
    whose dense row/percentile pair count exceeds a valid CUDA grid.

Regression coverage includes nullable percentile values and mixed-input validity-mask
boundaries at 31, 32, 33, 65, and 257 rows.

Validation on 2cf22a1fb5b864bd53128034d34217943bea0047:

[==========] Running 2 tests from 1 test suite.
[  PASSED  ] 2 tests.

[==========] 388 tests from 67 test suites ran. (2119 ms total)
[  PASSED  ] 388 tests.

The native build used CUDA 12.9.1 and CMAKE_CUDA_ARCHITECTURES=80-real;89-real.

Checklist

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

Fixes NVIDIA#24056.

Signed-off-by: Allen Xu <allxu@nvidia.com>
Address review feedback by writing non-empty digest rows directly into compact list child ranges. This avoids a full identity gather while preserving the no-empty fast path.

Signed-off-by: Allen Xu <allxu@nvidia.com>
Build the empty-digest mask and compact offsets asynchronously after the single required empty-count reduction.

Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
@wjxiz1992
wjxiz1992 requested a review from a team as a code owner September 10, 2026 03:02
@wjxiz1992
wjxiz1992 requested review from mhaseeb123 and simoneves and a lite review from Copilot September 10, 2026 03:02
@copy-pr-bot

copy-pr-bot Bot commented Sep 10, 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 libcudf Affects libcudf (C++/CUDA) code. label Sep 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The empty-percentile path incorrectly marks all rows null, and documentation needs to describe the empty-digest behavior.

Pull request overview

Fixes percentile_approx output for mixed empty and non-empty TDigests by compacting list children and preserving validity boundaries.

Changes:

  • Adds compact offsets and output handling for empty digests.
  • Uses bounded grid-stride percentile processing.
  • Adds mixed-input and validity-mask boundary regression tests.
File summaries
File Summary
cpp/tests/quantiles/percentile_approx_test.cpp Adds regression and boundary tests.
cpp/src/quantiles/tdigest/tdigest.cu Implements compact output and validity-mask handling.
Review details

Suppressed comments (2)

cpp/src/quantiles/tdigest/tdigest.cu:184

  • This new compact path makes empty digests produce null list rows with zero-length ranges, but the public percentile_approx documentation still promises that every list has percentiles.size() elements (cpp/include/cudf/quantiles.hpp:108-110). Please update the public and detail documentation to describe the empty-digest/null-row behavior; otherwise the API contract still describes the invalid pre-fix shape.
 * @param num_output_values Number of elements to allocate in the output column.
 * @param output_offsets    Optional compact output offsets for inputs containing empty digests.

cpp/src/quantiles/tdigest/tdigest.cu:395

  • This branch changes the existing empty-percentile behavior for non-empty digests: EmptyPercentiles expects valid zero-length lists (see percentile_approx_test.cpp:483-499), but make_all_null_result() marks every row null. For a mixed input it also loses the distinction between empty and non-empty digests. Keep the zero-length offsets/child path for percentiles.size() == 0, applying the empty-digest validity mask/count instead of forcing all rows null.
  if (percentiles.size() == 0) { return make_all_null_result(); }
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Review Change StackReview 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: b1edf4aa-5078-4e35-ae37-94a98412b768

📥 Commits

Reviewing files that changed from the base of the PR and between 2cf22a1 and 0ee212f.

📒 Files selected for processing (2)
  • cpp/src/quantiles/tdigest/tdigest.cu
  • cpp/tests/quantiles/percentile_approx_test.cpp

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved approximate percentile results for grouped data containing empty and non-empty digests.
    • Preserved correct null values, list offsets, and validity masks in mixed-result outputs.
    • Handled empty percentile inputs and empty digest rows more reliably.
  • Tests

    • Added coverage for null values and boundary-sized grouped results.

Walkthrough

Changes

The t-digest percentile path now supports compact list output for mixed empty and non-empty digests. Empty digest rows receive zero-length ranges, while non-empty rows write to compact child storage. Regression tests cover nullable percentiles and row-count boundaries.

Compact t-digest percentile output

Layer / File(s) Summary
Compact percentile kernel
cpp/src/quantiles/tdigest/tdigest.cu
compute_approx_percentiles accepts output sizing and optional offsets. The kernel supports compact and rectangular output with bounded grid-stride processing.
Mixed digest handling and regression coverage
cpp/src/quantiles/tdigest/tdigest.cu, cpp/tests/quantiles/percentile_approx_test.cpp
percentile_approx builds compact offsets, handles empty inputs, validates output-size overflow, and applies null masks. Tests cover mixed digests, nullable percentiles, and row counts from 31 through 257.

Estimated code review effort: 4 (Complex) | ~45 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 0ee21

The change corrects mixed empty and non-empty TDigest percentile output while preserving dense behavior, with focused boundary and nullability tests passing. No actionable merge-blocking risk remains.

Suggested reviewers: davidwendt

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary fix for mixed empty and non-empty TDigests in percentile_approx.
Description check ✅ Passed The description directly explains the mixed-TDigest bug, the compact-output fix, kernel behavior, regression tests, and validation results.
Linked Issues check ✅ Passed The changes address issue #24056 by producing zero-length ranges for empty digests, compacting non-empty output, preserving the dense path, bounding kernel launches, and adding the required regression…
Out of Scope Changes check ✅ Passed The implementation and tests remain within the linked issue scope. The changes are limited to percentile_approx handling, CUDA kernel behavior, and related regression coverage.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@wjxiz1992

Copy link
Copy Markdown
Contributor Author

Related implementation: #24095 was already open when this PR was published. The exact-head comparison and validation delta are documented there so the work can be consolidated without losing the additional coverage. Also, the automated empty-percentiles concern does not represent a regression: the existing EmptyPercentiles test intentionally constructs an all-null expected list column, and that test is included in the passing 388/388 QUANTILES_TEST run.

@wjxiz1992

Copy link
Copy Markdown
Contributor Author

Closing in favor of the earlier #24095 to avoid parallel conflicting implementations. I have moved the exact-head comparison and additional test/robustness observations to #24095 and will review that PR directly.

@wjxiz1992 wjxiz1992 closed this Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] percentile_approx returns non-empty null list rows for mixed empty and non-empty TDigests

2 participants