Removed deprecated multi-pattern overload of replace_re - #22639
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Breaking label still breaking the "Trigger Breaking Change Notifications" GHA when PR is in draft. |
|
This change will help remove some complexity from #21936 |
|
/ok to test |
|
/ok to test |
|
/ok to test |
KyleFromNVIDIA
left a comment
There was a problem hiding this comment.
Approved trivial CMake changes
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request removes the multi-pattern regex replacement API across C++, Java, and Python, narrowing replace_re to accept a single precompiled ChangesMulti-pattern regex replacement removal
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cudf/core/accessors/string.py`:
- Around line 1035-1038: The string accessor replace implementation now raises
ValueError when list-like pat/repl are used together with regex=True; update the
docstring for the replace method in cudf/core/accessors/string.py
(Series.str.replace) to explicitly document that list-like pat/repl are not
supported when regex=True and that regex defaults to True, and add a cudf-level
regression test (e.g., in tests/string/test_replace.py) that calls
Series.str.replace([...], [...], regex=True) and asserts it raises ValueError to
prevent regressions.
In `@python/pylibcudf/pylibcudf/strings/replace_re.pyx`:
- Around line 61-70: The code currently casts replacement to a string_scalar*
without validating its type, which can cause UB; before entering the with nogil:
block in replace_re, check that replacement is either None or a string Scalar
(e.g., ensure replacement.type corresponds to string/string_scalar) and raise a
Python-level TypeError if not, then only pass
dereference(<string_scalar*>(replacement.get())) to cpp_replace_re.replace_re
when the check has succeeded (or alternatively change the Cython signature to
accept a string_scalar specifically); ensure the validation happens on the
Python side before the with nogil: to avoid performing type checks without the
GIL.
🪄 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: b98da430-97e4-4a65-a72e-a6d9f9e20205
📒 Files selected for processing (13)
cpp/CMakeLists.txtcpp/include/cudf/strings/replace_re.hppcpp/src/strings/replace/multi_re.cujava/src/main/java/ai/rapids/cudf/ColumnView.javajava/src/main/native/src/ColumnViewJni.cppjava/src/test/java/ai/rapids/cudf/ColumnVectorTest.javapython/cudf/cudf/core/accessors/string.pypython/cudf/cudf/core/column/string.pypython/pylibcudf/pylibcudf/libcudf/strings/replace_re.pxdpython/pylibcudf/pylibcudf/strings/replace_re.pxdpython/pylibcudf/pylibcudf/strings/replace_re.pyipython/pylibcudf/pylibcudf/strings/replace_re.pyxpython/pylibcudf/tests/test_string_replace_re.py
💤 Files with no reviewable changes (8)
- cpp/CMakeLists.txt
- cpp/include/cudf/strings/replace_re.hpp
- python/pylibcudf/pylibcudf/libcudf/strings/replace_re.pxd
- python/pylibcudf/pylibcudf/strings/replace_re.pyi
- cpp/src/strings/replace/multi_re.cu
- java/src/main/native/src/ColumnViewJni.cpp
- java/src/test/java/ai/rapids/cudf/ColumnVectorTest.java
- java/src/main/java/ai/rapids/cudf/ColumnView.java
|
@mythrocks (@sameerz ) We are planning to remove the JNI binding and would like your feedback. |
|
Actionable comments posted: 0 |
|
/merge |
Description
Removes the multi-pattern version of the
cudf::strings::replace_reAPI which was deprecated in 26.06Checklist