Fix lazy quantifier priority handling in Glushkov regex engine - #23291
Closed
wjxiz1992 wants to merge 26 commits into
Closed
Fix lazy quantifier priority handling in Glushkov regex engine#23291wjxiz1992 wants to merge 26 commits into
wjxiz1992 wants to merge 26 commits into
Conversation
Signed-off-by: Allen Xu <allxu@nvidia.com>
wjxiz1992
marked this pull request as ready for review
July 16, 2026 08:45
wjxiz1992
requested review from
bdice,
Copilot and
igorpeshansky
and removed request for
Copilot
July 16, 2026 08:46
📝 WalkthroughWalkthroughThe PR updates cuDF to the 26.10 release, adds direct joins and Java artifact builds, changes CI timeout handling, and modifies regex, join, streaming, IO, dataframe, groupby, proxy, and rolling-expression behavior with corresponding tests. ChangesRelease and CI integration
Runtime features and compatibility
Glushkov lazy quantifier handling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
davidwendt
approved these changes
Jul 16, 2026
Contributor
|
/ok to test 588722c |
Contributor
|
@wjxiz1992 You will need to update the copyright as flagged here: https://github.com/rapidsai/cudf/actions/runs/29504984943/job/87643337773?pr=23291#step:7:348 |
Contributor
|
The fix looks good to me, thanks for fixing this |
Signed-off-by: Allen Xu <allxu@nvidia.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes an eligibility-check gap in the Glushkov regex fast path where certain Thompson-priority frontiers (notably involving ACCEPT preceding a later character-consuming transition) could cause lazy quantifiers to behave greedily, and adds a regression test to ensure correct split_record_re delimiter behavior.
Changes:
- Tightens Glushkov priority-conflict detection to reject frontiers where
ACCEPTis followed by a laterCHAR_POS, forcing fallback to the Thompson engine. - Documents the additional “priority frontier not representable by Glushkov bit ordering” rejection case.
- Adds a regression test covering
split_record_rewith a reluctant\r+?delimiter and a contrasting greedy\r+case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cpp/tests/strings/split_tests.cpp |
Adds SplitRecordRegexLazyQuantifier regression to validate split behavior differs between \r+? and \r+. |
cpp/src/strings/regex/glushkov_regcomp.hpp |
Updates eligibility documentation to mention Thompson-priority frontier conflicts. |
cpp/src/strings/regex/glushkov_regcomp.cpp |
Updates frontier_has_priority_conflict Rule 1 to reject ACCEPT followed by later CHAR_POS in Thompson priority order. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+281
to
+283
| * Rule 1 – END before later char: an ACCEPT item appears before a CHAR_POS | ||
| * item in Thompson priority order → the accepted path has higher | ||
| * priority than a continuation that Glushkov cannot kill correctly. |
Signed-off-by: Allen Xu <allxu@nvidia.com>
Contributor
Author
|
/ok to test c8151f0 |
Contributor
Author
|
blocked by main branch 26.10 ABI conflict. |
* Split the JAR build into three composable stages (static libcudf build, per-classifier JAR packaging, and Maven-repo gather) so each stage is independently runnable in CI and locally. * Link against a static libcudf built from source per CUDA version rather than a conda shared libcudf. * Emit the Maven classifier based on the host architecture the build runs on, introducing a new `-arm64` suffix to distinguish aarch64 JARs from their x86_64 counterparts. * Add `test_java_build_local.sh` as a one-command local reproducer of the full CI matrix for the host arch, with per-step timings and GPU compute-capability auto-detection. Contributes to NVIDIA#22204 Authors: - https://github.com/paul-aiyedun Approvers: - Mike Sarahan (https://github.com/msarahan) - Tim Liu (https://github.com/NvTimLiu) URL: NVIDIA#23261
…imeout (NVIDIA#23332) ## Description Timing out an individual test is actually not useful since it is not safe to cancel a test at an arbitrary point in execution: that might leave a collective dangling. Moreover, many of the Polars tests run quite close to the, arbitrarily chosen, timeout limit on CI runners if they are heavily loaded. Since the signal we actually want is the state of a hanging process, instead just add a test-suite level timeout with a utility to print the state of the hanging processes. closes NVIDIA#22948 ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes.
…sts (NVIDIA#23364) Split out of NVIDIA#23255 (1/6). `NumericalColumn.as_numerical_column` short-circuits casts between equivalent dtypes (same pylibcudf type, e.g. `float64` → `Float64`), but implemented the shortcut by assigning the target dtype onto `self._dtype` in place. The column object is shared with the caller's Series/DataFrame, so the *source* object silently changed dtype as a side effect of the cast. This returns a fresh column over the same pylibcudf data instead (`nans_to_nulls` first for float → masked casts), and adds a classic regression test. Fixes 5 pandas-tests (`test_stack_nullable_dtype[*]`, `test_loc_set_nan_in_categorical_series[Float64]`, `test_assert_series_equal_extension_dtype_mismatch`, `test_assert_frame_equal_extension_dtype_mismatch`); their xfail entries are removed. Attribution verified by running the node ids against an isolated build containing only this fix (they pass) and a clean build (they fail). Independent of the other NVIDIA#23255 split PRs; can merge in any order. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#23364
…IA#23001) Split out of NVIDIA#22927 per review. ## Problem A class-level attribute write on a cudf.pandas proxy type — e.g. `monkeypatch.setattr(pd.ExcelFile, "parse", fn)` — was only applied to the proxy class. Code that runs under `disable_module_accelerator()` (such as the pandas fallback path of `pd.read_excel`) resolves attributes from the *real* class, so a patch applied only to the proxy was invisible to it. ## Fix Add `_FastSlowProxyMeta.__setattr__`/`__delattr__` to mirror runtime class-level patches onto the underlying "slow" (real) type: - `__setattr__` mirrors the assignment after translating the assigned value into "slow" space. A plain value is forwarded as-is. Re-assigning the proxy's *pristine* attribute for a name (which is exactly what `monkeypatch.setattr` / `mock.patch.object` save and re-assign on undo) translates to the slow type's pristine attribute for that name — restored if the slow type had one of its own, or removed if it didn't (leaving any inherited implementation visible). Proxy machinery (e.g. a saved `pd.ExcelFile.parse`, a `_MethodProxy`) unwraps to the slow object it delegates to. - `__delattr__` mirrors a deletion as a deletion. Nothing is restored on delete. The pristine state is a per-type map `name -> (pristine proxy attribute, pristine slow class-dict entry)` snapshotted once, when `make_*_proxy_type` finishes building the type (the same point mirroring is enabled via `_fsproxy_mirror_slow_overrides`). It is a fixed translation table, not runtime patch tracking: there is no stash of "what to put back", and undo works for any code that follows the standard save/patch/re-assign pattern (pytest `monkeypatch`, `unittest.mock.patch.object`, manual saves) because the saved value itself identifies the pristine state. The translation is what makes mirroring safe at all — the values readable off a proxy type live in proxy space, and forwarding e.g. the saved `columns` property or `eval`/`query` functions verbatim onto `pandas.DataFrame` would install cudf machinery on the real class (for `columns` this infinitely recurses on the fallback path). cudf.pandas's own custom methods (`DataFrame.eval`/`query`) are installed via the new `_setattr_fsproxy_no_mirror` helper, which registers them as part of the proxy's pristine state without forwarding them to pandas. ## Tests Adds unit tests in `cudf_pandas_tests/test_fast_slow_proxy.py` covering: set/delete mirroring, monkeypatch round-trips (new attr, existing attr, nested, `delattr`), `mock.patch.object` (which saves the raw descriptor without resolving it), properties, plain data attributes, `staticmethod`/`classmethod` descriptor preservation, methods the slow type only inherits, and the no-mirror helper; plus an end-to-end test in `test_cudf_pandas.py` that patches/unpatches `DataFrame.columns`/`eval` and `Series.str` and checks real pandas is restored and functional. Removes 14 now-passing xfails from the pandas-tests plugin (13× `read_excel` engine-selection tests that monkeypatch the engine, plus a monkeypatch-registered custom accessor). The attribution of these 14 to the proxy fix (vs the Excel-reader fixes remaining in NVIDIA#22927) was verified locally by running each removed xfail with the proxy fix in isolation. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#23001
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
github-actions
Bot
added
Python
Contributor
|
Close in favor of #23381 since I failed the rebase on release/26.08. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.github/workflows/build.yaml (1)
475-544: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding
java-build/java-gatherruntime.These new jobs build static libcudf from scratch across 4 matrix combinations without a
timeout-minutes, unlike jobs routed throughcustom-job.yaml(which typically applies a default). A hang in the from-source build would consume the GitHub default (360 min) per matrix leg before failing.⏱️ Suggested addition
java-build: needs: [telemetry-setup] + timeout-minutes: 120 strategy:🤖 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 @.github/workflows/build.yaml around lines 475 - 544, Add explicit timeout-minutes limits to the java-build matrix job and java-gather job. Use a suitable bound for the from-source libcudf builds and a shorter appropriate bound for artifact assembly, ensuring hung jobs terminate without relying on GitHub’s default timeout.ci/timeout_with_stack.py (1)
292-298: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer
start_new_session=Trueoverpreexec_fn=os.setsid.
preexec_fnis documented as unsafe with threads and can deadlock the child beforeexec;start_new_session=Trueachieves the samesetsid()effect without that risk, and is exactly what Ruff'sPLW1509rule recommends.🛡️ Suggested fix
process = subprocess.Popen( cmd, - preexec_fn=os.setsid, + start_new_session=True, )As per coding guidelines,
**/*.pychanges should pass the configured Ruff linters, which include this exact rule.🤖 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 `@ci/timeout_with_stack.py` around lines 292 - 298, Update the subprocess.Popen call in the process-launching code to replace preexec_fn=os.setsid with start_new_session=True, preserving creation of a new process session and group while satisfying Ruff PLW1509. Remove the now-unneeded preexec_fn-specific comments or adjust them to describe the new option.Source: Coding guidelines
python/cudf/cudf/core/index.py (1)
811-907: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDense dtype-reconciliation logic in
Index.union— recommend dedicated test coverage.The new
dtype_ignored/promote_dtypeinterplay correctly handles the empty-RangeIndex/object-dtype pandas quirk (GH pandas-dev/pandas#60797) across all four return branches based on a manual trace, but the branch combinations (empty+ignorable vs. empty+non-ignorable, numeric promotion, same-kind datetime/timedelta unit promotion, tz-aware vs. tz-naive) are numerous and easy to regress silently. As per coding guidelines forpython/**/*.{py,pyx}, this touches type-promotion rules that "match expected behavior" — worth adding focused parametrized tests for each dtype_ignored/promote_dtype combination if not already covered elsewhere in this PR.🤖 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/core/index.py` around lines 811 - 907, Add focused parametrized tests for Index.union covering each dtype_ignored and promote_dtype combination: empty RangeIndex/object-dtype versus non-empty operands, numeric promotion, same-kind datetime/timedelta unit promotion, and timezone-aware versus timezone-naive inputs. Assert both result values and dtypes across the empty, equal, and non-empty union branches, preserving the expected pandas-compatible behavior.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 `@cpp/include/cudf/detail/utilities/device_atomics.cuh`:
- Around line 356-366: Update the documentation for the __int128_t atomic
addition near its architecture-specific version gate to accurately describe all
CUDA versions excluded from the native Hopper/Blackwell path, including CUDA
13.0, 13.1, and 13.2. Keep the wording synchronized with the existing
preprocessor condition rather than changing the implementation.
In `@java/ci/README.md`:
- Around line 47-51: Declare the language on both directory-tree output fences
in java/ci/README.md: change the opening fences at lines 47-51 and 75-80 to use
the text language.
- Around line 43-50: Replace release-specific artifact names and paths in
java/ci/README.md lines 43-50 and 75-79 with version-neutral `<version>`
placeholders. Update both the classifier JAR/POM example and the Maven
repository path/filenames consistently, preserving the documented directory
structure.
In `@python/cudf/cudf/tests/groupby/test_ffill.py`:
- Around line 66-85: Update test_groupby_fill_limit to parameterize only valid
limits [1, 2, None] for result comparisons, and add a separate test covering
limit values 0 and -1 that asserts both pandas and cuDF groupby fill methods
raise ValueError. Preserve coverage for both ffill and bfill and the existing
value cases.
---
Nitpick comments:
In @.github/workflows/build.yaml:
- Around line 475-544: Add explicit timeout-minutes limits to the java-build
matrix job and java-gather job. Use a suitable bound for the from-source libcudf
builds and a shorter appropriate bound for artifact assembly, ensuring hung jobs
terminate without relying on GitHub’s default timeout.
In `@ci/timeout_with_stack.py`:
- Around line 292-298: Update the subprocess.Popen call in the process-launching
code to replace preexec_fn=os.setsid with start_new_session=True, preserving
creation of a new process session and group while satisfying Ruff PLW1509.
Remove the now-unneeded preexec_fn-specific comments or adjust them to describe
the new option.
In `@python/cudf/cudf/core/index.py`:
- Around line 811-907: Add focused parametrized tests for Index.union covering
each dtype_ignored and promote_dtype combination: empty RangeIndex/object-dtype
versus non-empty operands, numeric promotion, same-kind datetime/timedelta unit
promotion, and timezone-aware versus timezone-naive inputs. Assert both result
values and dtypes across the empty, equal, and non-empty union branches,
preserving the expected pandas-compatible 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: fdb5a662-90f7-4dfc-b88d-8278831d6fea
📒 Files selected for processing (106)
.devcontainer/cuda12.9-conda/devcontainer.json.devcontainer/cuda12.9-pip/devcontainer.json.devcontainer/cuda13.3-conda/devcontainer.json.devcontainer/cuda13.3-pip/devcontainer.json.github/workflows/build.yaml.github/workflows/compute-sanitizer-run.yaml.github/workflows/pandas-tests.yaml.github/workflows/pr.yaml.github/workflows/test.yaml.pre-commit-config.yamlVERSIONci/run_cudf_polars_polars_tests.shci/run_cudf_polars_pytests.shci/test_cuml_compat.shci/test_narwhals.shci/test_wheel_cudf_polars.shci/timeout_with_stack.pyconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-133_arch-aarch64.yamlconda/environments/all_cuda-133_arch-x86_64.yamlconda/recipes/cudf/recipe.yamlconda/recipes/pylibcudf/recipe.yamlcpp/CMakeLists.txtcpp/benchmarks/CMakeLists.txtcpp/benchmarks/join/direct_join.cucpp/include/cudf/detail/utilities/device_atomics.cuhcpp/include/cudf/io/avro.hppcpp/include/cudf/io/csv.hppcpp/include/cudf/io/json.hppcpp/include/cudf/io/orc.hppcpp/include/cudf/join/direct_join.hppcpp/include/cudf/join/join.hppcpp/libcudf_streaming/benchmarks/bench_shuffle.cppcpp/libcudf_streaming/benchmarks/streaming/bench_streaming_shuffle.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/utils.cppcpp/libcudf_streaming/tests/streaming/base_streaming_fixture.hppcpp/libcudf_streaming/tests/streaming/test_table_chunk.cppcpp/libcudf_streaming/tests/test_shuffler.cppcpp/src/join/cross_join.cucpp/src/join/direct_join.cucpp/src/row_operator/row_operators.cucpp/tests/CMakeLists.txtcpp/tests/groupby/sum_tests.cppcpp/tests/join/cross_join_tests.cppcpp/tests/join/direct_join_tests.cppcpp/tests/streams/join_test.cppdependencies.yamljava/ci/README.mdjava/ci/argparse.shjava/ci/assemble_maven_repo.shjava/ci/build_cudf_java_jar.shjava/ci/build_cudf_java_jar_in_container.shjava/ci/build_static_libcudf.shjava/ci/build_static_libcudf_in_container.shjava/ci/test_java_build_local.shjava/pom.xmlpython/cudf/benchmarks/internal/bench_fast_slow_proxy.pypython/cudf/cudf/VERSIONpython/cudf/cudf/core/column/column.pypython/cudf/cudf/core/column/numerical.pypython/cudf/cudf/core/column/string.pypython/cudf/cudf/core/dataframe.pypython/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/core/index.pypython/cudf/cudf/core/indexed_frame.pypython/cudf/cudf/core/join/join.pypython/cudf/cudf/core/series.pypython/cudf/cudf/pandas/_wrappers/pandas.pypython/cudf/cudf/pandas/fast_slow_proxy.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/dataframe/methods/test_reductions.pypython/cudf/cudf/tests/dataframe/methods/test_rename.pypython/cudf/cudf/tests/dataframe/test_constructors.pypython/cudf/cudf/tests/groupby/test_agg.pypython/cudf/cudf/tests/groupby/test_apply.pypython/cudf/cudf/tests/groupby/test_ffill.pypython/cudf/cudf/tests/groupby/test_pct_change.pypython/cudf/cudf/tests/groupby/test_transform.pypython/cudf/cudf/tests/reshape/test_merge.pypython/cudf/cudf/tests/series/methods/test_astype.pypython/cudf/cudf/tests/series/methods/test_mode.pypython/cudf/cudf/tests/series/methods/test_reductions.pypython/cudf/cudf/utils/dtypes.pypython/cudf/cudf_pandas_tests/test_cudf_pandas.pypython/cudf/cudf_pandas_tests/test_fast_slow_proxy.pypython/cudf/cudf_pandas_tests/third_party_integration_tests/dependencies.yamlpython/cudf/pyproject.tomlpython/cudf_kafka/pyproject.tomlpython/cudf_polars/cudf_polars/dsl/expr.pypython/cudf_polars/cudf_polars/dsl/expressions/rolling.pypython/cudf_polars/cudf_polars/dsl/translate.pypython/cudf_polars/cudf_polars/engine/core.pypython/cudf_polars/pyproject.tomlpython/cudf_polars/tests/conftest.pypython/cudf_polars/tests/expressions/test_fixed_rolling.pypython/cudf_polars/tests/expressions/test_rolling.pypython/cudf_polars/tests/streaming/test_scan.pypython/cudf_polars/tests/streaming/test_select.pypython/cudf_polars/tests/streaming/test_sort.pypython/cudf_streaming/pyproject.tomlpython/custreamz/pyproject.tomlpython/dask_cudf/pyproject.tomlpython/libcudf/pyproject.tomlpython/libcudf_streaming/pyproject.tomlpython/pylibcudf/pyproject.toml
💤 Files with no reviewable changes (4)
- ci/test_cuml_compat.sh
- python/cudf_polars/tests/streaming/test_sort.py
- python/cudf_polars/tests/expressions/test_rolling.py
- python/cudf_polars/tests/streaming/test_scan.py
Comment on lines
356
to
366
| /** | ||
| * @brief Atomic addition for __int128_t with architecture-specific optimization | ||
| * | ||
| * Uses native 128-bit CAS on Hopper+ GPUs (compute capability 9.0+) for optimal | ||
| * performance. Falls back to two 64-bit atomic CAS operations with carry propagation | ||
| * on older GPU architectures. | ||
| * Uses native 128-bit CAS on Hopper+ GPUs for optimal performance, except for CUDA 13.0 on | ||
| * Blackwell. Other configurations fall back to two 64-bit atomic additions with carry | ||
| * propagation. | ||
| * | ||
| * @param address Pointer to the __int128_t value | ||
| * @param val Value to add | ||
| * @return The old value before addition | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Docstring doesn't match the actual version gate.
The doc says the native path is skipped "except for CUDA 13.0 on Blackwell," but the #if condition on lines 371-372 excludes the native path for __CUDACC_VER_MAJOR__ == 13 && __CUDACC_VER_MINOR__ < 3, i.e. CUDA 13.0, 13.1, and 13.2. Update the comment to match the code (or vice versa) so a future contributor narrowing/widening the gate doesn't work from stale documentation.
📝 Proposed doc fix
- * Uses native 128-bit CAS on Hopper+ GPUs for optimal performance, except for CUDA 13.0 on
+ * Uses native 128-bit CAS on Hopper+ GPUs for optimal performance, except for CUDA 13.0-13.2 on
* Blackwell. Other configurations fall back to two 64-bit atomic additions with carry
* propagation.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * @brief Atomic addition for __int128_t with architecture-specific optimization | |
| * | |
| * Uses native 128-bit CAS on Hopper+ GPUs (compute capability 9.0+) for optimal | |
| * performance. Falls back to two 64-bit atomic CAS operations with carry propagation | |
| * on older GPU architectures. | |
| * Uses native 128-bit CAS on Hopper+ GPUs for optimal performance, except for CUDA 13.0 on | |
| * Blackwell. Other configurations fall back to two 64-bit atomic additions with carry | |
| * propagation. | |
| * | |
| * @param address Pointer to the __int128_t value | |
| * @param val Value to add | |
| * @return The old value before addition | |
| */ | |
| /** | |
| * `@brief` Atomic addition for __int128_t with architecture-specific optimization | |
| * | |
| * Uses native 128-bit CAS on Hopper+ GPUs for optimal performance, except for CUDA 13.0-13.2 on | |
| * Blackwell. Other configurations fall back to two 64-bit atomic additions with carry | |
| * propagation. | |
| * | |
| * `@param` address Pointer to the __int128_t value | |
| * `@param` val Value to add | |
| * `@return` The old value before addition | |
| */ |
🤖 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/include/cudf/detail/utilities/device_atomics.cuh` around lines 356 - 366,
Update the documentation for the __int128_t atomic addition near its
architecture-specific version gate to accurately describe all CUDA versions
excluded from the native Hopper/Blackwell path, including CUDA 13.0, 13.1, and
13.2. Keep the wording synchronized with the existing preprocessor condition
rather than changing the implementation.
Comment on lines
+43
to
+50
| This compiles the JNI layer against the static libcudf from Step 1 and emits a | ||
| single classifier JAR (e.g. `cudf-26.08.0-SNAPSHOT-cuda12.jar`) plus its POM | ||
| into a classifier-named subdirectory under `--output-dir`: | ||
|
|
||
| ``` | ||
| /tmp/jars/cuda12/ | ||
| cudf-26.08.0-SNAPSHOT-cuda12.jar | ||
| cudf-26.08.0-SNAPSHOT.pom |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace release-specific artifact examples with version-neutral paths.
The new recommended workflow documents 26.08.0-SNAPSHOT, while this release cohort updates configuration to 26.10. Use <version> consistently so the instructions do not become stale each release.
java/ci/README.md#L43-L50: replace the hard-coded JAR and POM examples.java/ci/README.md#L75-L79: replace the hard-coded Maven repository path and filenames.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 1 file
java/ci/README.md#L43-L50(this comment)java/ci/README.md#L75-L79
🤖 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 `@java/ci/README.md` around lines 43 - 50, Replace release-specific artifact
names and paths in java/ci/README.md lines 43-50 and 75-79 with version-neutral
`<version>` placeholders. Update both the classifier JAR/POM example and the
Maven repository path/filenames consistently, preserving the documented
directory structure.
Comment on lines
+47
to
+51
| ``` | ||
| /tmp/jars/cuda12/ | ||
| cudf-26.08.0-SNAPSHOT-cuda12.jar | ||
| cudf-26.08.0-SNAPSHOT.pom | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Declare a language for the directory-tree fences.
markdownlint reports MD040 for both output examples; label them text.
java/ci/README.md#L47-L51: change the opening fence to```text.java/ci/README.md#L75-L80: change the opening fence to```text.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 1 file
java/ci/README.md#L47-L51(this comment)java/ci/README.md#L75-L80
🤖 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 `@java/ci/README.md` around lines 47 - 51, Declare the language on both
directory-tree output fences in java/ci/README.md: change the opening fences at
lines 47-51 and 75-80 to use the text language.
Source: Linters/SAST tools
Comment on lines
+66
to
+85
| @pytest.mark.parametrize("method", ["ffill", "bfill"]) | ||
| @pytest.mark.parametrize("limit", [0, 1, 2, -1, None]) | ||
| @pytest.mark.parametrize( | ||
| "values", | ||
| [ | ||
| [1.0, None, None, None, 2.0, None, None], | ||
| ["x", None, None, None, "y", None, None], | ||
| ], | ||
| ) | ||
| def test_groupby_fill_limit(method, limit, values): | ||
| # interleaved keys: limit counts group-relative positions | ||
| keys = ["a", "b"] * 7 | ||
| data = {"key": keys, "val": [v for v in values for _ in range(2)]} | ||
| pdf = pd.DataFrame(data) | ||
| gdf = cudf.DataFrame(data) | ||
|
|
||
| expect = getattr(pdf.groupby("key"), method)(limit=limit) | ||
| got = getattr(gdf.groupby("key"), method)(limit=limit) | ||
|
|
||
| assert_groupby_results_equal(expect, got) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Separate invalid limits from result-comparison cases.
limit=0 and limit=-1 cause pandas fill operations to raise ValueError, so Line 82 aborts those parametrizations before cuDF is compared. Keep [1, 2, None] here and add a separate exception-parity test for invalid limits. Pandas 3.0 validates that a limit must be greater than zero. (raw.githubusercontent.com)
Proposed fix
-@pytest.mark.parametrize("limit", [0, 1, 2, -1, None])
+@pytest.mark.parametrize("limit", [1, 2, None])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.parametrize("method", ["ffill", "bfill"]) | |
| @pytest.mark.parametrize("limit", [0, 1, 2, -1, None]) | |
| @pytest.mark.parametrize( | |
| "values", | |
| [ | |
| [1.0, None, None, None, 2.0, None, None], | |
| ["x", None, None, None, "y", None, None], | |
| ], | |
| ) | |
| def test_groupby_fill_limit(method, limit, values): | |
| # interleaved keys: limit counts group-relative positions | |
| keys = ["a", "b"] * 7 | |
| data = {"key": keys, "val": [v for v in values for _ in range(2)]} | |
| pdf = pd.DataFrame(data) | |
| gdf = cudf.DataFrame(data) | |
| expect = getattr(pdf.groupby("key"), method)(limit=limit) | |
| got = getattr(gdf.groupby("key"), method)(limit=limit) | |
| assert_groupby_results_equal(expect, got) | |
| `@pytest.mark.parametrize`("method", ["ffill", "bfill"]) | |
| `@pytest.mark.parametrize`("limit", [1, 2, None]) | |
| `@pytest.mark.parametrize`( | |
| "values", | |
| [ | |
| [1.0, None, None, None, 2.0, None, None], | |
| ["x", None, None, None, "y", None, None], | |
| ], | |
| ) | |
| def test_groupby_fill_limit(method, limit, values): | |
| # interleaved keys: limit counts group-relative positions | |
| keys = ["a", "b"] * 7 | |
| data = {"key": keys, "val": [v for v in values for _ in range(2)]} | |
| pdf = pd.DataFrame(data) | |
| gdf = cudf.DataFrame(data) | |
| expect = getattr(pdf.groupby("key"), method)(limit=limit) | |
| got = getattr(gdf.groupby("key"), method)(limit=limit) | |
| assert_groupby_results_equal(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_ffill.py` around lines 66 - 85, Update
test_groupby_fill_limit to parameterize only valid limits [1, 2, None] for
result comparisons, and add a separate test covering limit values 0 and -1 that
asserts both pandas and cuDF groupby fill methods raise ValueError. Preserve
coverage for both ffill and bfill and the existing value cases.
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
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
Closes #23287.
The Glushkov eligibility checker previously rejected an
ACCEPTitem 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 insplit_record_re.This PR:
ACCEPTitem is followed by a laterCHAR_POS, conservatively falling back to the Thompson engine;ACCEPT;StringsSplitTest.SplitRecordRegexLazyQuantifierto 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
StringsSplitTest.SplitRecordRegexLazyQuantifier: 1/1 passed.LIBCUDF_DISABLE_GLUSHKOV=1: 1/1 passed.STRINGS_TEST: 540/540 passed.spark-rapids-jnipackage using this cuDF checkout:BUILD SUCCESS; a second same-toolchain rebuild also completed successfully.RegularExpressionTranspilerSuite: 97 succeeded, 0 failed, 6 pre-existing canceled tests; MavenBUILD SUCCESS. The originalstring split fuzz - anchor focusedfailure passed.Checklist