Merge release/26.08 into main - #23398
Closed
davidwendt wants to merge 8 commits into
Closed
Conversation
…A#23381) Closes NVIDIA#23287. The Glushkov eligibility checker previously rejected an `ACCEPT` item only when it appeared before the first character-consuming frontier item. This missed Thompson-priority frontiers such as `[LF, ACCEPT, CR-repeat]`, where a successful accept has higher priority than a later continuation. Flattening that ordering into Glushkov bit positions caused a reluctant `\r+?` delimiter to consume a second `\r`, producing greedy behavior in `split_record_re`. This PR: - rejects a Glushkov frontier whenever an `ACCEPT` item is followed by a later `CHAR_POS`, conservatively falling back to the Thompson engine; - preserves safe frontiers that end in `ACCEPT`; - adds `StringsSplitTest.SplitRecordRegexLazyQuantifier` to verify the delimiter length and resulting split records. The change affects only patterns whose Thompson-priority ordering cannot be represented faithfully by the Glushkov fast path. Supported patterns continue to use Glushkov. ### Validation - Focused `StringsSplitTest.SplitRecordRegexLazyQuantifier`: 1/1 passed. - Focused regression with `LIBCUDF_DISABLE_GLUSHKOV=1`: 1/1 passed. - Full `STRINGS_TEST`: 540/540 passed. - Clean local `spark-rapids-jni` package using this cuDF checkout: `BUILD SUCCESS`; a second same-toolchain rebuild also completed successfully. - NVIDIA/cudf-spark, Scala 2.13 / Spark 4.0.1, `RegularExpressionTranspilerSuite`: 97 succeeded, 0 failed, 6 pre-existing canceled tests; Maven `BUILD SUCCESS`. The original `string split fuzz - anchor focused` failure passed. Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Muhammad Haseeb (https://github.com/mhaseeb123) - Yunsong Wang (https://github.com/PointKernel) - Vyas Ramasubramani (https://github.com/vyasr) - Igor Peshansky (https://github.com/igorpeshansky) URL: NVIDIA#23381
…VIDIA#23366) Split out of NVIDIA#23255 (5/6). `GroupBy.agg` flattened a MultiIndex-column source's aggregation result to flat tuple labels instead of keeping hierarchical columns like pandas. Preserve the MultiIndex (and its per-level metadata) when the aggregation keeps the source's tuple labels; relabeling aggregations (`agg(new=(col, func))`) emit new flat labels, so the source's multi-level metadata is not attached to those. Fixes 3 pandas-tests (`test_groupby_with_hier_columns`, `test_wrap_aggregated_output_multindex`, `test_multiindex_custom_func[<lambda>0]`); their xfail entries are removed. Attribution verified by running the node ids against an isolated build containing only this change (pass) and a clean build (fail). Independent of the other NVIDIA#23255 split PRs; the unstack PR (4/6) depends on this one for two entangled tests. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#23366
libcudf's SUM/PRODUCT scans promote narrow integers to 64-bit. pandas does the same for numpy dtypes (int8 -> int64, pandas GH#37493) but preserves masked extension dtypes (Int16 stays Int16, pandas GH#58811), wrapping on overflow. Cast the scan result back to the original dtype for masked integer inputs only. Fixes 8 pandas-tests (`test_groupby_cumsum_mask[Int*/UInt*-True-3]`, `test_nan_in_cumsum_group_label`); their xfail entries are removed. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Matthew Roeschke (https://github.com/mroeschke) URL: NVIDIA#23299
…aitables (NVIDIA#23293) To avoid cancellation in python leading to attempting to set a result on an already cancelled Future, use the new pattern introduced in rapidsai/rapidsmpf#1131 Authors: - Lawrence Mitchell (https://github.com/wence-) - Tom Augspurger (https://github.com/TomAugspurger) Approvers: - Tom Augspurger (https://github.com/TomAugspurger) - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#23293
Part of NVIDIA#22124 This PR rewrites the mixed inner/left/full joins as equality-then-filter: run the keys through `cudf::hash_join`, then apply the conditional predicate to the index pairs via a new `filter_join_indices` primitive (AST + JIT, plus `filter_join_indices_output_size`). `mixed_full_join` becomes a left join plus `finalize_full_join`. This replaces the fused mixed-join kernels, reusing `hash_join` instead of duplicating it. Authors: - Yunsong Wang (https://github.com/PointKernel) Approvers: - Muhammad Haseeb (https://github.com/mhaseeb123) - Shruti Shivakumar (https://github.com/shrshi) - Nghia Truong (https://github.com/ttnghia) URL: NVIDIA#23012
…truct pre-filters for inner joins (NVIDIA#22996) Add a streaming optimizer pass that attempts to pre-filter one side of an input to inner joins before actor-graph lowering. The pass uses existing dynamic-planning scan statistics and join metadata to determine where it is beneficial to push a semi-join against a join key onto the other side of a join. The simplest example of such a rewrite is that we turn ```python left.join(right, on="key", how="inner") ``` into, assuming we somehow determine that `right` is selective, ```python ( left.join(right.select("key"), on="key", how="semi") .join(right, on="key", how="inner") ) ``` The optimization pass handles the case where a "domain" key, used to provide the right-hand side of the semi join, is "simple" and derived directly from some input node, as well as the more complex case where a domain key is already constrained by some other semi-join filter. Only inner joins are rewritten, and only if all the keys are simple column keys. If heuristics determine that simple keys are not selective, we also don't perform the rewrite. Material results of this change running NDSH SF30K on 8xNVL4 nodes are (previous results come from the change in NVIDIA#22995): * **Q5 doesn't OOM on 8 nodes anymore and improved runtime performance: 9.35s lukewarm, 5.14s hot** (previously 40.25s lukewarm, OOM on hot) * Q9 unchanged performance or slight regression: 47.12s lukewarm, 32.68s hot (previously 43.63s lukewarm, 30.56s hot) Authors: - Peter Andreas Entschev (https://github.com/pentschev) - Lawrence Mitchell (https://github.com/wence-) Approvers: - Lawrence Mitchell (https://github.com/wence-) - Mads R. B. Kristensen (https://github.com/madsbk) - Tom Augspurger (https://github.com/TomAugspurger) URL: NVIDIA#22996
Closes NVIDIA#23348 Fixes build/CI by linking RMM to cuDF testing libraries. Authors: - Abigale Kim (https://github.com/abigalekim) Approvers: - Kyle Edwards (https://github.com/KyleFromNVIDIA) - Muhammad Haseeb (https://github.com/mhaseeb123) - Bradley Dice (https://github.com/bdice) URL: NVIDIA#23341
davidwendt
added
improvement
github-actions
Bot
added
libcudf
📝 WalkthroughWalkthroughChangesThe pull request refactors mixed joins and join-index sizing, adds configurable streaming join filter pushdown, updates bloom-filter stream and cancellation handling, and fixes groupby metadata, nullable dtype preservation, and regex priority detection. Join implementation and sizing
Streaming bloom-filter lifecycle
Streaming join filter pushdown
Independent correctness fixes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
python/cudf/cudf/tests/groupby/test_cummulative.py (1)
110-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand cumulative-scan edge-case coverage.
These tests cover one nullable fixture and one non-null
int8fixture, but not empty, all-null, single-element, or mixed-type inputs. Add cases such as an all-null group and nullable unsigned integers to catch mask and cast regressions.As per coding guidelines, Python test files must cover empty, all-null, single-element, and mixed-type inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf/cudf/tests/groupby/test_cummulative.py` around lines 110 - 131, Expand the cumulative groupby scan tests around test_groupby_cumscan_masked_dtype_preserved and test_groupby_cumsum_numpy_dtype_promotes to cover empty, all-null, single-element, and mixed-type inputs, including nullable unsigned integer columns. Parameterize or add focused cases for both cumsum and cumprod where applicable, and continue comparing cudf results with pandas via assert_eq while preserving dtype and mask behavior.Source: Coding guidelines
cpp/src/join/mixed_join.cu (1)
110-111: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAllocate intermediate equality-match indices from the current device resource, not the output
mr. In both call sites theleft_indices/right_indicesreturned byequality_join_indicesare temporaries consumed byfilter_join_indices/filter_join_indices_output_size; only those final calls should use the caller-suppliedmr. Sizing the intermediate gather maps from the output resource can inflate/exhaust a pool sized for the result.
cpp/src/join/mixed_join.cu#L110-L111: passcudf::get_current_device_resource_ref()instead ofmrtoequality_join_indices.cpp/src/join/mixed_join.cu#L167-L168: passcudf::get_current_device_resource_ref()instead ofmrtoequality_join_indices.As per coding guidelines: "Returned allocations must use the passed memory resource; temporary allocations must use
cudf::get_current_device_resource_ref()."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/join/mixed_join.cu` around lines 110 - 111, Update both equality_join_indices call sites in cpp/src/join/mixed_join.cu at lines 110-111 and 167-168 to allocate temporary left_indices/right_indices using cudf::get_current_device_resource_ref() instead of mr; retain mr for the final filter_join_indices or filter_join_indices_output_size calls.Source: Coding guidelines
cpp/src/join/filter_join_indices/filter_join_indices.cu (2)
236-265: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
output_sizedoesn't skip the costly part of LEFT_JOIN sizing.For
LEFT_JOIN, thecuco::static_setbuild andinsert_if(lines 214-232) — which is the expensive part of size-counting — runs unconditionally regardless ofoutput_size; only the trailingcub::DeviceReduce::Sumis skipped whenoutput_size.has_value(). This is correct (the set is also needed later to locate unmatched rows), but it means the perf benefit promised by theoutput_sizeparameter's documentation ("skips the internal size-counting pass") is much smaller forLEFT_JOINthan forINNER_JOIN/FULL_JOIN. Consider clarifying the doc incpp/include/cudf/join/join.hpp(around theoutput_sizeparameter description) to note this LEFT_JOIN-specific caveat so callers don't overestimate the speedup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/join/filter_join_indices/filter_join_indices.cu` around lines 236 - 265, Clarify the output_size parameter documentation in join.hpp to state that, for LEFT_JOIN, the static_set construction and insert_if work still runs because it is required to identify unmatched rows; only the final size-reduction pass is skipped. Keep the existing description for INNER_JOIN and FULL_JOIN unchanged.
1-1: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClarify that
output_sizeonly partially skips size-counting forLEFT_JOIN.The
output_sizefast-path is fully effective forINNER_JOIN/FULL_JOIN(skipscount_ifentirely), but forLEFT_JOINthe costlycuco::static_setbuild/insert_ifstill runs unconditionally (it's needed to locate unmatched rows for materialization regardless ofoutput_size); only the trailingcub::DeviceReduce::Sumis skipped. The public doc doesn't call out this asymmetry, so callers may overestimate the perf win from precomputingoutput_sizeforLEFT_JOIN.
cpp/src/join/filter_join_indices/filter_join_indices.cu#L236-265: no code change needed; this is the root cause of the doc mismatch (cuco set build is unconditional here).cpp/include/cudf/include/cudf/join/join.hpp#L347-349: update the@param output_sizedoc to note that forLEFT_JOINonly the final size reduction is skipped, not the full size-counting work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/join/filter_join_indices/filter_join_indices.cu` at line 1, Update the public `@param` output_size documentation in the join API to clarify that precomputing output_size fully avoids size counting for INNER_JOIN and FULL_JOIN, but for LEFT_JOIN only skips the final size reduction; the cuco::static_set construction and insert_if work still runs. Do not change the implementation around filter_join_indices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf_polars/cudf_polars/utils/config.py`:
- Around line 454-458: Update the threshold documentation near
optimize_join_filter_pushdown to state that a filter is inserted on the
to-be-filtered table when the key-provider-rows to table-rows ratio is below the
threshold. Explicitly document that threshold=0 disables the rewrite, while
preserving the default value of 0.5.
---
Nitpick comments:
In `@cpp/src/join/filter_join_indices/filter_join_indices.cu`:
- Around line 236-265: Clarify the output_size parameter documentation in
join.hpp to state that, for LEFT_JOIN, the static_set construction and insert_if
work still runs because it is required to identify unmatched rows; only the
final size-reduction pass is skipped. Keep the existing description for
INNER_JOIN and FULL_JOIN unchanged.
- Line 1: Update the public `@param` output_size documentation in the join API to
clarify that precomputing output_size fully avoids size counting for INNER_JOIN
and FULL_JOIN, but for LEFT_JOIN only skips the final size reduction; the
cuco::static_set construction and insert_if work still runs. Do not change the
implementation around filter_join_indices.
In `@cpp/src/join/mixed_join.cu`:
- Around line 110-111: Update both equality_join_indices call sites in
cpp/src/join/mixed_join.cu at lines 110-111 and 167-168 to allocate temporary
left_indices/right_indices using cudf::get_current_device_resource_ref() instead
of mr; retain mr for the final filter_join_indices or
filter_join_indices_output_size calls.
In `@python/cudf/cudf/tests/groupby/test_cummulative.py`:
- Around line 110-131: Expand the cumulative groupby scan tests around
test_groupby_cumscan_masked_dtype_preserved and
test_groupby_cumsum_numpy_dtype_promotes to cover empty, all-null,
single-element, and mixed-type inputs, including nullable unsigned integer
columns. Parameterize or add focused cases for both cumsum and cumprod where
applicable, and continue comparing cudf results with pandas via assert_eq while
preserving dtype and mask behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9d77c084-56e5-481c-a109-c1aed59ba030
📒 Files selected for processing (57)
cpp/CMakeLists.txtcpp/include/cudf/detail/join/join.hppcpp/include/cudf/join/join.hppcpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hppcpp/libcudf_streaming/src/bloom_filter.cppcpp/libcudf_streaming/src/detail/device_bloom_filter.cucpp/src/join/filter_join_indices/filter_join_indices.cucpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel.cuhcpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel.hppcpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel_complex.cucpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel_null_complex.cucpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel_null_primitive.cucpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel_primitive.cucpp/src/join/mixed_filter_join_common_utils.cuhcpp/src/join/mixed_join.cucpp/src/join/mixed_join_common_utils.cuhcpp/src/join/mixed_join_kernel.cucpp/src/join/mixed_join_kernel.cuhcpp/src/join/mixed_join_kernel.hppcpp/src/join/mixed_join_kernel_nulls.cucpp/src/join/mixed_join_semi.cucpp/src/join/mixed_join_size_kernel.cucpp/src/join/mixed_join_size_kernel.cuhcpp/src/join/mixed_join_size_kernel.hppcpp/src/join/mixed_join_size_kernel_nulls.cucpp/src/strings/regex/glushkov_regcomp.cppcpp/src/strings/regex/glushkov_regcomp.hppcpp/tests/join/mixed_join_tests.cucpp/tests/streams/join_test.cppcpp/tests/strings/split_tests.cppdocs/cudf/source/cudf_polars/api.mddocs/cudf/source/cudf_polars/options.mdpython/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/groupby/test_agg.pypython/cudf/cudf/tests/groupby/test_cummulative.pypython/cudf_polars/cudf_polars/dsl/utils/column_domain.pypython/cudf_polars/cudf_polars/engine/core.pypython/cudf_polars/cudf_polars/engine/options.pypython/cudf_polars/cudf_polars/streaming/actor_graph/utils.pypython/cudf_polars/cudf_polars/streaming/explain.pypython/cudf_polars/cudf_polars/streaming/join.pypython/cudf_polars/cudf_polars/streaming/join_filter_pushdown.pypython/cudf_polars/cudf_polars/streaming/parallel.pypython/cudf_polars/cudf_polars/utils/config.pypython/cudf_polars/tests/dsl/test_column_domain.pypython/cudf_polars/tests/quent/test_quent.pypython/cudf_polars/tests/streaming/test_dataframescan.pypython/cudf_polars/tests/streaming/test_hstack.pypython/cudf_polars/tests/streaming/test_join.pypython/cudf_polars/tests/streaming/test_join_filter_pushdown.pypython/cudf_polars/tests/streaming/test_options.pypython/cudf_polars/tests/streaming/test_parallel.pypython/cudf_polars/tests/streaming/test_scan.pypython/cudf_polars/tests/test_config.pypython/cudf_streaming/cudf_streaming/bloom_filter.pyxpython/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py
💤 Files with no reviewable changes (9)
- cpp/src/join/mixed_join_size_kernel.hpp
- cpp/src/join/mixed_join_kernel.cu
- cpp/src/join/mixed_join_kernel.hpp
- cpp/src/join/mixed_join_size_kernel_nulls.cu
- cpp/src/join/mixed_join_kernel.cuh
- cpp/src/join/mixed_join_size_kernel.cuh
- cpp/src/join/mixed_join_kernel_nulls.cu
- cpp/src/join/mixed_join_size_kernel.cu
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Contributor
Author
|
Already a PR for this here: #23395 |
3 tasks
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes merge conflicts for #23390