Skip to content

Commit 59116da

Browse files
committed
Address feedback and fix mismatched parentheses in the test
Tighten cases where new groups slipped through the existing guards. Add a regression test. Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
1 parent 998ba54 commit 59116da

4 files changed

Lines changed: 30 additions & 6 deletions

File tree

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2057,7 +2057,9 @@ object RegexRewrite {
20572057
@scala.annotation.tailrec
20582058
private def removeBrackets(astLs: collection.Seq[RegexAST]): collection.Seq[RegexAST] = {
20592059
astLs match {
2060-
case collection.Seq(RegexGroup(_, RegexSequence(terms))) => removeBrackets(terms)
2060+
case collection.Seq(RegexGroup(
2061+
RegexGroup.Capturing | RegexGroup.NonCapturing,
2062+
RegexSequence(terms))) => removeBrackets(terms)
20612063
case _ => astLs
20622064
}
20632065
}
@@ -2119,7 +2121,8 @@ object RegexRewrite {
21192121

21202122
private def getMultipleContainsLiterals(ast: RegexAST): Seq[UTF8String] = {
21212123
ast match {
2122-
case RegexGroup(_, term) => getMultipleContainsLiterals(term)
2124+
case RegexGroup(RegexGroup.Capturing | RegexGroup.NonCapturing, term) =>
2125+
getMultipleContainsLiterals(term)
21232126
case RegexChoice(RegexSequence(parts), ls) if isLiteralString(parts) => {
21242127
getMultipleContainsLiterals(ls) match {
21252128
case Seq() => Seq.empty
@@ -2136,7 +2139,8 @@ object RegexRewrite {
21362139
ast match {
21372140
case RegexRepetition(RegexChar('.'), SimpleQuantifier('*')) => true
21382141
case RegexSequence(parts) if parts.forall(isWildcard) => true
2139-
case RegexGroup(_, term) if isWildcard(term) => true
2142+
case RegexGroup(RegexGroup.Capturing | RegexGroup.NonCapturing, term)
2143+
if isWildcard(term) => true
21402144
case _ => false
21412145
}
21422146
}

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/stringFunctions.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1244,7 +1244,7 @@ object GpuRegExpUtils {
12441244

12451245
def getChoicesFromRegex(regex: RegexAST): Option[Seq[String]] = {
12461246
regex match {
1247-
case RegexGroup(_, t) =>
1247+
case RegexGroup(RegexGroup.Capturing | RegexGroup.NonCapturing, t) =>
12481248
getChoicesFromRegex(t)
12491249
case RegexChoice(a, b) =>
12501250
getChoicesFromRegex(a) match {

tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ class RegularExpressionParserSuite extends AnyFunSuite {
139139
RegexCharacterRange(RegexChar('A'), RegexChar('Z')))),
140140
SimpleQuantifier('+')
141141
)
142-
)))
142+
))
143143
),
144144
RegexEscaped(']')
145145
))
@@ -262,7 +262,7 @@ class RegularExpressionParserSuite extends AnyFunSuite {
262262

263263
assert(parse("(?:a?)") === RegexSequence(ListBuffer(
264264
RegexGroup(RegexGroup.NonCapturing, RegexSequence(ListBuffer(
265-
RegexRepetition(RegexChar('a'), SimpleQuantifier('?')))))))
265+
RegexRepetition(RegexChar('a'), SimpleQuantifier('?'))))))))
266266
}
267267

268268
test("group not starting with ? is a capturing group") {

tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionRewriteSuite.scala

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,4 +97,24 @@ class RegularExpressionRewriteSuite extends AnyFunSuite {
9797
)
9898
verifyRewritePattern(patterns, excepted)
9999
}
100+
101+
test("regex rewrite ignores unsupported group types") {
102+
import RegexOptimizationType._
103+
// Lookaround, independent, and named-capture groups are not supported on the GPU. They
104+
// now parse successfully, so the rewrite optimizer must treat them as opaque (only
105+
// capturing/non-capturing groups may be unwrapped) and never mistake them for a simple
106+
// contains/multiple-contains/prefix pattern.
107+
val patterns = Seq(
108+
"(?=abc)",
109+
"(?!abc)",
110+
"(?<=abc)",
111+
"(?<!abc)",
112+
"(?>abc)",
113+
"(?<n>abc)",
114+
"(?<n>abc|def)",
115+
"(?>.*)abc"
116+
)
117+
val excepted = Seq.fill(patterns.length)(NoOptimization)
118+
verifyRewritePattern(patterns, excepted)
119+
}
100120
}

0 commit comments

Comments
 (0)