Main merge release/26.08 - #23395
Merged
Merged
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
github-actions
Bot
added
libcudf
Contributor
Author
|
/merge nosquash |
wence-
added
improvement
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesJoin execution and filter sizing
Streaming bloom-filter lifecycle
Glushkov regex priority
Groupby metadata and dtypes
Streaming join-filter pushdown
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: 3
🧹 Nitpick comments (5)
cpp/tests/strings/split_tests.cpp (1)
520-547: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the regression with required string-column edge cases.
This new test only exercises one non-null, unsliced, small ASCII row. Add coverage for the required empty, null, sliced, boundary/multi-block, and non-ASCII inputs so the priority fix is validated across the relevant column layouts.
As per coding guidelines, C++ string tests must cover empty inputs, nulls, sliced columns, boundary and multi-block sizes, and non-ASCII UTF-8.
🤖 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/tests/strings/split_tests.cpp` around lines 520 - 547, Expand SplitRecordRegexLazyQuantifier to cover empty strings, null rows, sliced string columns, boundary and multi-block-sized inputs, and non-ASCII UTF-8 values. Reuse the existing lazy and greedy regex assertions across these layouts, ensuring expected outputs preserve nulls, slicing offsets, and correct split behavior for each case.Source: Coding guidelines
python/cudf/cudf/tests/groupby/test_agg.py (1)
809-821: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExpand coverage for the new MultiIndex metadata paths.
The current test covers only a non-empty frame with string-valued levels. It does not exercise the empty-value-column branch at GroupBy.agg Lines 1340-1354 or verify preservation of non-object
level_dtypesat Line 1373. Add a one-column MultiIndex frame grouped by its only column, plus a fixture with an integer-valued level.As per coding guidelines,
python/**/test_*.pytests should cover empty inputs and relevant edge cases.Suggested regression coverage
+def test_agg_empty_multiindex_columns_preserved(): + pdf = pd.DataFrame( + [[1], [1], [2]], + columns=pd.MultiIndex.from_tuples( + [("key", 0)], names=["l0", "l1"] + ), + ) + gdf = cudf.DataFrame(pdf) + + expect = pdf.groupby(("key", 0)).agg("sum") + got = gdf.groupby(("key", 0)).agg("sum") + assert_eq(expect, got)🤖 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_agg.py` around lines 809 - 821, Expand test_agg_multiindex_columns_preserved to cover an empty-value-column case by adding a one-column MultiIndex DataFrame grouped by its only column and comparing cudf with pandas. Also add coverage using an integer-valued MultiIndex level to verify non-object level_dtypes are preserved, while retaining the existing non-empty string-level case.Source: Coding guidelines
python/cudf/cudf/tests/groupby/test_cummulative.py (1)
110-120: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise cumulative state and unsigned nullable dtypes.
Each current group has at most one non-null value, so a scan that fails to carry state across rows could still pass. The implementation also covers unsigned extension dtypes, but the test only uses signed
Int16.As per coding guidelines, Python tests should cover relevant edge cases for nullable dtypes.
Suggested fixture expansion
`@pytest.mark.parametrize`("op", ["cumsum", "cumprod"]) -def test_groupby_cumscan_masked_dtype_preserved(op): +@pytest.mark.parametrize("dtype", ["Int16", "UInt16"]) +def test_groupby_cumscan_masked_dtype_preserved(op, dtype): pdf = pd.DataFrame( - {"a": [1, 1, 2], "b": [1, pd.NA, 2]}, dtype="Int16" + {"a": [1, 1, 1, 2], "b": [1, pd.NA, 2, 3]}, dtype=dtype )🤖 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 - 120, Expand test_groupby_cumscan_masked_dtype_preserved to include multiple non-null values within the same group, verifying cumulative state across rows, and parameterize coverage for unsigned nullable extension dtypes in addition to signed Int16. Keep expected results sourced from pandas and continue asserting dtype-preserving equality with cudf.Source: Coding guidelines
cpp/tests/streams/join_test.cpp (1)
151-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the
output_sizehint-bypass path.Only the
std::nulloptpath is exercised here. The hint-bypass arithmetic (especially the LEFT_JOIN/FULL_JOIN unsigned subtraction againstoutput_size) is a new, correctness-sensitive path that isn't tested with a real precomputed size in this file.✅ Suggested addition
TEST_F(JoinTest, LeftJoinWithPostFilter) { cudf::hash_join hash_joiner(table1, cudf::null_equality::EQUAL, cudf::test::get_default_stream()); auto hash_join_result = hash_joiner.left_join(table0, std::nullopt, cudf::test::get_default_stream()); auto hash_filter_result = cudf::filter_join_indices(conditional0, conditional1, cudf::device_span<cudf::size_type const>(*hash_join_result.first), cudf::device_span<cudf::size_type const>(*hash_join_result.second), left_zero_eq_right_zero, cudf::join_kind::LEFT_JOIN, std::nullopt, cudf::test::get_default_stream()); + + auto const output_size_result = + cudf::filter_join_indices_output_size(conditional0, + conditional1, + cudf::device_span<cudf::size_type const>(*hash_join_result.first), + cudf::device_span<cudf::size_type const>(*hash_join_result.second), + left_zero_eq_right_zero, + cudf::join_kind::LEFT_JOIN, + cudf::test::get_default_stream()); + + auto const hinted_result = + cudf::filter_join_indices(conditional0, + conditional1, + cudf::device_span<cudf::size_type const>(*hash_join_result.first), + cudf::device_span<cudf::size_type const>(*hash_join_result.second), + left_zero_eq_right_zero, + cudf::join_kind::LEFT_JOIN, + output_size_result.first, + cudf::test::get_default_stream()); }🤖 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/tests/streams/join_test.cpp` around lines 151 - 166, Add coverage in TEST_F(JoinTest, LeftJoinWithPostFilter) for the filter_join_indices output_size hint-bypass path by computing and passing a real precomputed output size instead of only std::nullopt. Exercise the LEFT_JOIN arithmetic with a valid output_size and assert the resulting indices match the existing no-hint result; include FULL_JOIN coverage if the shared test setup permits.cpp/src/join/mixed_join.cu (1)
109-111: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAllocate equality-index scratch from the current device resource, not the output
mr.equality_join_indicesproducesleft_indices/right_indicesthat are transient inputs to the subsequent filter step and are never returned; per guideline, temporaries must usecudf::get_current_device_resource_ref().
cpp/src/join/mixed_join.cu#L109-L111: passcudf::get_current_device_resource_ref()toequality_join_indiceshere so the scratch indices don't come from the caller's result resource.cpp/src/join/mixed_join.cu#L166-L168: apply the same change incompute_mixed_join_output_size.🤖 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 109 - 111, Update both equality_join_indices call sites in cpp/src/join/mixed_join.cu at lines 109-111 and 166-168 to pass cudf::get_current_device_resource_ref() for the transient equality-index allocations instead of the output mr resource.Source: Coding guidelines
🤖 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/engine/options.py`:
- Around line 358-360: Update from_dict so an explicit join_filter_pushdown=None
remains None rather than becoming UNSPECIFIED, preserving disabled behavior
across to_dict/from_dict round trips. Revise from_dict’s None-semantics
documentation and add a regression test covering this join_filter_pushdown round
trip.
In `@python/cudf_polars/cudf_polars/utils/config.py`:
- Around line 454-456: Correct the public threshold docstring wording so it
clearly states that a filter is inserted on the to-be-filtered table when the
key-provider-rows to to-be-filtered-table-rows ratio is below the threshold.
Preserve the existing default value and configuration semantics.
In `@python/cudf_streaming/cudf_streaming/bloom_filter.pyx`:
- Around line 212-216: Update the AllReduce path around await_cpp_future to
coordinate cancellation across ranks before entering the collective, rather than
only closing local channels via shutdown_channels. Ensure a failure or
cancellation propagates an abort/notification to peer ranks so no rank remains
blocked, and add a multi-rank regression test covering this failure scenario.
---
Nitpick comments:
In `@cpp/src/join/mixed_join.cu`:
- Around line 109-111: Update both equality_join_indices call sites in
cpp/src/join/mixed_join.cu at lines 109-111 and 166-168 to pass
cudf::get_current_device_resource_ref() for the transient equality-index
allocations instead of the output mr resource.
In `@cpp/tests/streams/join_test.cpp`:
- Around line 151-166: Add coverage in TEST_F(JoinTest, LeftJoinWithPostFilter)
for the filter_join_indices output_size hint-bypass path by computing and
passing a real precomputed output size instead of only std::nullopt. Exercise
the LEFT_JOIN arithmetic with a valid output_size and assert the resulting
indices match the existing no-hint result; include FULL_JOIN coverage if the
shared test setup permits.
In `@cpp/tests/strings/split_tests.cpp`:
- Around line 520-547: Expand SplitRecordRegexLazyQuantifier to cover empty
strings, null rows, sliced string columns, boundary and multi-block-sized
inputs, and non-ASCII UTF-8 values. Reuse the existing lazy and greedy regex
assertions across these layouts, ensuring expected outputs preserve nulls,
slicing offsets, and correct split behavior for each case.
In `@python/cudf/cudf/tests/groupby/test_agg.py`:
- Around line 809-821: Expand test_agg_multiindex_columns_preserved to cover an
empty-value-column case by adding a one-column MultiIndex DataFrame grouped by
its only column and comparing cudf with pandas. Also add coverage using an
integer-valued MultiIndex level to verify non-object level_dtypes are preserved,
while retaining the existing non-empty string-level case.
In `@python/cudf/cudf/tests/groupby/test_cummulative.py`:
- Around line 110-120: Expand test_groupby_cumscan_masked_dtype_preserved to
include multiple non-null values within the same group, verifying cumulative
state across rows, and parameterize coverage for unsigned nullable extension
dtypes in addition to signed Int16. Keep expected results sourced from pandas
and continue asserting dtype-preserving equality with cudf.
🪄 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: 9db777c9-a99c-472d-ad98-f2144da5ba59
📒 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 (10)
- cpp/CMakeLists.txt
- cpp/src/join/mixed_join_size_kernel_nulls.cu
- cpp/src/join/mixed_join_kernel_nulls.cu
- cpp/src/join/mixed_join_kernel.hpp
- cpp/src/join/mixed_join_size_kernel.hpp
- cpp/src/join/mixed_join_kernel.cu
- cpp/src/join/mixed_join_size_kernel.cu
- cpp/src/join/mixed_join_size_kernel.cuh
- cpp/src/join/mixed_join_kernel.cuh
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Member
|
Commit SHAs match what I see in #23390. There are some newer commits that have made it into that PR but that's fine, if they end up not being auto-mergeable we can just do this again. And @wence- informed me that these CUDA 12.2.2 issues are expected:
And fixed separately in #23392 no-squash merging this. |
3 tasks
This was referenced Jul 30, 2026
3 tasks
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
Fix conflicts for automerger
Checklist