Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughChangesFULL join filtering
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to 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)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
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?
|
Thinking about this issue again, the bug report indeed uncovered some leftover code that I didn’t clean up during the bug fix and With that in mind, could we simplify the implementation by rejecting To perform a full join, we would manually call We would also need to extend the finalizer’s documented contract to accept filtered LEFT results. @bdice what do you think? |
lamarrr
left a comment
There was a problem hiding this comment.
Thanks for this PR and unifying both precompiled and JIT implementations.
Just a few minor changes left
|
Agent reply: Bradley asked me (his coding agent) to reply on his behalf. @PointKernel, implemented your proposal: 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
left a comment
There was a problem hiding this comment.
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.
|
/ok to test 1a175e0 |
|
/ok to test baa9e52 |
|
Agent reply: Bradley asked me (his coding agent) to reply on his behalf. Correction to my previous reply: Bradley wanted Commit 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. @PointKernel, this preserves the implementation strategy you proposed while keeping the public API ergonomic and foolproof, consistent with your later review observation. |
|
@bdice How about performance? The reason I didn’t do it this way was because IIRC, it resulted in a 2–3x slowdown. |
|
/ok to test 73a45f4 |
Description
FULL join filtering split rejected equality candidates independently, which could emit duplicate or spurious unmatched rows when equality keys repeat.
Keep
FULL_JOINsupported 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 existingLEFT_JOINfiltering path, and callsdetail::finalize_full_jointo 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 bymixed_full_join.filter_join_indices_output_sizeretains 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_JOINfiltering 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
JOIN_TEST,STREAM_JOIN_TEST,generate_ctest_json, andcudf_identify_stream_usage_mode_cudfin the release/26.10 CUDA 13.3 conda devcontainer.JOIN_TESTandSTREAM_JOIN_TESTpassed with--output-on-failure.Checklist