Refactor regex group parsing and explicitly reject unsupported group types - #15331
Conversation
Updated the RegexGroup implementation to use a more explicit representation of capturing and non-capturing groups and lookarounds. - Refactored RegexGroup to use a combined RegexGroup.Type representation. - Updated tests in RegularExpressionParserSuite and RegularExpressionTranspilerSuite to reflect the new group handling. - Removed unused parameters in method signatures related to group handling. Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
Updated RegexGroup to parse independent, named, positive lookbehind, and negative lookbehind groups. - Added representation and parsing of new group types to the RegexParser. - Updated the CudfRegexTranspiler to explicitly reject unsupported group types during transpilation. - Added test cases in RegularExpressionParserSuite and RegularExpressionTranspilerSuite to validate the new group handling and ensure unsupported patterns are correctly identified. - Updated the old `test_rlike_fallback_lookaheads` function to support additional regex patterns, specifically independent, named capture, and new lookaround groups, and renamed it appropriately. Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
…ex group types Added entries for lookahead/lookbehind groups, independent groups, and named capture groups to compatibility.md, clarifying the current limitations of regex support on the GPU. Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
Greptile SummaryRefactors
Confidence Score: 5/5Safe to merge — all new group types are explicitly rejected by the transpiler, triggering reliable CPU fallback with clear error messages. The refactoring is mechanical and internally consistent: every call site that previously destructured the old three-field RegexGroup has been updated to the new groupType field. The two new early-rejection arms in the transpiler close the silent-discard gaps described in the PR. The rewrite optimizer guards are correctly tightened to Capturing | NonCapturing. Parser edge cases are handled. Test coverage spans parser AST assertions, transpiler rejection, rewrite optimizer, and integration fallback. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["parseGroup()"] --> B{"starts with '?'\nand second char in ':!=<>'?"}
B -- No --> C[RegexGroup.Capturing]
B -- Yes --> D["consume '?', then next char"]
D --> E{Char?}
E -- "':'" --> F[RegexGroup.NonCapturing]
E -- "'='" --> G[RegexGroup.PositiveLookahead]
E -- "'!'" --> H[RegexGroup.NegativeLookahead]
E -- "'>'" --> I[RegexGroup.Independent]
E -- "'<'" --> J{"pos < length?"}
J -- No --> K["throw: trailing '<'"]
J -- Yes --> L["consume next char"]
L --> M{Char?}
M -- "'='" --> N[RegexGroup.PositiveLookbehind]
M -- "'!'" --> O[RegexGroup.NegativeLookbehind]
M -- "isLetter" --> P["parse name loop\nthen consume '>'"]
P --> Q[RegexGroup.Named]
M -- "other" --> R["throw: unexpected char"]
subgraph Transpiler ["CudfRegexTranspiler.rewrite()"]
T1{"groupType?"}
T1 -- "Capturing / NonCapturing" --> T2[rewrite term and emit group]
T1 -- "Lookahead / Lookbehind\nIndependent / Named" --> T3["throw RegexUnsupportedException\n→ CPU fallback"]
end
C --> T1
F --> T1
G --> T1
H --> T1
I --> T1
N --> T1
O --> T1
Q --> T1
Reviews (6): Last reviewed commit: "Merge branch 'main' into regex-transpile..." | Re-trigger Greptile |
e1407ae to
59116da
Compare
Tighten cases where new groups slipped through the existing guards. Add a regression test. Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
59116da to
53e957f
Compare
Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
|
@greptile please re-review. |
|
build |
Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
|
build |
revans2
left a comment
There was a problem hiding this comment.
If I really want to pick some nits we could have a few more tests for things beyond RLIKE, but I think that is not really needed as most of the code is common between them all.
Yeah, I was piling new cases onto the previously existing test, which only covered RLIKE. We can look into adding more coverage in a follow-up if we choose to. |
Description
Refactors regex group handling to use an explicit
RegexGroup.Typerepresentation instead of separatecaptureandlookaheadfields.The regex parser now recognizes additional valid Java group constructs:
These constructs remain unsupported by cuDF and are explicitly rejected during transpilation so they reliably fall back to CPU. The change also prevents unsupported groups adjacent to
$or\Zfrom being silently discarded.Compatibility documentation now explicitly lists all unsupported group types, including lookahead groups.
Testing includes:
RLikefalls back to CPURelated issues: #14946, #14947.
Checklists
Documentation
Testing
Performance