Refactor regex quantifiers into base and mode [reduced-it] [databricks] - #15899
Refactor regex quantifiers into base and mode [reduced-it] [databricks]#15899wjxiz1992 wants to merge 4 commits into
Conversation
Closes NVIDIA#15831 Contributes to NVIDIA#14733 Validation: - Tests: succeeded 183, failed 0, canceled 6, ignored 0, pending 0 - 7 passed, 89 deselected, 2 warnings in 13.61s Signed-off-by: Allen Xu <allxu@nvidia.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It is a broad refactor in a core parsing/transpilation path where subtle semantic or diagnostic-position regressions are possible despite unit coverage.
Pull request overview
Refactors the regex quantifier AST model to a single RegexQuantifier(base, mode) representation, separating repetition shape (base) from matching behavior (mode), while keeping diagnostics positions and the cuDF 999-count boundary behavior intact.
Changes:
- Replaces the former quantifier subtype hierarchy with
RegexQuantifier.Base+RegexQuantifier.Mode, constructed directly by the parser. - Updates cuDF transpiler quantifier pattern matches to use the new
base/modedecomposition. - Updates Scala test suites and fuzz generation to build quantifiers via the new model and adds an independence/equality regression test.
File summaries
| File | Description |
|---|---|
| tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala | Updates fuzz regex generation to construct RegexQuantifier(base, mode) rather than subtype + mode-copy helpers. |
| tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala | Updates expected ASTs to the new quantifier model and adds a test asserting base/mode independence plus equality ignoring diagnostic position. |
| sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala | Refactors parsing and transpilation logic to use RegexQuantifier(base, mode); introduces Base ADT and ports quantifier-related matching/serialization accordingly. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
build |
Apply the requested parser, AST, and test cleanups while preserving the production parse validation boundary. Performance: these changes run only while parsing and transpiling regex plans; they add no per-row or GPU-kernel work, and the consolidated matches reduce dispatch branches. Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
2 similar comments
|
build |
|
build |
igorpeshansky
left a comment
There was a problem hiding this comment.
Would like to resolve the "closer to Java semantics" discussion eventually, but it doesn't have to be addressed in this PR. The rest are minor cleanups.
| Some(QuantifierVariableLength(minLength, maxLength)) | ||
| Some(Variable(minLength, maxLength)) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
FWIW, I think sticking closer to Java semantics helps maintain cleaner code and avoids questions about consistency… Since these edge cases are only reachable from tests, it would be easiest to match Java to decide on the expected behavior.
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
JaCoCo production line coverage: +61 lines (
sql-plugin +61; shim 330, fix-line measurement against this PR diff)Closes #15831
Contributes to #14733
Follow-up to #15478 and the review suggestion.
Description
AI assistance: The change and PR description were prepared with Codex assistance.
What this fixes
Replace the three regex quantifier AST subtypes and mode-copy helpers with one product representation,
RegexQuantifier(base, mode). This is a semantics-preserving follow-up: accepted syntax, serialized regexes, fallback behavior, diagnostic positions, and the 999 repetition boundary are unchanged.Approach
Base:ZeroOrOne,ZeroOrMore,OneOrMore,Fixed(length), orVariable(min, max).Mode:Greedy,Reluctant, orPossessive.Because #15831 is explicitly a behavior-preserving refactor, there is no pre-fix GPU/CPU mismatch to reproduce. Validation instead compares the baseline and patched acceptance suites and reruns the focused CPU/GPU Python cases established by #15478.
Whole-system design assessment
Disposition:
ARCHITECTURAL_FIXThe smallest coherent change is at the AST boundary: base and mode become explicit independent dimensions, and all parser/transpiler/rewrite/serialization consumers use that single representation. This removes subtype-specific reconstruction while preserving the existing public behavior and fallback diagnostics.
Tests added
RegularExpressionParserSuite->issue-15831: quantifier base and mode are independent semantic dimensionsRegularExpressionParserSuite,RegularExpressionTranspilerSuite,RegularExpressionRewriteSuite, andRegExpUtilsSuiteregexp_test.pyselection covering negative-limit split, unsupported fallback, repetition replacement, possessive fallback, extract fallbacks, and lazy quantifiersLocal validation
Scala suites:
JaCoCo production fix-line measurement:
Python IT with Spark 3.3 and Python 3.10:
Scalastyle:
End-to-end CPU/GPU parity on the patched dist JAR:
Performance impact
Regex parsing remains O(pattern length), with constant work per quantifier. This runs during expression translation, not per input row; it adds no JNI or GPU work. Direct construction removes the simple-quantifier mode-copy allocation, while brace quantifiers retain the same aggregate temporary-allocation count. A runtime benchmark is not warranted for this cold-path representation refactor.
Checklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance