Fix regressions related to cuDF changes in handline of end-of-line/string anchors #7211
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Andy Grove <andygrove@nvidia.com>
|
build |
|
There was a test failure against Spark 3.1.1. I will investigate: |
…d the integration test failure but there are now a few failing java unit tests
|
build |
andygrove
commented
Jan 26, 2023
andygrove
commented
Jan 26, 2023
Comment on lines
-460
to
-463
| The following regular expression patterns are known to potentially produce different results on the GPU | ||
| vs the CPU. | ||
|
|
||
| - Word and non-word boundaries, `\b` and `\B` |
There was a problem hiding this comment.
We had two sections listing known edge cases, so I consolidated them by moving this content.
|
@NVnavkumar This is ready for review now |
NVnavkumar
reviewed
Jan 26, 2023
docs/compatibility.md
Outdated
| - Word and non-word boundaries, `\b` and `\B` | ||
| - Line anchor `$` will incorrectly match any of the unicode characters `\u0085`, `\u2028`, or `\u2029` followed by | ||
| another line-terminator, such as `\n`. For example, the pattern `TEST$` will match `TEST\u0085\n` on the GPU but | ||
| not on the CPU ([#7585](https://github.com/NVIDIA/spark-rapids/issues/7585)). | ||
|
|
||
| The following regular expression patterns are not yet supported on the GPU and will fall back to the CPU. | ||
|
|
||
| - Line anchor `^` is not supported in some contexts, such as when combined with a choice (`^|a`). | ||
| - Line anchor `$` is not supported by `regexp_replace`, and in some rare contexts. |
There was a problem hiding this comment.
Is this true now? We still have integration tests for it.
There was a problem hiding this comment.
Good catch. Updated this line.
| @@ -1144,8 +1143,8 @@ class CudfRegexTranspiler(mode: RegexMode) { | |||
| case 'z' if mode == RegexSplitMode => | |||
There was a problem hiding this comment.
We left this split case for \z, but the error message just claims that \z is not supported on GPU. We should either remove this case, or clarify the error messaging.
NVnavkumar
approved these changes
Jan 26, 2023
|
build |
andygrove
added a commit
to andygrove/spark-rapids
that referenced
this pull request
Jan 27, 2023
…-line/string anchors (NVIDIA#7211)" This reverts commit 3398daa.
andygrove
added a commit
to andygrove/spark-rapids
that referenced
this pull request
Jan 27, 2023
…-line/string anchors (NVIDIA#7211)" This reverts commit 3398daa. Signed-off-by: Andy Grove <andygrove@nvidia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #7090
Rationale
Now that rapidsai/cudf#11979 is resolved using the fix described rapidsai/cudf#11979 (comment), the regular expression transpiler code needed to be updated for new handling of $, \z and \Z.
Follow-on issue:
Changes in this PR:
$to reflect recent changes in cuDF\zbecause there is no longer an equivalent in cuDF that we can transpile to\zfallback to CPU\u0085,\u2028, and\u2029