From 0b267348c00b77cfb3cfa0d5b62bbb4ff4a79085 Mon Sep 17 00:00:00 2001 From: Allen Xu Date: Fri, 4 Sep 2026 13:22:07 +0800 Subject: [PATCH 1/5] fix(regex): unify quantifier base and mode (#15831) Closes #15831 Contributes to #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 --- .../com/nvidia/spark/rapids/RegexParser.scala | 150 +++++++++--------- .../rapids/RegularExpressionParserSuite.scala | 80 +++++++--- .../RegularExpressionTranspilerSuite.scala | 25 ++- 3 files changed, 138 insertions(+), 117 deletions(-) diff --git a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala index 8d2703726f6..4f3992420a4 100644 --- a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala +++ b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala @@ -161,7 +161,9 @@ class RegexParser(pattern: String) { * {n,} * {n,m} (only valid if m >= n) */ - private def tryParseBraceQuantifier(): Option[RegexQuantifier] = { + private def tryParseBraceQuantifier(): Option[RegexQuantifier.Base] = { + import RegexQuantifier._ + // The caller restores its position when this is a literal brace rather than a quantifier. consumeExpected('{') consumeInt.flatMap { minLength => @@ -171,13 +173,13 @@ class RegexParser(pattern: String) { val maxLength = consumeInt() if (peek().contains('}') && maxLength.forall(_ >= minLength)) { consumeExpected('}') - Some(QuantifierVariableLength(minLength, maxLength)) + Some(Variable(minLength, maxLength)) } else { None } case Some('}') => consumeExpected('}') - Some(QuantifierFixedLength(minLength)) + Some(Fixed(minLength)) case _ => None } @@ -191,9 +193,15 @@ class RegexParser(pattern: String) { val baseQuantifier = peek() match { case Some('{') => tryParseBraceQuantifier() - case Some(ch) if "*+?".contains(ch) => + case Some('*') => + consume() + Some(ZeroOrMore) + case Some('+') => consume() - Some(SimpleQuantifier(ch)) + Some(OneOrMore) + case Some('?') => + consume() + Some(ZeroOrOne) case _ => None } @@ -208,7 +216,7 @@ class RegexParser(pattern: String) { Possessive case _ => Greedy } - val quantifier = base.withMode(mode) + val quantifier = RegexQuantifier(base, mode) // Point diagnostics at the modifier when present, otherwise at the base quantifier. quantifier.position = Some(if (mode == Greedy) start else pos - 1) Some(quantifier) @@ -781,6 +789,8 @@ sealed case class RegexRewriteFlags( RegexSplitMode if performing a split (string_split) */ class CudfRegexTranspiler(mode: RegexMode) { + import RegexQuantifier._ + // cuDF reads at most three count digits for a repetition. // https://github.com/NVIDIA/cudf/blob/7a6f5c1a/cpp/src/strings/regex/regcomp.cpp#L684 private val maxRepetitionCount = 999 @@ -789,11 +799,11 @@ class CudfRegexTranspiler(mode: RegexMode) { 'b' -> '\b', 'e' -> '\u001b') private def exceedsCudfRepetitionCountLimit(quantifier: RegexQuantifier): Boolean = { - quantifier match { - case QuantifierFixedLength(length, _) => length > maxRepetitionCount - case QuantifierVariableLength(minLength, maxLength, _) => + quantifier.base match { + case Fixed(length) => length > maxRepetitionCount + case Variable(minLength, maxLength) => minLength > maxRepetitionCount || maxLength.exists(_ > maxRepetitionCount) - case _: SimpleQuantifier => false + case ZeroOrOne | ZeroOrMore | OneOrMore => false } } @@ -1580,7 +1590,7 @@ class CudfRegexTranspiler(mode: RegexMode) { s"cuDF does not support repetition counts greater than $maxRepetitionCount", repetition.position) - case (_, q) if q.isPossessive => + case (_, q @ RegexQuantifier(_, Possessive)) => throw new RegexUnsupportedException( s"Possessive quantifier ${q.toRegexString} not supported", q.position) @@ -1594,12 +1604,12 @@ class CudfRegexTranspiler(mode: RegexMode) { "regexp_split on GPU does not support empty match repetition consistently with Spark", quantifier.position) - case (_, QuantifierVariableLength(0, Some(0), _)) if mode != RegexFindMode => + case (_, RegexQuantifier(Variable(0, Some(0)), _)) if mode != RegexFindMode => throw new RegexUnsupportedException( "regex_replace and regex_split on GPU do not support repetition with {0,0}", quantifier.position) - case (_, QuantifierFixedLength(0, _)) if mode != RegexFindMode => + case (_, RegexQuantifier(Fixed(0), _)) if mode != RegexFindMode => throw new RegexUnsupportedException( "regex_replace and regex_split on GPU do not support repetition with {0}", quantifier.position) @@ -1609,12 +1619,13 @@ class CudfRegexTranspiler(mode: RegexMode) { "Repetition of lookaround, independent, or named capture groups is not supported", g.position) - case (RegexGroup(groupType, term), SimpleQuantifier(ch, _)) - if "+*".contains(ch) && !isSupportedRepetitionBase(term) => - (term, ch) match { + case (RegexGroup(groupType, term), RegexQuantifier(simpleBase, _)) + if (simpleBase == OneOrMore || simpleBase == ZeroOrMore) && + !isSupportedRepetitionBase(term) => + (term, simpleBase) match { // \Z is not supported in groups - case (RegexEscaped('A'), '+') | - (RegexSequence(ListBuffer(RegexEscaped('A'))), '+') => + case (RegexEscaped('A'), OneOrMore) | + (RegexSequence(ListBuffer(RegexEscaped('A'))), OneOrMore) => // (\A)+ can be transpiled to (\A) (dropping the repetition) // we use rewrite(...) here to handle logic regarding modes // (\A is not supported in RegexSplitMode) @@ -1627,7 +1638,7 @@ class CudfRegexTranspiler(mode: RegexMode) { s"cuDF does not support repetition of group containing: " + s"${unsupportedTerm.toRegexString}", term.position) } - case (RegexGroup(groupType, term), QuantifierVariableLength(_, _, _)) + case (RegexGroup(groupType, term), RegexQuantifier(Variable(_, _), _)) if !isSupportedRepetitionBase(term) => term match { // \Z is not supported in groups @@ -1646,7 +1657,7 @@ class CudfRegexTranspiler(mode: RegexMode) { s"cuDF does not support repetition of group containing: " + s"${unsupportedTerm.toRegexString}", term.position) } - case (RegexGroup(groupType, term), QuantifierFixedLength(n, _)) + case (RegexGroup(groupType, term), RegexQuantifier(Fixed(n), _)) if !isSupportedRepetitionBase(term) => term match { // \Z is not supported in groups @@ -1664,13 +1675,14 @@ class CudfRegexTranspiler(mode: RegexMode) { s"cuDF does not support repetition of group containing: " + s"${unsupportedTerm.toRegexString}", term.position) } - case (RegexGroup(_, term), SimpleQuantifier('?', _)) => + case (RegexGroup(_, term), RegexQuantifier(ZeroOrOne, _)) => if (isEntirelyWordBoundary(term) || isEntirelyLineAnchor(term)) { throw new RegexUnsupportedException( s"cuDF does not support repetition of: ${term.toRegexString}", term.position) } RegexRepetition(rewrite(base, None, flags), quantifier) - case (RegexEscaped(ch), SimpleQuantifier('+', _)) if "AZ".contains(ch) => + case (RegexEscaped(ch), RegexQuantifier(OneOrMore, _)) + if "AZ".contains(ch) => // \A+ can be transpiled to \A (dropping the repetition) // \Z+ can be transpiled to \Z (dropping the repetition) // we use rewrite(...) here to handle logic regarding modes @@ -1678,11 +1690,12 @@ class CudfRegexTranspiler(mode: RegexMode) { rewrite(base, previous, flags) // NOTE: \A* can be transpiled to \A? // however, \A? is not supported in libcudf yet - case (RegexEscaped(ch), QuantifierFixedLength(n, _)) if n > 0 && "AZ".contains(ch) => + case (RegexEscaped(ch), RegexQuantifier(Fixed(n), _)) + if n > 0 && "AZ".contains(ch) => // \A{2} can be transpiled to \A (dropping the repetition) // \Z{2} can be transpiled to \Z (dropping the repetition) rewrite(base, previous, flags) - case (RegexEscaped(ch), QuantifierVariableLength(n, _, _)) + case (RegexEscaped(ch), RegexQuantifier(Variable(n, _), _)) if n > 0 && "AZ".contains(ch) => // \A{1,5} can be transpiled to \A (dropping the repetition) // \Z{1,} can be transpiled to \Z (dropping the repetition) @@ -1742,14 +1755,14 @@ class CudfRegexTranspiler(mode: RegexMode) { (ll, rr) match { // ll = lazyQuantifier inside a choice case (RegexSequence(ListBuffer(RegexRepetition( - _, SimpleQuantifier('?', RegexQuantifier.Reluctant)))), _) | + _, RegexQuantifier(ZeroOrOne, Reluctant)))), _) | // rr = lazyQuantifier inside a choice (_, RegexSequence(ListBuffer(RegexRepetition( - _, SimpleQuantifier('?', RegexQuantifier.Reluctant))))) => + _, RegexQuantifier(ZeroOrOne, Reluctant))))) => throw new RegexUnsupportedException( "cuDF does not support lazy quantifier inside choice", r.position) case (_, RegexChoice(RegexSequence(_), RegexSequence(ListBuffer(RegexRepetition( - RegexEscaped('A'), SimpleQuantifier('?', _)), _)))) => + RegexEscaped('A'), RegexQuantifier(ZeroOrOne, _)), _)))) => throw new RegexUnsupportedException("Invalid regex pattern at position", r.position) case _ => } @@ -1765,12 +1778,12 @@ class CudfRegexTranspiler(mode: RegexMode) { } part match { case RegexRepetition(base, quantifier) => (base, quantifier) match { - case (_, QuantifierVariableLength(0, Some(0), _)) => + case (_, RegexQuantifier(Variable(0, Some(0)), _)) => throw new RegexUnsupportedException( "Repetition with {0,0} not supported in capture groups", quantifier.position) - case (_, QuantifierFixedLength(0, _)) => + case (_, RegexQuantifier(Fixed(0), _)) => throw new RegexUnsupportedException( "Repetition with {0} not supported in capture groups", quantifier.position) @@ -2043,73 +2056,52 @@ sealed case class RegexRepetition(a: RegexAST, quantifier: RegexQuantifier) exte } object RegexQuantifier { + sealed trait Base + case object ZeroOrOne extends Base + case object ZeroOrMore extends Base + case object OneOrMore extends Base + final case class Fixed(length: Int) extends Base + final case class Variable(minLength: Int, maxLength: Option[Int]) extends Base + sealed trait Mode case object Greedy extends Mode case object Reluctant extends Mode case object Possessive extends Mode } -sealed trait RegexQuantifier { +sealed case class RegexQuantifier( + base: RegexQuantifier.Base, + mode: RegexQuantifier.Mode = RegexQuantifier.Greedy) { import RegexQuantifier._ - def mode: Mode - def minLength: Int - protected def baseToRegexString: String - protected def copyWithMode(newMode: Mode): RegexQuantifier - var position: Option[Int] = None - final def withMode(newMode: Mode): RegexQuantifier = { - val updated = copyWithMode(newMode) - updated.position = position - updated + def minLength: Int = base match { + case ZeroOrOne | ZeroOrMore => 0 + case OneOrMore => 1 + case Fixed(length) => length + case Variable(minLength, _) => minLength } - final def isPossessive: Boolean = mode == Possessive - final def toRegexString: String = { + def toRegexString: String = { + val baseString = base match { + case ZeroOrOne => "?" + case ZeroOrMore => "*" + case OneOrMore => "+" + case Fixed(length) => s"{$length}" + case Variable(minLength, maxLength) => + maxLength match { + case Some(max) => s"{$minLength,$max}" + case None => s"{$minLength,}" + } + } val suffix = mode match { case Greedy => "" case Reluctant => "?" case Possessive => "+" } - s"$baseToRegexString$suffix" - } -} - -sealed case class SimpleQuantifier( - ch: Char, - mode: RegexQuantifier.Mode = RegexQuantifier.Greedy) extends RegexQuantifier { - override def minLength: Int = if (ch == '+') 1 else 0 - override protected def baseToRegexString: String = ch.toString - override protected def copyWithMode(newMode: RegexQuantifier.Mode): RegexQuantifier = - copy(mode = newMode) -} - -sealed case class QuantifierFixedLength( - length: Int, - mode: RegexQuantifier.Mode = RegexQuantifier.Greedy) - extends RegexQuantifier { - override def minLength: Int = length - override protected def baseToRegexString: String = s"{$length}" - override protected def copyWithMode(newMode: RegexQuantifier.Mode): RegexQuantifier = - copy(mode = newMode) -} - -sealed case class QuantifierVariableLength( - minLength: Int, - maxLength: Option[Int], - mode: RegexQuantifier.Mode = RegexQuantifier.Greedy) - extends RegexQuantifier { - override protected def baseToRegexString: String = { - maxLength match { - case Some(max) => - s"{$minLength,$max}" - case _ => - s"{$minLength,}" - } + s"$baseString$suffix" } - override protected def copyWithMode(newMode: RegexQuantifier.Mode): RegexQuantifier = - copy(mode = newMode) } sealed trait RegexCharacterClassComponent extends RegexAST @@ -2348,7 +2340,7 @@ object RegexRewrite { private def isWildcard(ast: RegexAST): Boolean = { ast match { case RegexRepetition(RegexChar('.'), - SimpleQuantifier('*', RegexQuantifier.Greedy)) => true + RegexQuantifier(RegexQuantifier.ZeroOrMore, RegexQuantifier.Greedy)) => true case RegexSequence(parts) if parts.forall(isWildcard) => true case RegexGroup(groupType, term) if isTransparentGroup(groupType) && isWildcard(term) => true case _ => false diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala index 7b1ea725656..4949daf5ad0 100644 --- a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala @@ -26,6 +26,8 @@ import org.scalatest.funsuite.AnyFunSuite @scala.annotation.nowarn("cat=lint-missing-interpolator") class RegularExpressionParserSuite extends AnyFunSuite { + import RegexQuantifier._ + test("detect regexp strings") { // Based on https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html val strings: Seq[String] = Seq("\\", "\u0000", "\\x00", "\\.", @@ -51,15 +53,15 @@ class RegularExpressionParserSuite extends AnyFunSuite { test("simple quantifier") { assert(parse("a{1}") === RegexSequence(ListBuffer( - RegexRepetition(RegexChar('a'), QuantifierFixedLength(1))))) + RegexRepetition(RegexChar('a'), RegexQuantifier(Fixed(1)))))) } test("bounded reluctant quantifiers have semantic modes") { val cases = Seq( - "a{2}?" -> QuantifierFixedLength(2, RegexQuantifier.Reluctant), - "a{2,}?" -> QuantifierVariableLength(2, None, RegexQuantifier.Reluctant), - "a{2,3}?" -> QuantifierVariableLength(2, Some(3), RegexQuantifier.Reluctant), - "a{0,3}?" -> QuantifierVariableLength(0, Some(3), RegexQuantifier.Reluctant)) + "a{2}?" -> RegexQuantifier(Fixed(2), Reluctant), + "a{2,}?" -> RegexQuantifier(Variable(2, None), Reluctant), + "a{2,3}?" -> RegexQuantifier(Variable(2, Some(3)), Reluctant), + "a{0,3}?" -> RegexQuantifier(Variable(0, Some(3)), Reluctant)) cases.foreach { case (pattern, quantifier) => val ast = RegexSequence(ListBuffer(RegexRepetition(RegexChar('a'), quantifier))) @@ -68,6 +70,34 @@ class RegularExpressionParserSuite extends AnyFunSuite { } } + test("issue-15831: quantifier base and mode are independent semantic dimensions") { + val bases: Seq[(RegexQuantifier.Base, String, Int)] = Seq( + (ZeroOrOne, "?", 0), + (ZeroOrMore, "*", 0), + (OneOrMore, "+", 1), + (Fixed(2), "{2}", 2), + (Variable(2, None), "{2,}", 2), + (Variable(2, Some(3)), "{2,3}", 2)) + val modes = Seq( + (Greedy, ""), + (Reluctant, "?"), + (Possessive, "+")) + + bases.foreach { case (base, baseString, minLength) => + modes.foreach { case (mode, suffix) => + val quantifier = RegexQuantifier(base, mode) + assert(quantifier.minLength === minLength) + assert(quantifier.toRegexString === s"$baseString$suffix") + } + } + + val first = RegexQuantifier(Fixed(2)) + first.position = Some(1) + val second = RegexQuantifier(Fixed(2)) + second.position = Some(9) + assert(first === second) + } + test("quantifier diagnostics point at the base or mode modifier") { val cases = Seq( "a*" -> 1, @@ -86,9 +116,9 @@ class RegularExpressionParserSuite extends AnyFunSuite { // Regression test for https://github.com/NVIDIA/cudf-spark/issues/15495 test("quantifier integer boundaries") { val supportedBoundaries: Seq[(String, RegexQuantifier)] = Seq( - s"a{${Int.MaxValue}}" -> QuantifierFixedLength(Int.MaxValue), - s"a{${Int.MaxValue},}" -> QuantifierVariableLength(Int.MaxValue, None), - s"a{1,${Int.MaxValue}}" -> QuantifierVariableLength(1, Some(Int.MaxValue))) + s"a{${Int.MaxValue}}" -> RegexQuantifier(Fixed(Int.MaxValue)), + s"a{${Int.MaxValue},}" -> RegexQuantifier(Variable(Int.MaxValue, None)), + s"a{1,${Int.MaxValue}}" -> RegexQuantifier(Variable(1, Some(Int.MaxValue)))) supportedBoundaries.foreach { case (pattern, quantifier) => assert(new RegexParser(pattern).parseUnchecked() === RegexSequence(ListBuffer(RegexRepetition(RegexChar('a'), quantifier)))) @@ -123,7 +153,7 @@ class RegularExpressionParserSuite extends AnyFunSuite { assert(parse("a*+") === RegexSequence(ListBuffer( RegexRepetition(RegexChar('a'), - SimpleQuantifier('*', RegexQuantifier.Possessive))))) + RegexQuantifier(ZeroOrMore, Possessive))))) } test("stacked quantifiers are unsupported") { @@ -204,7 +234,7 @@ class RegularExpressionParserSuite extends AnyFunSuite { RegexRepetition( RegexCharacterClass(negated = true, ListBuffer(RegexChar(']'), RegexChar('+'), RegexChar('d'))), - SimpleQuantifier('+'))))) + RegexQuantifier(OneOrMore))))) } test("character classes containing ']'") { @@ -255,7 +285,7 @@ class RegularExpressionParserSuite extends AnyFunSuite { RegexRepetition( RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('A'), RegexChar('Z')))), - SimpleQuantifier('+') + RegexQuantifier(OneOrMore) ) )) ), @@ -336,26 +366,26 @@ class RegularExpressionParserSuite extends AnyFunSuite { assert(parse("(3?)+") === RegexSequence(ListBuffer(RegexRepetition(RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer(RegexRepetition(RegexChar('3'), - SimpleQuantifier('?'))))),SimpleQuantifier('+'))))) + RegexQuantifier(ZeroOrOne))))),RegexQuantifier(OneOrMore))))) } test("repetition with group containing escape character") { assert(parse(raw"(\A)+") === RegexSequence(ListBuffer(RegexRepetition(RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer(RegexEscaped('A')))), - SimpleQuantifier('+')))) + RegexQuantifier(OneOrMore)))) ) assert(parse(raw"(?:\A)+") === RegexSequence(ListBuffer(RegexRepetition(RegexGroup(RegexGroup.NonCapturing, RegexSequence(ListBuffer(RegexEscaped('A')))), - SimpleQuantifier('+')))) + RegexQuantifier(OneOrMore)))) ) } test("group containing choice with repetition") { assert(parse("(\t+|a)") == RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexChoice(RegexSequence(ListBuffer( - RegexRepetition(RegexChar('\t'),SimpleQuantifier('+')))), + RegexRepetition(RegexChar('\t'),RegexQuantifier(OneOrMore)))), RegexSequence(ListBuffer(RegexChar('a')))))))) } @@ -376,7 +406,7 @@ class RegularExpressionParserSuite extends AnyFunSuite { test("group containing quantifier") { assert(parse("(?:a?)") === RegexSequence(ListBuffer( RegexGroup(RegexGroup.NonCapturing, RegexSequence(ListBuffer( - RegexRepetition(RegexChar('a'), SimpleQuantifier('?')))))))) + RegexRepetition(RegexChar('a'), RegexQuantifier(ZeroOrOne)))))))) assert(parse("(?i:a)") === RegexSequence(ListBuffer( RegexGroup(RegexGroup.ScopedFlags(RegexFlagSet(Set(RegexFlag.CaseInsensitive), Set())), RegexSequence(ListBuffer(RegexChar('a'))))))) @@ -413,43 +443,43 @@ class RegularExpressionParserSuite extends AnyFunSuite { assert(ast === RegexSequence(ListBuffer(RegexChar('^'), RegexRepetition(RegexCharacterClass(negated = false, ListBuffer( - RegexChar('+'), RegexEscaped('-'))), SimpleQuantifier('?')), + RegexChar('+'), RegexEscaped('-'))), RegexQuantifier(ZeroOrOne)), RegexGroup(RegexGroup.Capturing, RegexChoice(RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexChoice(RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexRepetition(RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - SimpleQuantifier('+'))))))), + RegexQuantifier(OneOrMore))))))), RegexChoice(RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexRepetition( RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - SimpleQuantifier('*')), RegexEscaped('.'), + RegexQuantifier(ZeroOrMore)), RegexEscaped('.'), RegexRepetition( RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - SimpleQuantifier('+'))))))), RegexSequence(ListBuffer( + RegexQuantifier(OneOrMore))))))), RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexRepetition( RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - SimpleQuantifier('+')), RegexEscaped('.'), + RegexQuantifier(OneOrMore)), RegexEscaped('.'), RegexRepetition(RegexCharacterClass(negated = false, ListBuffer(RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - SimpleQuantifier('*')))))))))), + RegexQuantifier(ZeroOrMore)))))))))), RegexRepetition( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexCharacterClass(negated = false, ListBuffer(RegexChar('e'), RegexChar('E'))), RegexRepetition(RegexCharacterClass(negated = false, - ListBuffer(RegexChar('+'), RegexEscaped('-'))),SimpleQuantifier('?')), + ListBuffer(RegexChar('+'), RegexEscaped('-'))),RegexQuantifier(ZeroOrOne)), RegexRepetition(RegexCharacterClass(negated = false, ListBuffer(RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - SimpleQuantifier('+'))))), SimpleQuantifier('?')), + RegexQuantifier(OneOrMore))))), RegexQuantifier(ZeroOrOne)), RegexRepetition(RegexCharacterClass(negated = false, ListBuffer( RegexChar('f'), RegexChar('F'), RegexChar('d'), RegexChar('D'))), - SimpleQuantifier('?'))))))), + RegexQuantifier(ZeroOrOne))))))), RegexChoice(RegexSequence(ListBuffer( RegexChar('I'), RegexChar('n'), RegexChar('f'))), RegexSequence(ListBuffer( diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala index 2c4283905fb..802feee39f6 100644 --- a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala @@ -1714,26 +1714,25 @@ class FuzzRegExp(suggestedChars: String, skipKnownIssues: Boolean = true, } private def repetition(depth: Int) = { - val modes = Seq[RegexQuantifier.Mode]( - RegexQuantifier.Greedy, - RegexQuantifier.Reluctant, - RegexQuantifier.Possessive) + import RegexQuantifier._ + val modes = Seq[Mode](Greedy, Reluctant, Possessive) val mode = modes(rr.nextInt(modes.length)) - RegexRepetition(generate(depth + 1), quantifier.withMode(mode)) + RegexRepetition(generate(depth + 1), RegexQuantifier(quantifierBase, mode)) } - private def quantifier: RegexQuantifier = { - val generators = Seq[() => RegexQuantifier]( - () => SimpleQuantifier('+'), - () => SimpleQuantifier('*'), - () => SimpleQuantifier('?'), - () => QuantifierFixedLength(rr.nextInt(3)), - () => QuantifierVariableLength(rr.nextInt(3), None), + private def quantifierBase: RegexQuantifier.Base = { + import RegexQuantifier._ + val generators = Seq[() => Base]( + () => OneOrMore, + () => ZeroOrMore, + () => ZeroOrOne, + () => Fixed(rr.nextInt(3)), + () => Variable(rr.nextInt(3), None), () => { // this intentionally generates some invalid quantifiers where the maxLength // is less than the minLength, such as "{2,1}" which should be handled as a // literal string match on "{2,1}" rather than as a valid quantifier. - QuantifierVariableLength(rr.nextInt(3), Some(rr.nextInt(3))) + Variable(rr.nextInt(3), Some(rr.nextInt(3))) } ) generators(rr.nextInt(generators.length))() From 9f3a4ff4ad71fb936c29f0606b991af9383c1c85 Mon Sep 17 00:00:00 2001 From: Allen Xu Date: Sat, 5 Sep 2026 15:19:42 +0800 Subject: [PATCH 2/5] Address regex quantifier review feedback 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 --- .../com/nvidia/spark/rapids/RegexParser.scala | 127 +++++------------- .../rapids/RegularExpressionParserSuite.scala | 63 +++++---- .../RegularExpressionTranspilerSuite.scala | 4 +- 3 files changed, 73 insertions(+), 121 deletions(-) diff --git a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala index 4f3992420a4..9763bcbbfa0 100644 --- a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala +++ b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala @@ -194,13 +194,13 @@ class RegexParser(pattern: String) { case Some('{') => tryParseBraceQuantifier() case Some('*') => - consume() + consumeExpected('*') Some(ZeroOrMore) case Some('+') => - consume() + consumeExpected('+') Some(OneOrMore) case Some('?') => - consume() + consumeExpected('?') Some(ZeroOrOne) case _ => None } @@ -209,17 +209,15 @@ class RegexParser(pattern: String) { case Some(base) => val mode = peek() match { case Some('?') => - consume() + consumeExpected('?') Reluctant case Some('+') => - consume() + consumeExpected('+') Possessive case _ => Greedy } - val quantifier = RegexQuantifier(base, mode) // Point diagnostics at the modifier when present, otherwise at the base quantifier. - quantifier.position = Some(if (mode == Greedy) start else pos - 1) - Some(quantifier) + Some(new RegexQuantifier(base, mode, if (mode == Greedy) start else pos - 1)) case None => pos = start None @@ -1590,9 +1588,10 @@ class CudfRegexTranspiler(mode: RegexMode) { s"cuDF does not support repetition counts greater than $maxRepetitionCount", repetition.position) - case (_, q @ RegexQuantifier(_, Possessive)) => + case (_, RegexQuantifier(_, Possessive)) => throw new RegexUnsupportedException( - s"Possessive quantifier ${q.toRegexString} not supported", q.position) + s"Possessive quantifier ${quantifier.toRegexString} not supported", + quantifier.position) case (_, q) if mode == RegexSplitMode && flags.emptyRepetition && q.minLength == 0 => @@ -1604,14 +1603,11 @@ class CudfRegexTranspiler(mode: RegexMode) { "regexp_split on GPU does not support empty match repetition consistently with Spark", quantifier.position) - case (_, RegexQuantifier(Variable(0, Some(0)), _)) if mode != RegexFindMode => - throw new RegexUnsupportedException( - "regex_replace and regex_split on GPU do not support repetition with {0,0}", - quantifier.position) - - case (_, RegexQuantifier(Fixed(0), _)) if mode != RegexFindMode => + case (_, RegexQuantifier(Variable(0, Some(0)) | Fixed(0), _)) + if mode != RegexFindMode => throw new RegexUnsupportedException( - "regex_replace and regex_split on GPU do not support repetition with {0}", + s"regex_replace and regex_split on GPU do not support repetition with " + + s"${quantifier.toRegexString}", quantifier.position) case (g @ RegexGroup(groupType, _), _) if !RegexGroup.isCudfGroupType(groupType) => @@ -1619,53 +1615,20 @@ class CudfRegexTranspiler(mode: RegexMode) { "Repetition of lookaround, independent, or named capture groups is not supported", g.position) - case (RegexGroup(groupType, term), RegexQuantifier(simpleBase, _)) - if (simpleBase == OneOrMore || simpleBase == ZeroOrMore) && - !isSupportedRepetitionBase(term) => - (term, simpleBase) match { - // \Z is not supported in groups - case (RegexEscaped('A'), OneOrMore) | - (RegexSequence(ListBuffer(RegexEscaped('A'))), OneOrMore) => - // (\A)+ can be transpiled to (\A) (dropping the repetition) - // we use rewrite(...) here to handle logic regarding modes - // (\A is not supported in RegexSplitMode) - RegexGroup(groupType, rewrite(term, previous, flags)) - // NOTE: (\A)* can be transpiled to (\A)? - // however, (\A)? is not supported in libcudf yet - case _ => - val unsupportedTerm = getUnsupportedRepetitionBase(term) - throw new RegexUnsupportedException( - s"cuDF does not support repetition of group containing: " + - s"${unsupportedTerm.toRegexString}", term.position) + case (RegexGroup(_, term), RegexQuantifier(ZeroOrOne, _)) => + if (isEntirelyWordBoundary(term) || isEntirelyLineAnchor(term)) { + throw new RegexUnsupportedException( + s"cuDF does not support repetition of: ${term.toRegexString}", term.position) } - case (RegexGroup(groupType, term), RegexQuantifier(Variable(_, _), _)) - if !isSupportedRepetitionBase(term) => + RegexRepetition(rewrite(base, None, flags), quantifier) + case (RegexGroup(groupType, term), _) if !isSupportedRepetitionBase(term) => term match { // \Z is not supported in groups - case RegexEscaped('A') | - RegexSequence(ListBuffer(RegexEscaped('A'))) + case RegexEscaped('A') | RegexSequence(ListBuffer(RegexEscaped('A'))) if quantifier.minLength > 0 => - // (\A){1,} can be transpiled to (\A) (dropping the repetition) - // we use rewrite(...) here to handle logic regarding modes - // (\A is not supported in RegexSplitMode) - RegexGroup(groupType, rewrite(term, previous, flags)) - // NOTE: (\A)* can be transpiled to (\A)? - // however, (\A)? is not supported in libcudf yet - case _ => - val unsupportedTerm = getUnsupportedRepetitionBase(term) - throw new RegexUnsupportedException( - s"cuDF does not support repetition of group containing: " + - s"${unsupportedTerm.toRegexString}", term.position) - } - case (RegexGroup(groupType, term), RegexQuantifier(Fixed(n), _)) - if !isSupportedRepetitionBase(term) => - term match { - // \Z is not supported in groups - case RegexEscaped('A') | - RegexSequence(ListBuffer(RegexEscaped('A'))) if n > 0 => - // (\A){1,} can be transpiled to (\A) (dropping the repetition) - // we use rewrite(...) here to handle logic regarding modes - // (\A is not supported in RegexSplitMode) + // (\A)+, (\A){2}, and (\A){1,} can be transpiled to (\A) + // (dropping the repetition). We use rewrite(...) here to handle logic + // regarding modes (\A is not supported in RegexSplitMode). RegexGroup(groupType, rewrite(term, previous, flags)) // NOTE: (\A)* can be transpiled to (\A)? // however, (\A)? is not supported in libcudf yet @@ -1675,31 +1638,14 @@ class CudfRegexTranspiler(mode: RegexMode) { s"cuDF does not support repetition of group containing: " + s"${unsupportedTerm.toRegexString}", term.position) } - case (RegexGroup(_, term), RegexQuantifier(ZeroOrOne, _)) => - if (isEntirelyWordBoundary(term) || isEntirelyLineAnchor(term)) { - throw new RegexUnsupportedException( - s"cuDF does not support repetition of: ${term.toRegexString}", term.position) - } - RegexRepetition(rewrite(base, None, flags), quantifier) - case (RegexEscaped(ch), RegexQuantifier(OneOrMore, _)) - if "AZ".contains(ch) => - // \A+ can be transpiled to \A (dropping the repetition) - // \Z+ can be transpiled to \Z (dropping the repetition) - // we use rewrite(...) here to handle logic regarding modes - // (\A and \Z are not supported in RegexSplitMode) + case (RegexEscaped(ch), _) if quantifier.minLength > 0 && "AZ".contains(ch) => + // \A+, \A{2}, and \A{1,5} can be transpiled to \A (dropping the repetition). + // \Z+, \Z{2}, and \Z{1,} can be transpiled to \Z (dropping the repetition). + // We use rewrite(...) here to handle logic regarding modes + // (\A and \Z are not supported in RegexSplitMode). rewrite(base, previous, flags) // NOTE: \A* can be transpiled to \A? // however, \A? is not supported in libcudf yet - case (RegexEscaped(ch), RegexQuantifier(Fixed(n), _)) - if n > 0 && "AZ".contains(ch) => - // \A{2} can be transpiled to \A (dropping the repetition) - // \Z{2} can be transpiled to \Z (dropping the repetition) - rewrite(base, previous, flags) - case (RegexEscaped(ch), RegexQuantifier(Variable(n, _), _)) - if n > 0 && "AZ".contains(ch) => - // \A{1,5} can be transpiled to \A (dropping the repetition) - // \Z{1,} can be transpiled to \Z (dropping the repetition) - rewrite(base, previous, flags) case _ if isSupportedRepetitionBase(base) => RegexRepetition(rewrite(base, None, flags), quantifier) case _ => @@ -2060,8 +2006,8 @@ object RegexQuantifier { case object ZeroOrOne extends Base case object ZeroOrMore extends Base case object OneOrMore extends Base - final case class Fixed(length: Int) extends Base - final case class Variable(minLength: Int, maxLength: Option[Int]) extends Base + sealed case class Fixed(length: Int) extends Base + sealed case class Variable(minLength: Int, maxLength: Option[Int]) extends Base sealed trait Mode case object Greedy extends Mode @@ -2071,9 +2017,14 @@ object RegexQuantifier { sealed case class RegexQuantifier( base: RegexQuantifier.Base, - mode: RegexQuantifier.Mode = RegexQuantifier.Greedy) { + mode: RegexQuantifier.Mode) { import RegexQuantifier._ + def this(base: RegexQuantifier.Base, mode: RegexQuantifier.Mode, position: Int) = { + this(base, mode) + this.position = Some(position) + } + var position: Option[Int] = None def minLength: Int = base match { @@ -2089,11 +2040,7 @@ sealed case class RegexQuantifier( case ZeroOrMore => "*" case OneOrMore => "+" case Fixed(length) => s"{$length}" - case Variable(minLength, maxLength) => - maxLength match { - case Some(max) => s"{$minLength,$max}" - case None => s"{$minLength,}" - } + case Variable(minLength, maxLength) => s"{$minLength,${maxLength.mkString}}" } val suffix = mode match { case Greedy => "" diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala index 4949daf5ad0..b3abc5b244f 100644 --- a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala @@ -25,7 +25,6 @@ import org.scalatest.funsuite.AnyFunSuite // Java replacement strings such as ${1} are intentionally not Scala interpolated strings. @scala.annotation.nowarn("cat=lint-missing-interpolator") class RegularExpressionParserSuite extends AnyFunSuite { - import RegexQuantifier._ test("detect regexp strings") { @@ -53,7 +52,7 @@ class RegularExpressionParserSuite extends AnyFunSuite { test("simple quantifier") { assert(parse("a{1}") === RegexSequence(ListBuffer( - RegexRepetition(RegexChar('a'), RegexQuantifier(Fixed(1)))))) + RegexRepetition(RegexChar('a'), RegexQuantifier(Fixed(1), Greedy))))) } test("bounded reluctant quantifiers have semantic modes") { @@ -70,8 +69,9 @@ class RegularExpressionParserSuite extends AnyFunSuite { } } - test("issue-15831: quantifier base and mode are independent semantic dimensions") { - val bases: Seq[(RegexQuantifier.Base, String, Int)] = Seq( + test("quantifier base and mode are independent semantic dimensions") { + val bases = Seq( + // base, regex string, minimum match length (ZeroOrOne, "?", 0), (ZeroOrMore, "*", 0), (OneOrMore, "+", 1), @@ -79,6 +79,7 @@ class RegularExpressionParserSuite extends AnyFunSuite { (Variable(2, None), "{2,}", 2), (Variable(2, Some(3)), "{2,3}", 2)) val modes = Seq( + // mode, regex suffix (Greedy, ""), (Reluctant, "?"), (Possessive, "+")) @@ -88,13 +89,13 @@ class RegularExpressionParserSuite extends AnyFunSuite { val quantifier = RegexQuantifier(base, mode) assert(quantifier.minLength === minLength) assert(quantifier.toRegexString === s"$baseString$suffix") + assert(parse(s"a$baseString$suffix") === + RegexSequence(ListBuffer(RegexRepetition(RegexChar('a'), quantifier)))) } } - val first = RegexQuantifier(Fixed(2)) - first.position = Some(1) - val second = RegexQuantifier(Fixed(2)) - second.position = Some(9) + val first = new RegexQuantifier(Fixed(2), Greedy, 1) + val second = new RegexQuantifier(Fixed(2), Greedy, 9) assert(first === second) } @@ -116,9 +117,9 @@ class RegularExpressionParserSuite extends AnyFunSuite { // Regression test for https://github.com/NVIDIA/cudf-spark/issues/15495 test("quantifier integer boundaries") { val supportedBoundaries: Seq[(String, RegexQuantifier)] = Seq( - s"a{${Int.MaxValue}}" -> RegexQuantifier(Fixed(Int.MaxValue)), - s"a{${Int.MaxValue},}" -> RegexQuantifier(Variable(Int.MaxValue, None)), - s"a{1,${Int.MaxValue}}" -> RegexQuantifier(Variable(1, Some(Int.MaxValue)))) + s"a{${Int.MaxValue}}" -> RegexQuantifier(Fixed(Int.MaxValue), Greedy), + s"a{${Int.MaxValue},}" -> RegexQuantifier(Variable(Int.MaxValue, None), Greedy), + s"a{1,${Int.MaxValue}}" -> RegexQuantifier(Variable(1, Some(Int.MaxValue)), Greedy)) supportedBoundaries.foreach { case (pattern, quantifier) => assert(new RegexParser(pattern).parseUnchecked() === RegexSequence(ListBuffer(RegexRepetition(RegexChar('a'), quantifier)))) @@ -234,7 +235,7 @@ class RegularExpressionParserSuite extends AnyFunSuite { RegexRepetition( RegexCharacterClass(negated = true, ListBuffer(RegexChar(']'), RegexChar('+'), RegexChar('d'))), - RegexQuantifier(OneOrMore))))) + RegexQuantifier(OneOrMore, Greedy))))) } test("character classes containing ']'") { @@ -285,7 +286,7 @@ class RegularExpressionParserSuite extends AnyFunSuite { RegexRepetition( RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('A'), RegexChar('Z')))), - RegexQuantifier(OneOrMore) + RegexQuantifier(OneOrMore, Greedy) ) )) ), @@ -364,28 +365,31 @@ class RegularExpressionParserSuite extends AnyFunSuite { test("repetition with group containing simple repetition") { assert(parse("(3?)+") === - RegexSequence(ListBuffer(RegexRepetition(RegexGroup(RegexGroup.Capturing, - RegexSequence(ListBuffer(RegexRepetition(RegexChar('3'), - RegexQuantifier(ZeroOrOne))))),RegexQuantifier(OneOrMore))))) + RegexSequence(ListBuffer( + RegexRepetition( + RegexGroup(RegexGroup.Capturing, + RegexSequence(ListBuffer( + RegexRepetition(RegexChar('3'), RegexQuantifier(ZeroOrOne, Greedy))))), + RegexQuantifier(OneOrMore, Greedy))))) } test("repetition with group containing escape character") { assert(parse(raw"(\A)+") === RegexSequence(ListBuffer(RegexRepetition(RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer(RegexEscaped('A')))), - RegexQuantifier(OneOrMore)))) + RegexQuantifier(OneOrMore, Greedy)))) ) assert(parse(raw"(?:\A)+") === RegexSequence(ListBuffer(RegexRepetition(RegexGroup(RegexGroup.NonCapturing, RegexSequence(ListBuffer(RegexEscaped('A')))), - RegexQuantifier(OneOrMore)))) + RegexQuantifier(OneOrMore, Greedy)))) ) } test("group containing choice with repetition") { assert(parse("(\t+|a)") == RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexChoice(RegexSequence(ListBuffer( - RegexRepetition(RegexChar('\t'),RegexQuantifier(OneOrMore)))), + RegexRepetition(RegexChar('\t'),RegexQuantifier(OneOrMore, Greedy)))), RegexSequence(ListBuffer(RegexChar('a')))))))) } @@ -406,7 +410,7 @@ class RegularExpressionParserSuite extends AnyFunSuite { test("group containing quantifier") { assert(parse("(?:a?)") === RegexSequence(ListBuffer( RegexGroup(RegexGroup.NonCapturing, RegexSequence(ListBuffer( - RegexRepetition(RegexChar('a'), RegexQuantifier(ZeroOrOne)))))))) + RegexRepetition(RegexChar('a'), RegexQuantifier(ZeroOrOne, Greedy)))))))) assert(parse("(?i:a)") === RegexSequence(ListBuffer( RegexGroup(RegexGroup.ScopedFlags(RegexFlagSet(Set(RegexFlag.CaseInsensitive), Set())), RegexSequence(ListBuffer(RegexChar('a'))))))) @@ -443,43 +447,44 @@ class RegularExpressionParserSuite extends AnyFunSuite { assert(ast === RegexSequence(ListBuffer(RegexChar('^'), RegexRepetition(RegexCharacterClass(negated = false, ListBuffer( - RegexChar('+'), RegexEscaped('-'))), RegexQuantifier(ZeroOrOne)), + RegexChar('+'), RegexEscaped('-'))), RegexQuantifier(ZeroOrOne, Greedy)), RegexGroup(RegexGroup.Capturing, RegexChoice(RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexChoice(RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexRepetition(RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - RegexQuantifier(OneOrMore))))))), + RegexQuantifier(OneOrMore, Greedy))))))), RegexChoice(RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexRepetition( RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - RegexQuantifier(ZeroOrMore)), RegexEscaped('.'), + RegexQuantifier(ZeroOrMore, Greedy)), RegexEscaped('.'), RegexRepetition( RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - RegexQuantifier(OneOrMore))))))), RegexSequence(ListBuffer( + RegexQuantifier(OneOrMore, Greedy))))))), RegexSequence(ListBuffer( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexRepetition( RegexCharacterClass(negated = false, ListBuffer( RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - RegexQuantifier(OneOrMore)), RegexEscaped('.'), + RegexQuantifier(OneOrMore, Greedy)), RegexEscaped('.'), RegexRepetition(RegexCharacterClass(negated = false, ListBuffer(RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - RegexQuantifier(ZeroOrMore)))))))))), + RegexQuantifier(ZeroOrMore, Greedy)))))))))), RegexRepetition( RegexGroup(RegexGroup.Capturing, RegexSequence(ListBuffer( RegexCharacterClass(negated = false, ListBuffer(RegexChar('e'), RegexChar('E'))), RegexRepetition(RegexCharacterClass(negated = false, - ListBuffer(RegexChar('+'), RegexEscaped('-'))),RegexQuantifier(ZeroOrOne)), + ListBuffer(RegexChar('+'), RegexEscaped('-'))), + RegexQuantifier(ZeroOrOne, Greedy)), RegexRepetition(RegexCharacterClass(negated = false, ListBuffer(RegexCharacterRange(RegexChar('0'), RegexChar('9')))), - RegexQuantifier(OneOrMore))))), RegexQuantifier(ZeroOrOne)), + RegexQuantifier(OneOrMore, Greedy))))), RegexQuantifier(ZeroOrOne, Greedy)), RegexRepetition(RegexCharacterClass(negated = false, ListBuffer( RegexChar('f'), RegexChar('F'), RegexChar('d'), RegexChar('D'))), - RegexQuantifier(ZeroOrOne))))))), + RegexQuantifier(ZeroOrOne, Greedy))))))), RegexChoice(RegexSequence(ListBuffer( RegexChar('I'), RegexChar('n'), RegexChar('f'))), RegexSequence(ListBuffer( diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala index 802feee39f6..5f8f5e38aa8 100644 --- a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala @@ -1715,14 +1715,14 @@ class FuzzRegExp(suggestedChars: String, skipKnownIssues: Boolean = true, private def repetition(depth: Int) = { import RegexQuantifier._ - val modes = Seq[Mode](Greedy, Reluctant, Possessive) + val modes = Seq(Greedy, Reluctant, Possessive) val mode = modes(rr.nextInt(modes.length)) RegexRepetition(generate(depth + 1), RegexQuantifier(quantifierBase, mode)) } private def quantifierBase: RegexQuantifier.Base = { import RegexQuantifier._ - val generators = Seq[() => Base]( + val generators = Seq( () => OneOrMore, () => ZeroOrMore, () => ZeroOrOne, From cac97d29996812fb273d416c11648fd5c199c869 Mon Sep 17 00:00:00 2001 From: Allen Xu Date: Wed, 9 Sep 2026 15:16:40 +0800 Subject: [PATCH 3/5] Address regex parser review follow-ups Signed-off-by: Allen Xu --- .../com/nvidia/spark/rapids/RegexParser.scala | 25 ++++++++++--------- .../rapids/RegularExpressionParserSuite.scala | 13 ++++++++++ .../RegularExpressionTranspilerSuite.scala | 6 ++--- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala index 9763bcbbfa0..352b8064ddd 100644 --- a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala +++ b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala @@ -171,17 +171,19 @@ class RegexParser(pattern: String) { case Some(',') => consumeExpected(',') val maxLength = consumeInt() - if (peek().contains('}') && maxLength.forall(_ >= minLength)) { - consumeExpected('}') - Some(Variable(minLength, maxLength)) - } else { - None + if (!peek().contains('}')) { + throw new PatternSyntaxException("Unclosed counted closure", pattern, pos) + } + maxLength.filter(_ < minLength).foreach { _ => + throw new PatternSyntaxException("Illegal repetition range", pattern, pos) } + consumeExpected('}') + Some(Variable(minLength, maxLength)) case Some('}') => consumeExpected('}') Some(Fixed(minLength)) case _ => - None + throw new PatternSyntaxException("Unclosed counted closure", pattern, pos) } } } @@ -1603,8 +1605,7 @@ class CudfRegexTranspiler(mode: RegexMode) { "regexp_split on GPU does not support empty match repetition consistently with Spark", quantifier.position) - case (_, RegexQuantifier(Variable(0, Some(0)) | Fixed(0), _)) - if mode != RegexFindMode => + case (_, RegexQuantifier(Variable(0, Some(0)) | Fixed(0), _)) if mode != RegexFindMode => throw new RegexUnsupportedException( s"regex_replace and regex_split on GPU do not support repetition with " + s"${quantifier.toRegexString}", @@ -2015,12 +2016,12 @@ object RegexQuantifier { case object Possessive extends Mode } -sealed case class RegexQuantifier( - base: RegexQuantifier.Base, - mode: RegexQuantifier.Mode) { +import RegexQuantifier.{Base, Mode} + +sealed case class RegexQuantifier(base: RegexQuantifier.Base, mode: RegexQuantifier.Mode) { import RegexQuantifier._ - def this(base: RegexQuantifier.Base, mode: RegexQuantifier.Mode, position: Int) = { + def this(base: Base, mode: Mode, position: Int) = { this(base, mode) this.position = Some(position) } diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala index b3abc5b244f..df02dc42b78 100644 --- a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionParserSuite.scala @@ -114,6 +114,19 @@ class RegularExpressionParserSuite extends AnyFunSuite { } } + test("unchecked parser matches Java errors for malformed counted quantifiers") { + Seq("a{3,4!", "a{3,2}").foreach { pattern => + val expected = intercept[PatternSyntaxException] { + java.util.regex.Pattern.compile(pattern) + } + val actual = intercept[PatternSyntaxException] { + new RegexParser(pattern).parseUnchecked() + } + assert(actual.getDescription === expected.getDescription, pattern) + assert(actual.getIndex === expected.getIndex, pattern) + } + } + // Regression test for https://github.com/NVIDIA/cudf-spark/issues/15495 test("quantifier integer boundaries") { val supportedBoundaries: Seq[(String, RegexQuantifier)] = Seq( diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala index 5f8f5e38aa8..a34345c8ca8 100644 --- a/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RegularExpressionTranspilerSuite.scala @@ -1729,10 +1729,8 @@ class FuzzRegExp(suggestedChars: String, skipKnownIssues: Boolean = true, () => Fixed(rr.nextInt(3)), () => Variable(rr.nextInt(3), None), () => { - // this intentionally generates some invalid quantifiers where the maxLength - // is less than the minLength, such as "{2,1}" which should be handled as a - // literal string match on "{2,1}" rather than as a valid quantifier. - Variable(rr.nextInt(3), Some(rr.nextInt(3))) + val minLength = rr.nextInt(3) + Variable(minLength, Some(minLength + rr.nextInt(3))) } ) generators(rr.nextInt(generators.length))() From dbff01effe5439901f2ce7bf1b3ba206761e7e79 Mon Sep 17 00:00:00 2001 From: Allen Xu Date: Wed, 9 Sep 2026 15:46:38 +0800 Subject: [PATCH 4/5] Fix RegexQuantifier import ordering Signed-off-by: Allen Xu --- .../src/main/scala/com/nvidia/spark/rapids/RegexParser.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala index 352b8064ddd..b588eef1da3 100644 --- a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala +++ b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala @@ -22,6 +22,7 @@ import scala.collection.mutable.ListBuffer import com.nvidia.spark.rapids.GpuOverrides.regexMetaChars import com.nvidia.spark.rapids.RegexParser.toReadableString +import com.nvidia.spark.rapids.RegexQuantifier.{Base, Mode} import org.apache.spark.unsafe.types.UTF8String @@ -2016,8 +2017,6 @@ object RegexQuantifier { case object Possessive extends Mode } -import RegexQuantifier.{Base, Mode} - sealed case class RegexQuantifier(base: RegexQuantifier.Base, mode: RegexQuantifier.Mode) { import RegexQuantifier._ From e76806599f06b95bb697ec54342c3245630e593c Mon Sep 17 00:00:00 2001 From: Allen Xu Date: Fri, 11 Sep 2026 07:38:09 +0800 Subject: [PATCH 5/5] Restore qualified RegexQuantifier constructor types Signed-off-by: Allen Xu --- .../src/main/scala/com/nvidia/spark/rapids/RegexParser.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala index b588eef1da3..d619f3c0f4c 100644 --- a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala +++ b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala @@ -22,7 +22,6 @@ import scala.collection.mutable.ListBuffer import com.nvidia.spark.rapids.GpuOverrides.regexMetaChars import com.nvidia.spark.rapids.RegexParser.toReadableString -import com.nvidia.spark.rapids.RegexQuantifier.{Base, Mode} import org.apache.spark.unsafe.types.UTF8String @@ -2020,7 +2019,7 @@ object RegexQuantifier { sealed case class RegexQuantifier(base: RegexQuantifier.Base, mode: RegexQuantifier.Mode) { import RegexQuantifier._ - def this(base: Base, mode: Mode, position: Int) = { + def this(base: RegexQuantifier.Base, mode: RegexQuantifier.Mode, position: Int) = { this(base, mode) this.position = Some(position) }