Enables `$`, `\z`, and `\Z` in `REGEXP_REPLACE` on the GPU #5491

NVnavkumar · 2022-05-14T01:19:23Z

This enables the end-of-line anchor $, end-of-input anchor \z, and end-of-input-before-line-termination anchor \Z to be used in regular expressions in REGEXP_REPLACE(...) on the GPU. This does this by doing minor manipulation on the replacement string in addition to regular expression transpiling. The idea is to potentially match a line termination in a capture group and use that as an available back-reference in the replacement string to preserve it in the output to match how Spark does this on the CPU.

…ng replacement if necessary for regexp_replace Signed-off-by: Navin Kumar <navink@nvidia.com>

Signed-off-by: Navin Kumar <navink@nvidia.com>

…roups Signed-off-by: Navin Kumar <navink@nvidia.com>

Signed-off-by: Navin Kumar <navink@nvidia.com>

…ring_anchors_regexp_replace

NVnavkumar · 2022-05-16T14:05:12Z

build

integration_tests/src/main/python/string_test.py

revans2 · 2022-05-16T14:36:02Z

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

+                    containsBackref = hasBackref
+                    replacement = Some(GpuRegExpUtils.unescapeReplaceString(convertedRep))
+                }
+              case _ =>


nit: If this is really a noop, then it is cleaner to to a foreach instead of a match

repl.foreach { rep => ... }

fixed via code suggestion that was applied

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

revans2 · 2022-05-16T14:40:53Z

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

@@ -583,6 +689,10 @@ class CudfRegexTranspiler(mode: RegexMode) {
              // Java, it's treated as a single (b$ and b$$ are synonymous), so we create 
              // an empty RegexAST that outputs to empty string
              RegexEmpty()
+            case Some(RegexChar(ch)) if mode == RegexReplaceMode
+                && lineTerminatorChars.contains(ch) =>
+                throw new RegexUnsupportedException("Regex sequences with a line terminator " 


Can we add a fallback test to verify that this is working as expected? Does this conflict with the test just below it? What happens if we have something like ABC$A\n is this going to do the right thing?

I will follow up on this in a separate PR today

I added a test for this in the follow-up PR #5519

This case statement is guarded on the condition mode == RegexReplaceMode and the one below it is not so there is no conflict between the two. The ch being tested here is prior to the $ so would not match the ABC$A\n case.

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

…ing anchors regex replace integration test Signed-off-by: Navin Kumar <navink@nvidia.com>

revans2

Still have a few nits that would be nice to have fixed, but they are not blockers.

NVnavkumar · 2022-05-16T18:23:33Z

build

Signed-off-by: Navin Kumar <navink@nvidia.com>

NVnavkumar · 2022-05-16T19:45:22Z

build

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

andygrove · 2022-05-17T14:51:39Z

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

+            repl match {
+              case Some(rep) =>
+                GpuRegExpUtils.backrefConversion(rep) match {
+                  case (hasBackref, convertedRep) =>
+                    containsBackref = hasBackref
+                    replacement = Some(GpuRegExpUtils.unescapeReplaceString(convertedRep))
+                }
+              case _ =>
+            }


Suggested change

repl match {

case Some(rep) =>

GpuRegExpUtils.backrefConversion(rep) match {

case (hasBackref, convertedRep) =>

containsBackref = hasBackref

replacement = Some(GpuRegExpUtils.unescapeReplaceString(convertedRep))

}

case _ =>

}

repl.map(GpuRegExpUtils.backrefConversion).foreach {

case (hasBackref, convertedRep) =>

containsBackref = hasBackref

replacement = Some(GpuRegExpUtils.unescapeReplaceString(convertedRep))

}

…laceMeta.scala

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

…scala

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

…scala

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

…scala

andygrove · 2022-05-17T14:58:33Z

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

@@ -853,7 +853,7 @@ class GpuRLikeMeta(
        case Literal(str: UTF8String, DataTypes.StringType) if str != null =>
          try {
            // verify that we support this regex and can transpile it to cuDF format
-            pattern = Some(new CudfRegexTranspiler(RegexFindMode).transpile(str.toString))
+            pattern = (new CudfRegexTranspiler(RegexFindMode).transpile(str.toString, None))._1


nit: it isn't obvious when reading this code what _1 represents - I will follow up with a separate PR to make this clearer

andygrove · 2022-05-17T15:06:10Z

build

andygrove

LGTM overall. I will follow up with a PR to fix outstanding feedback from this PR.

andygrove · 2022-05-17T18:42:46Z

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

  /**
   * Parse Java regular expression and translate into cuDF regular expression.
   *
   * @param pattern Regular expression that is valid in Java's engine
   * @return Regular expression in cuDF format
   */
-  def transpile(pattern: String): String = {
+  def transpile(pattern: String, repl: Option[String]): (Option[String], Option[String]) = {


I don't think we need the first item in the tuple to be optional since we always return a value

NVnavkumar added 7 commits May 10, 2022 17:00

WIP: adding replacement parameter to transpiler, to support transpili…

37d6af2

…ng replacement if necessary for regexp_replace Signed-off-by: Navin Kumar <navink@nvidia.com>

WIP: replacement parameter in transpiler

8bfd426

Signed-off-by: Navin Kumar <navink@nvidia.com>

Parse regexp replacement string

0064bac

Signed-off-by: Navin Kumar <navink@nvidia.com>

WIP: handling anchors in replacement, some corner cases to handle

9cb23c3

Signed-off-by: Navin Kumar <navink@nvidia.com>

WIP: anchors in replacement, now handling previouly defined capture g…

8248507

…roups Signed-off-by: Navin Kumar <navink@nvidia.com>

Updated regexp_replace meta and added integration test

19ef5b5

Signed-off-by: Navin Kumar <navink@nvidia.com>

Merge branch 'branch-22.06' of github.com:NVIDIA/spark-rapids into st…

b4f4dfd

…ring_anchors_regexp_replace

NVnavkumar self-assigned this May 14, 2022

NVnavkumar requested a review from andygrove May 14, 2022 01:19

sameerz added the feature request New feature or request label May 15, 2022

sameerz added this to the May 2 - May 20 milestone May 15, 2022

revans2 reviewed May 16, 2022

View reviewed changes

Deleted some commented out code, and added more special cases for str…

5554919

…ing anchors regex replace integration test Signed-off-by: Navin Kumar <navink@nvidia.com>

revans2 previously approved these changes May 16, 2022

View reviewed changes

Fix scala test for regex_replace

ec2d72d

Signed-off-by: Navin Kumar <navink@nvidia.com>

NVnavkumar dismissed revans2’s stale review via ec2d72d May 16, 2022 19:45

andygrove reviewed May 17, 2022

View reviewed changes

Update sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuRegExpRep…

9436994

…laceMeta.scala

andygrove reviewed May 17, 2022

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala Outdated Show resolved Hide resolved

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

14cb8a2

…scala

andygrove reviewed May 17, 2022

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala Outdated Show resolved Hide resolved

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

370bafc

…scala

andygrove reviewed May 17, 2022

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RegexParser.scala Outdated Show resolved Hide resolved

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

3022e04

…scala

andygrove reviewed May 17, 2022

View reviewed changes

andygrove approved these changes May 17, 2022

View reviewed changes

andygrove reviewed May 17, 2022

View reviewed changes

andygrove mentioned this pull request May 17, 2022

Address feedback in "string anchors regexp replace" PR #5519

Merged

andygrove merged commit a1fb914 into NVIDIA:branch-22.06 May 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enables `$`, `\z`, and `\Z` in `REGEXP_REPLACE` on the GPU #5491

Enables `$`, `\z`, and `\Z` in `REGEXP_REPLACE` on the GPU #5491

NVnavkumar commented May 14, 2022

NVnavkumar commented May 16, 2022

revans2 May 16, 2022

andygrove May 17, 2022

revans2 May 16, 2022

andygrove May 17, 2022

andygrove May 17, 2022

revans2 left a comment

NVnavkumar commented May 16, 2022

NVnavkumar commented May 16, 2022

andygrove May 17, 2022

andygrove May 17, 2022

andygrove commented May 17, 2022

andygrove left a comment

andygrove May 17, 2022

Enables $, \z, and \Z in REGEXP_REPLACE on the GPU #5491

Enables $, \z, and \Z in REGEXP_REPLACE on the GPU #5491

Conversation

NVnavkumar commented May 14, 2022

NVnavkumar commented May 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revans2 left a comment

Choose a reason for hiding this comment

NVnavkumar commented May 16, 2022

NVnavkumar commented May 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented May 17, 2022

andygrove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enables `$`, `\z`, and `\Z` in `REGEXP_REPLACE` on the GPU #5491

Enables `$`, `\z`, and `\Z` in `REGEXP_REPLACE` on the GPU #5491