Skip to content

Add common load factor validation for joins - #23379

Merged
rapids-bot[bot] merged 6 commits into
NVIDIA:mainfrom
PointKernel:common-join-load-factor-validation
Jul 27, 2026
Merged

Add common load factor validation for joins#23379
rapids-bot[bot] merged 6 commits into
NVIDIA:mainfrom
PointKernel:common-join-load-factor-validation

Conversation

@PointKernel

@PointKernel PointKernel commented Jul 21, 2026

Copy link
Copy Markdown
Member

Description

This PR adds a common cudf::detail::checked_load_factor helper and uses it across cudf::hash_join, cudf::distinct_hash_join, cudf::filtered_join, and cudf::mark_join.

This ensures all hash-table joins validate the load factor before constructing CUCO storage and consistently throw std::invalid_argument for values outside (0, 1]. It also adds the missing load-factor documentation and tests for filtered and mark joins.

Checklist

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

@copy-pr-bot

copy-pr-bot Bot commented Jul 21, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Jul 21, 2026
@PointKernel PointKernel changed the title Centralize join load factor validation Add common load factor validation for joins Jul 21, 2026
@PointKernel PointKernel self-assigned this Jul 21, 2026
@PointKernel PointKernel added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Jul 21, 2026
@PointKernel
PointKernel marked this pull request as ready for review July 21, 2026 21:57
@PointKernel
PointKernel requested a review from a team as a code owner July 21, 2026 21:57
@coderabbitai

coderabbitai Bot commented Jul 21, 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: 6f8e4408-7690-4f20-8d57-4f976ef04b7a

📥 Commits

Reviewing files that changed from the base of the PR and between 576d93c and a8b71c3.

📒 Files selected for processing (2)
  • cpp/include/cudf/join/mark_join.hpp
  • cpp/tests/join/semi_anti_join_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • cpp/include/cudf/join/mark_join.hpp
  • cpp/tests/join/semi_anti_join_tests.cpp

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Invalid join load_factor values are now consistently rejected with std::invalid_argument.
    • Applies to hash, filtered, mark, semi/anti, and distinct joins.
    • Join capacity/bucket sizing now validates load_factor before it’s used.
  • Documentation

    • Updated join constructor documentation to state that load_factor outside (0, 1] throws std::invalid_argument.
  • Tests

    • Updated and added tests to expect std::invalid_argument for invalid load_factor across join types.

Walkthrough

Join load-factor validation is centralized in checked_load_factor, integrated into distinct, hash, filtered, and mark joins, documented in public constructor APIs, and covered by updated exception tests using std::invalid_argument.

Changes

Join load-factor validation

Layer / File(s) Summary
Centralized load-factor validation
cpp/src/join/join_common_utils.hpp, cpp/src/join/join_utils.cu
Adds checked_load_factor, which validates the (0, 1] range and throws std::invalid_argument for invalid values.
Join constructor integration
cpp/src/join/*join*.cu, cpp/include/cudf/join/*join.hpp
Routes join hash-table and bucket-storage sizing through checked_load_factor and documents the resulting exception behavior.
Exception behavior tests
cpp/tests/join/*
Updates invalid-load-factor expectations and adds filtered and mark join coverage using std::invalid_argument.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested labels: doc

Suggested reviewers: shrshi, vyasr, mhaseeb123

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

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.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: centralizing load factor validation across joins.
Description check ✅ Passed The description matches the changeset and accurately describes the new helper, affected joins, docs, and tests.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment thread cpp/tests/join/semi_anti_join_tests.cpp
Comment thread cpp/include/cudf/join/mark_join.hpp Outdated

@igorpeshansky igorpeshansky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM :shipit:

@PointKernel

Copy link
Copy Markdown
Member Author

/merge

@rapids-bot
rapids-bot Bot merged commit 6ea90fd into NVIDIA:main Jul 27, 2026
136 checks passed
@PointKernel
PointKernel deleted the common-join-load-factor-validation branch July 27, 2026 16:37
@coderabbitai coderabbitai Bot mentioned this pull request Jul 29, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants