Add additional regex pytests derived from cudf-spark integration tests - #23065
Merged
Conversation
davidwendt
added
improvement
github-actions
Bot
added
Python
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR expands regex test coverage in two pylibcudf test modules (test_string_contains.py, test_string_replace_re.py) by adding a shared ChangesSpark regex test coverage
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
python/pylibcudf/tests/test_string_spark_regex.py (1)
70-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a shared assertion helper to reduce duplication.
Nearly every
test_contains_re_*andtest_replace_re_*function repeats the identical 4-line pattern of buildinggot, computingexpectvia pyarrow, and callingassert_column_eq. Extracting a small helper (e.g.,_assert_contains_matches(strings, pattern)/_assert_replace_matches(strings, pattern, repl)) would reduce duplication across ~13 test functions and make future changes (e.g., swapping the comparison library) a one-line edit.♻️ Example helper extraction
+def _assert_contains_matches(strings, pattern): + got = plc.strings.contains.contains_re( + plc.Column.from_arrow(strings), + _make_prog(pattern), + ) + expect = pc.match_substring_regex(strings, pattern) + assert_column_eq(expect, got) + + def test_contains_re_basic(spark_strings, pattern): - got = plc.strings.contains.contains_re( - plc.Column.from_arrow(spark_strings), - _make_prog(pattern), - ) - expect = pc.match_substring_regex(spark_strings, pattern) - assert_column_eq(expect, got) + _assert_contains_matches(spark_strings, pattern)Also applies to: 243-380
🤖 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/pylibcudf/tests/test_string_spark_regex.py` around lines 70 - 227, The test functions repeat the same build/compare/assert flow, so extract shared assertion helpers to reduce duplication. Add a small helper around the repeated pattern in test_contains_re_* (and the related test_replace_re_* group mentioned in the review) that takes the input column/pattern and performs the got/expect/assert_column_eq steps. Update the existing tests to call the helper, using the existing symbols _make_prog, plc.strings.contains.contains_re, pc.match_substring_regex, and assert_column_eq so the behavior stays unchanged.
🤖 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.
Nitpick comments:
In `@python/pylibcudf/tests/test_string_spark_regex.py`:
- Around line 70-227: The test functions repeat the same build/compare/assert
flow, so extract shared assertion helpers to reduce duplication. Add a small
helper around the repeated pattern in test_contains_re_* (and the related
test_replace_re_* group mentioned in the review) that takes the input
column/pattern and performs the got/expect/assert_column_eq steps. Update the
existing tests to call the helper, using the existing symbols _make_prog,
plc.strings.contains.contains_re, pc.match_substring_regex, and assert_column_eq
so the behavior stays unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8b2131e8-b868-4cbe-9a6c-347ab70e76a3
📒 Files selected for processing (1)
python/pylibcudf/tests/test_string_spark_regex.py
mroeschke
reviewed
Jul 1, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/pylibcudf/tests/test_string_contains.py (1)
114-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the repeated test bodies.
test_contains_re_basic,_alternation,_anchors_wildcards,_bounded_repetition,_groups,_char_classes, and_escape_edge_casesall share an identical 5-line body, differing only in the parametrize pattern list. Could combine into a single parametrized test (or a shared helper function called by each) while keeping the category comments for documentation purposes.♻️ Example consolidation
+def _assert_contains_re(spark_strings, pattern): + got = plc.strings.contains.contains_re( + plc.Column.from_arrow(spark_strings), + _make_prog(pattern), + ) + expect = pc.match_substring_regex(spark_strings, pattern) + assert_column_eq(expect, got) + + # Basic quantifiers (test_rlike, test_regexp, test_regexp_like) `@pytest.mark.parametrize`( "pattern", [ "a{2}", "a{1,3}", "a{1,}", "a[bc]d", ], ) def test_contains_re_basic(spark_strings, pattern): - got = plc.strings.contains.contains_re( - plc.Column.from_arrow(spark_strings), - _make_prog(pattern), - ) - expect = pc.match_substring_regex(spark_strings, pattern) - assert_column_eq(expect, got) + _assert_contains_re(spark_strings, pattern)Also applies to: 263-280
🤖 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/pylibcudf/tests/test_string_contains.py` around lines 114 - 249, Consolidate the duplicated regex assertion logic across test_contains_re_basic, test_contains_re_alternation, test_contains_re_anchors_wildcards, test_contains_re_bounded_repetition, test_contains_re_groups, and test_contains_re_char_classes by moving the shared contains_re/pc.match_substring_regex/assert_column_eq body into one parametrized test or a small helper. Keep the existing category comments for readability, but avoid repeating the same setup and assertion block in each test function; use the unique test names and _make_prog to preserve current behavior.python/pylibcudf/tests/test_string_replace_re.py (1)
53-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
spark_stringsfixture across test modules.This fixture is identical to the one in
test_string_contains.py(Lines 75-109 per provided context). Consider hoisting it into a sharedconftest.pyfixture to avoid maintaining two copies in sync.🤖 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/pylibcudf/tests/test_string_replace_re.py` around lines 53 - 89, The spark_strings fixture is duplicated in multiple test modules, so move the shared array fixture into a common conftest.py and remove the local copy from test_string_replace_re.py. Keep the fixture name spark_strings so existing tests continue to use it without changes, and update any imports or module-level references if needed.
🤖 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.
Nitpick comments:
In `@python/pylibcudf/tests/test_string_contains.py`:
- Around line 114-249: Consolidate the duplicated regex assertion logic across
test_contains_re_basic, test_contains_re_alternation,
test_contains_re_anchors_wildcards, test_contains_re_bounded_repetition,
test_contains_re_groups, and test_contains_re_char_classes by moving the shared
contains_re/pc.match_substring_regex/assert_column_eq body into one parametrized
test or a small helper. Keep the existing category comments for readability, but
avoid repeating the same setup and assertion block in each test function; use
the unique test names and _make_prog to preserve current behavior.
In `@python/pylibcudf/tests/test_string_replace_re.py`:
- Around line 53-89: The spark_strings fixture is duplicated in multiple test
modules, so move the shared array fixture into a common conftest.py and remove
the local copy from test_string_replace_re.py. Keep the fixture name
spark_strings so existing tests continue to use it without changes, and update
any imports or module-level references if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 24b4b88b-b586-42a5-8be9-b2faf275b9fc
📒 Files selected for processing (2)
python/pylibcudf/tests/test_string_contains.pypython/pylibcudf/tests/test_string_replace_re.py
mroeschke
approved these changes
Jul 1, 2026
Contributor
|
/merge |
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
3 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
Adds an additional set of regex pytests based on the cudf-spark test suite.
This is split out from the work in #21936 since it is the only python change there.
Checklist