Return null for non-participating regex capture groups in strings::extract by galipremsagar · Pull Request #23123 · rapidsai/cudf

galipremsagar · 2026-07-06T17:40:40Z

Description

Fixes the tests/strings/test_extract.py capture-group failures under cudf.pandas (e.g. test_extract_dataframe_capture_groups_index, 135 parametrizations): a capture group that did not participate in the overall match (such as an unmatched optional group, (\d)? against "C") left its range uninitialized but regexec returned it as a valid match, so strings::extract produced an empty string where pandas produces null. The final return now requires a valid range for sub-groups; a group that participates with an empty match ((\d)*) still returns "". Adds a libcudf gtest and a cudf Python test. The remaining string=object failures in that file are an unrelated, inherent object-dtype None-vs-NaN representation difference and stay in the xfail list.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

A capture group that does not participate in the overall match (e.g. an unmatched optional group) left its range uninitialized but was returned as a valid empty match, so strings::extract produced an empty string where pandas produces a null.

copy-pr-bot · 2026-07-06T17:40:43Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

galipremsagar · 2026-07-06T17:41:45Z

/okay to test a17fa3a

coderabbitai · 2026-07-06T17:44:48Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2f783e22-8e2e-4cde-b390-8914f6faea03

📥 Commits

Reviewing files that changed from the base of the PR and between 8837536 and 5870411.

📒 Files selected for processing (1)

python/cudf/cudf/tests/series/accessors/test_str.py

🚧 Files skipped from review as they are similar to previous changes (1)

python/cudf/cudf/tests/series/accessors/test_str.py

📝 Walkthrough

Summary by CodeRabbit

Bug Fixes
- Improved regex extraction to handle optional capture groups more accurately.
- Non-participating groups now return no value instead of an invalid match, while groups that match an empty string still return an empty string.
- Series.str.extract(..., expand=True) now matches pandas more closely for these cases.

Walkthrough

Updates regex extraction so non-participating capture groups return nullopt instead of an empty match, and adds C++ and Python tests covering null versus empty-string behavior for optional capture groups.

Changes

Non-participating group fix

Layer / File(s)	Summary
Regex match result logic for non-participating groups `cpp/src/strings/regex/regex.inl`	`reprog_device::extract` now returns `nullopt` when `call_regexec` reports a non-participating capture group, and the SPDX header is updated.
C++ and Python extract tests `cpp/tests/strings/extract_tests.cpp`, `python/cudf/cudf/tests/series/accessors/test_str.py`	Adds coverage for optional groups that do not participate versus groups that match an empty string, and updates the SPDX header in the C++ test file.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

rapidsai/cudf#22408: Related regex capture-group handling changes that also distinguish non-participating matches from empty matches.

Suggested labels: 3 - Ready for Review

Suggested reviewers: davidwendt, vyasr, qbacpey, PointKernel

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the main fix: returning null for non-participating regex capture groups in strings::extract.
Description check	✅ Passed	The description directly explains the bug, fix, and added tests, matching the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

🧹 Nitpick comments (1)

cpp/src/strings/regex/regex.inl (1)
240-253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Doxygen doesn't mention the new nullopt-for-non-participating-group behavior.

The @return doc still just says >0 if match found; consider a brief note that non-overall groups that did not participate in the match now return cuda::std::nullopt even when the overall match succeeded.
🤖 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/strings/regex/regex.inl` around lines 240 - 253, The Doxygen for the
regex evaluation function does not describe the new non-participating-group
return behavior. Update the comment on the main regex execution API (the
function documented with dstr, jnk, begin, end, and group_id) so the `@return`
section still notes successful matches but also explicitly mentions that
non-overall groups which do not participate now return cuda::std::nullopt.

🤖 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 `@cpp/src/strings/regex/regex.inl`:
- Around line 240-253: The Doxygen for the regex evaluation function does not
describe the new non-participating-group return behavior. Update the comment on
the main regex execution API (the function documented with dstr, jnk, begin,
end, and group_id) so the `@return` section still notes successful matches but
also explicitly mentions that non-overall groups which do not participate now
return cuda::std::nullopt.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 54bae93e-5ce0-4976-ae99-1fee2502ec94

📥 Commits

Reviewing files that changed from the base of the PR and between 39bd2c7 and a17fa3a.

📒 Files selected for processing (3)

cpp/src/strings/regex/regex.inl
cpp/tests/strings/extract_tests.cpp
python/cudf/cudf/tests/series/accessors/test_str.py

davidwendt

The seems like it should be a breaking change since it is changing the expected behavior.

galipremsagar · 2026-07-06T18:40:55Z

/okay to test c2e326c

galipremsagar · 2026-07-06T20:05:32Z

/okay to test 359747e

Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>

davidwendt

Approving C++ changes.

galipremsagar · 2026-07-07T07:21:25Z

/okay to test 4eafc94

galipremsagar · 2026-07-07T17:27:01Z

/okay to test 5870411

galipremsagar · 2026-07-10T05:16:38Z

/merge

…15280) **JaCoCo sql-plugin line coverage: +25 lines** (8 interval-parsing lines covered by `CsvScanForIntervalSuite`, shim 351; 17 `regexp_extract_all` lines covered by `test_regexp_extract_all_idx_positive`, shim 401) Closes #15275. Closes #15281. ## Root cause [rapidsai/cudf#23123](rapidsai/cudf#23123) changed regex extraction so optional capture groups that do not participate in a match return null instead of an empty string. Two cudf-spark paths relied on the previous representation: - day-time interval parsing treated optional sign and fractional-seconds captures as non-null values; - `regexp_extract_all` removed null list elements, which lost Spark's required empty-string placeholders for unmatched captures and could change list cardinality. Because both failures come from the same cuDF behavior change and independently break premerge, this PR contains the two focused compatibility fixes so the complete Blossom matrix can validate and merge them together. ## Fix Due to the nature of the premerge CI, I have to merge 2 different fixes(though they are caused by the same upstream change) in 1 PR: ### Day-time intervals - Normalize missing sign comparisons to `false` before combining the two signs with XOR. - Normalize a missing fractional-seconds capture to numeric zero before decimal conversion. ### `regexp_extract_all` - Preserve one output list element per regex match. - Convert an unmatched requested capture from null to Spark's empty string. - Preserve empty lists for non-null inputs with no matches and null lists for null inputs. - Restore the input row count when cuDF returns a zero-row list column for an all-no-match batch. The longer-term proposal to move Spark-specific regex result normalization behind the JNI boundary is tracked in [NVIDIA/cudf-spark-jni#4821](NVIDIA/cudf-spark-jni#4821). This PR keeps the current ownership boundaries and fixes the two affected consumers without adding a temporary compatibility switch. ## Validation - Scala 2.12 / Spark 3.5.1 `CsvScanForIntervalSuite`: `Tests: succeeded 6, failed 0, canceled 1, ignored 0, pending 0`; reactor `BUILD SUCCESS`. - Scala 2.13 / Spark 4.0.1 production and integration-test build: reactor `BUILD SUCCESS`. - Spark 4.0.1 `regexp_test.py::test_regexp_extract_all_idx_positive`: `3 passed, 39627 deselected`. - The same regex IT with forced OOM injection: `3 passed, 39627 deselected`. - JaCoCo fix-line intersection: 8 of 16 added interval lines plus 17 of 30 added regex lines covered, for 25 unique added production lines. - `scripts/check-shim-coverage.sh`: passed. ## Performance impact The interval change adds null normalization only in the specialized string-to-day-time-interval path. The regex change removes the previous max-list-width expansion and reconstructs the existing variable-length list column in place; the focused microbenchmark measured 0.538 s before versus 0.464 s after (about 13.7% faster). No new regex match pass is introduced. Documentation - [ ] Updated for new or modified user-facing features or behaviors - [x] No user-facing change Testing - [ ] Added or modified tests to cover new code paths - [x] Covered by existing tests (`CsvScanForIntervalSuite` and `regexp_test.py::test_regexp_extract_all_idx_positive`, including forced OOM injection.) - [ ] Not required Performance - [x] Tests ran and results are added in the PR description - [ ] Issue filed with a link in the PR description - [ ] Not required --------- Signed-off-by: Allen Xu <allxu@nvidia.com>

galipremsagar requested review from a team as code owners July 6, 2026 17:40

galipremsagar requested review from TomAugspurger, brandon-b-miller, lamarrr and shrshi July 6, 2026 17:40

galipremsagar requested a review from davidwendt July 6, 2026 17:41

github-actions Bot assigned galipremsagar Jul 6, 2026

Merge branch 'main' into fix-regex-nonparticipating-group

a17fa3a

github-actions Bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. labels Jul 6, 2026

github-project-automation Bot added this to cuDF Python Jul 6, 2026

galipremsagar added bug Something isn't working non-breaking Non-breaking change labels Jul 6, 2026

GPUtester moved this to In Progress in cuDF Python Jul 6, 2026

coderabbitai Bot reviewed Jul 6, 2026

View reviewed changes

davidwendt requested changes Jul 6, 2026

View reviewed changes

Comment thread cpp/src/strings/regex/regex.inl Outdated

galipremsagar added breaking Breaking change and removed non-breaking Non-breaking change labels Jul 6, 2026

Address review

1e9406e

galipremsagar requested a review from davidwendt July 6, 2026 18:40

Merge branch 'main' into fix-regex-nonparticipating-group

c2e326c

Merge branch 'main' into fix-regex-nonparticipating-group

359747e

davidwendt reviewed Jul 6, 2026

View reviewed changes

Comment thread cpp/src/strings/regex/regex.inl Outdated

Update regex.inl

8837536

Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>

davidwendt approved these changes Jul 6, 2026

View reviewed changes

Merge branch 'main' into fix-regex-nonparticipating-group

4eafc94

mroeschke approved these changes Jul 7, 2026

View reviewed changes

Comment thread python/cudf/cudf/tests/series/accessors/test_str.py Outdated

galipremsagar added 2 commits July 7, 2026 17:11

Parametrize pat in test_string_extract_nonparticipating_group

a12c295

Merge branch 'main' into fix-regex-nonparticipating-group

5870411

mhaseeb123 approved these changes Jul 8, 2026

View reviewed changes

galipremsagar added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Jul 10, 2026

rapids-bot Bot merged commit 76c95f6 into rapidsai:main Jul 10, 2026
138 checks passed

github-project-automation Bot moved this from In Progress to Done in cuDF Python Jul 10, 2026

Uh oh!

Conversation

galipremsagar commented Jul 6, 2026

Description

Checklist

Uh oh!

copy-pr-bot Bot commented Jul 6, 2026

Uh oh!

galipremsagar commented Jul 6, 2026

Uh oh!

coderabbitai Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

davidwendt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

galipremsagar commented Jul 6, 2026

Uh oh!

galipremsagar commented Jul 6, 2026

Uh oh!

Uh oh!

davidwendt left a comment

Choose a reason for hiding this comment

Uh oh!

galipremsagar commented Jul 7, 2026

Uh oh!

Uh oh!

galipremsagar commented Jul 7, 2026

Uh oh!

galipremsagar commented Jul 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coderabbitai Bot commented Jul 6, 2026 •

edited

Loading