Skip to content

Fix FULL_JOIN filtering through internal finalization - #24146

Open
bdice wants to merge 11 commits into
NVIDIA:release/26.10from
bdice:fix/full-join-filter-26.10
Open

bdice wants to merge 11 commits into
NVIDIA:release/26.10from
bdice:fix/full-join-filter-26.10

Conversation

@bdice

@bdice bdice commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Description

FULL join filtering split rejected equality candidates independently, which could emit duplicate or spurious unmatched rows when equality keys repeat.

Keep FULL_JOIN supported by the public filtering APIs while reusing the established composition internally. For AST and both JIT paths, the implementation removes the unmatched-right entries from the input FULL maps, invokes the existing LEFT_JOIN filtering path, and calls detail::finalize_full_join to append exactly one entry for each right row with no surviving match. This gives callers the direct FULL API while sharing the same LEFT-filter-plus-finalization behavior used by mixed_full_join.

filter_join_indices_output_size retains a dedicated non-materializing FULL count: it counts surviving pairs per left row, tracks matched right rows, and includes unmatched rows from both sides. A supplied output size is forwarded to finalization as the exact unmatched-right count after LEFT filtering. The full-join benchmark again exercises the direct API, and the finalizer contract documents that filtered LEFT results are valid input.

Regression coverage compares direct FULL_JOIN filtering with the explicit LEFT-filter/finalize composition, an independent host oracle, and conditional FULL join. It covers duplicate equality keys, nulls, empty inputs, output sizing, supplied sizes, ownership, AST JIT, and string JIT.

Closes #24145.

Validation

  • Built JOIN_TEST, STREAM_JOIN_TEST, generate_ctest_json, and cudf_identify_stream_usage_mode_cudf in the release/26.10 CUDA 13.3 conda devcontainer.
  • JOIN_TEST and STREAM_JOIN_TEST passed with --output-on-failure.
  • Repository pre-commit checks passed, including clang-format and Doxygen validation.
  • The benchmark call site was compiled but performance benchmarks were not run.

Checklist

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

@bdice
bdice requested a review from a team as a code owner September 13, 2026 06:20
@bdice
bdice requested review from lamarrr and vuule September 13, 2026 06:20
@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 13, 2026
bdice added a commit to bdice/cudf that referenced this pull request Sep 13, 2026
@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

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: a18145e6-d245-422d-b7eb-e65de1daf6d7

📥 Commits

Reviewing files that changed from the base of the PR and between 1a175e0 and baa9e52.

📒 Files selected for processing (1)
  • cpp/benchmarks/join/filter_join_indices.cpp

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


📝 Summary

Summary by CodeRabbit

  • Behavior Changes

    • Full-join filtering is no longer supported directly by filtering APIs.
    • To filter a full join, apply filtering to a left join first, then finalize the result as a full join.
    • The combined full-join operation remains available through mixed_full_join.
    • Filtering APIs now support inner and left joins, with consistent handling of empty maps and output sizes.
  • Documentation

    • Clarified full-join finalization requirements, including unmatched right-side rows and filtered results.

Walkthrough

Changes

FULL join filtering

Layer / File(s) Summary
Filtering contracts and implementation
cpp/include/cudf/join/join.hpp, cpp/src/join/filter_join_indices/*
Filtering APIs and JIT paths now accept only INNER_JOIN and LEFT_JOIN. FULL_JOIN is rejected, and full-join-specific output counting and materialization were removed.
Full-join composition and integration
cpp/include/cudf/join/hash_join.hpp, cpp/benchmarks/join/filter_join_indices.cpp
Documentation and the benchmark use filtered LEFT join results followed by finalize_partitioned_full_join.
Filtering regression coverage
cpp/tests/join/mixed_join_tests.cu, cpp/tests/streams/join_test.cpp
Tests cover left-join filtering, full-join rejection, empty maps, duplicate keys, nulls, many-to-many matches, output sizing, and JIT paths.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to baa9e

The API change consistently routes filtered full joins through LEFT filtering and finalization, with coverage for rejection and composed behavior. No merge-blocking issue remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #24145 requires consistent behavior for materialization, output sizing, and JIT filtering. The changes reject FULL_JOIN in all filter_join_indices overloads and in `filter_join_indices_outpu…
Out of Scope Changes check ✅ Passed The changes stay within Issue #24145. The finalizer documentation, benchmark, stream coverage, and regression tests support the replacement composition and verify the removed FULL filtering behavior. …
Title check ✅ Passed The title addresses FULL_JOIN filtering and finalization, which are central to the pull request. It is somewhat misleading because the implementation removes FULL_JOIN filtering support from the publi…
Description check ✅ Passed The description directly discusses FULL_JOIN filtering, finalization, benchmarks, tests, and validation. It is related to the changeset, although it conflicts with the provided implementation summary …
✨ 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.

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

The github issue is valid but the fix needs further discussion.

Could we reuse the LEFT-filter + finalize_full_join composition already used by mixed_full_join, instead of adding a separate FULL materialization path? Maintaining two implementations makes full-join behavior harder to follow and keep consistent. Is there a performance or memory usage concern that makes the existing composition unsuitable?

@PointKernel

Copy link
Copy Markdown
Member

Thinking about this issue again, the bug report indeed uncovered some leftover code that I didn’t clean up during the bug fix and mixed_full_join migration. I also did some local testing and found that the existing solution with a follow-up finalization step provides better runtime performance and lower peak memory usage.

With that in mind, could we simplify the implementation by rejecting FULL_JOIN in filter_join_indices, both JIT overloads, and the output-size calculation, and documenting the existing composition instead?

To perform a full join, we would manually call hash_join::left_join, then invoke filter_join_indices(LEFT_JOIN), and finally call hash_join::finalize_partitioned_full_join, similar to the existing mixed_full_join implementation.

We would also need to extend the finalizer’s documented contract to accept filtered LEFT results.

@bdice what do you think?

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

Thanks for this PR and unifying both precompiled and JIT implementations.
Just a few minor changes left

Comment thread cpp/src/join/filter_join_indices/filter_join_indices.cu Outdated
Comment thread cpp/src/join/filter_join_indices/filter_join_indices.cu Outdated
Comment thread cpp/src/join/filter_join_indices/filter_join_indices.cu
@copy-pr-bot

copy-pr-bot Bot commented Sep 15, 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.

@bdice bdice changed the title Fix full join filtering with duplicate equality keys Remove unsupported FULL_JOIN filtering Sep 15, 2026
@bdice

bdice commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Agent reply: Bradley asked me (his coding agent) to reply on his behalf.

@PointKernel, implemented your proposal: filter_join_indices, both JIT overloads, and output-size calculation now reject FULL_JOIN. The dedicated FULL materialization and sizing paths are removed. The documented sequence is LEFT equality join → filter_join_indices(LEFT_JOIN)hash_join::finalize_partitioned_full_join, which delegates to detail::finalize_full_join. The finalizer contract now explicitly accepts filtered LEFT results, and the FULL benchmark uses that composition.

The regression cases now validate the composition against the host and conditional-FULL oracles, with additional rejection checks for every filtering entry point. On release/26.10 with CUDA 13.3: JOIN_TEST passed 851 tests (2 existing skips), including all 13 composition/rejection cases; STREAM_JOIN_TEST passed all 27 tests. Pre-commit checks passed. No new performance benchmark results are claimed.

@lamarrr, the allocations and FULL transform addressed by your three inline suggestions were removed along with the dedicated FULL implementation.

Java PR #24097 still needs to align its FULL behavior with this native API change before merging.

PointKernel
PointKernel previously approved these changes Sep 15, 2026

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

Yes, this is technically correct, though the API is awkward to use. Requiring users to perform a left join, apply post-filtering, and manually finalize the results is neither ergonomic nor foolproof. That said, I don't have a better solution at the moment. Thanks @bdice for the discussion and for addressing my review comments.

@PointKernel

Copy link
Copy Markdown
Member

/ok to test 1a175e0

@PointKernel

Copy link
Copy Markdown
Member

/ok to test baa9e52

@PointKernel PointKernel added the bug Something isn't working label Sep 16, 2026
@PointKernel PointKernel added the breaking Breaking change label Sep 16, 2026
@bdice bdice changed the title Remove unsupported FULL_JOIN filtering Fix FULL_JOIN filtering through internal finalization Sep 17, 2026
@bdice

bdice commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Agent reply: Bradley asked me (his coding agent) to reply on his behalf.

Correction to my previous reply: Bradley wanted FULL_JOIN to remain supported by filter_join_indices, with the LEFT-filter/finalize sequence performed for the caller rather than exposed as a manual workflow.

Commit 733eca2241 now does that. The AST and both JIT entry points remove the original unmatched-right entries from FULL equality maps, invoke filter_join_indices(LEFT_JOIN), and call detail::finalize_full_join to reconstruct the right-side complement. The output-size API computes the corresponding FULL size without materializing the result, and a supplied size is used as the exact unmatched-right count during finalization.

The regression test compares the direct FULL call against the explicit LEFT → filter → finalize composition, as well as the independent host and conditional-FULL oracles, including duplicate keys, nulls, empty inputs, sizing, and both JIT paths. JOIN_TEST and STREAM_JOIN_TEST pass in the release/26.10 CUDA 13.3 conda devcontainer, and pre-commit passes.

@PointKernel, this preserves the implementation strategy you proposed while keeping the public API ergonomic and foolproof, consistent with your later review observation.

@PointKernel

Copy link
Copy Markdown
Member

@bdice How about performance? The reason I didn’t do it this way was because IIRC, it resulted in a 2–3x slowdown.

@PointKernel

Copy link
Copy Markdown
Member

/ok to test 73a45f4

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

Labels

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

Projects

Status: Burndown

Development

Successfully merging this pull request may close these issues.

3 participants