Skip to content

Add row mask builder APIs for multifile hybrid scan - #22792

Merged
rapids-bot[bot] merged 35 commits into
NVIDIA:mainfrom
mhaseeb123:fea/hybrid-scan-multifile-row-mask
Jun 16, 2026
Merged

Add row mask builder APIs for multifile hybrid scan#22792
rapids-bot[bot] merged 35 commits into
NVIDIA:mainfrom
mhaseeb123:fea/hybrid-scan-multifile-row-mask

Conversation

@mhaseeb123

Copy link
Copy Markdown
Contributor

Description

Contributes to #22583

This PR adds multifile hybrid scan reader APIs to construct row masks for two-step materialization.

Checklist

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

@mhaseeb123
mhaseeb123 requested a review from a team as a code owner June 4, 2026 23:59
@mhaseeb123
mhaseeb123 requested review from mythrocks and vyasr June 4, 2026 23:59
@mhaseeb123 mhaseeb123 added cuIO cuIO issue non-breaking Non-breaking change cudf-polars Issues specific to cudf-polars labels Jun 5, 2026
@mhaseeb123 mhaseeb123 removed the cudf-polars Issues specific to cudf-polars label Jun 5, 2026
@mhaseeb123 mhaseeb123 removed this from cuDF Python Jun 5, 2026
@coderabbitai

coderabbitai Bot commented Jun 5, 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: 7d662b22-7bec-4348-b45f-922c664a3679

📥 Commits

Reviewing files that changed from the base of the PR and between aedc4d2 and d08ba9d.

📒 Files selected for processing (1)
  • cpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp

📝 Walkthrough

Summary by CodeRabbit

  • Tests
    • Improved validation assertions for row mask handling in hybrid scan operations to ensure accuracy of row group processing.

Walkthrough

Adds <functional> include to the hybrid scan multifile filters test file and strengthens the BuildAllTrueRowMask test by computing expected_num_rows from reader->total_rows_in_row_groups(row_group_indices), asserting mask column size against it, and verifying the host-side count of true values equals expected_num_rows.

Changes

BuildAllTrueRowMask test assertion improvements

Layer / File(s) Summary
Header addition and row-mask assertion refinement
cpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
Adds <functional> include; introduces expected_num_rows derived from total_rows_in_row_groups, uses it to assert mask column size, and adds a host-side count of true values in the BOOL8 mask to validate correctness.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related issues

Possibly related PRs

  • rapidsai/cudf#22716: Changes to hybrid scan row-mask sizing and offset computation using total_rows_in_row_groups align directly with the assertion logic added here.
  • rapidsai/cudf#22793: Introduces the hybrid_scan_multifile::build_all_true_row_mask API that the updated test assertions here validate.
  • rapidsai/cudf#22715: Adds multifile row-group pruning APIs including total_rows_in_row_groups, against which the new test expectations are derived.

Suggested reviewers

  • shrshi
  • lamarrr
  • vuule
  • vyasr
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title refers to adding row mask builder APIs for multifile hybrid scan, but the actual changeset only modifies test assertions and adds an include to a test file, not the APIs themselves. Clarify whether this PR actually adds the APIs or only adds tests for them. If it only adds tests, consider a title like 'Add tests for multifile hybrid scan row mask builder APIs' or similar.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description states the PR adds multifile hybrid scan reader APIs for row masks, which aligns with the test file changes that validate row mask construction functionality.
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.

@vuule
vuule self-requested a review June 6, 2026 02:09
Comment thread cpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
@mhaseeb123
mhaseeb123 requested a review from Matt711 June 10, 2026 22:14

@Matt711 Matt711 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, nothing substantive to add here. I should probably be using these APIs in #22857

@Matt711 Matt711 removed their assignment Jun 11, 2026
@mhaseeb123 mhaseeb123 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Needs Review Waiting for reviewer to review or respond labels Jun 13, 2026
@mhaseeb123

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit b749f72 into NVIDIA:main Jun 16, 2026
356 of 364 checks passed
@mhaseeb123
mhaseeb123 deleted the fea/hybrid-scan-multifile-row-mask branch June 16, 2026 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge cuIO cuIO issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants