You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2023/01/04 05:55:54 UTC
[GitHub] [spark] panbingkun opened a new pull request, #39383: [SPARK-41780][SQL] Should throw INVALID_PARAMETER_VALUE when the parameters `regexp` in regexp_replace is invalid
panbingkun opened a new pull request, #39383:
URL: https://github.com/apache/spark/pull/39383
### What changes were proposed in this pull request?
### Why are the changes needed?
### Does this PR introduce _any_ user-facing change?
Yes.
### How was this patch tested?
Add new UT.
Pass GA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #39383: [SPARK-41780][SQL] Should throw INVALID_PARAMETER_VALUE when the parameters `regexp` in regexp_replace is invalid
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #39383:
URL: https://github.com/apache/spark/pull/39383#discussion_r1061705413
##########
sql/core/src/test/scala/org/apache/spark/sql/StringFunctionsSuite.scala:
##########
@@ -663,4 +664,18 @@ class StringFunctionsSuite extends QueryTest with SharedSparkSession {
start = 7,
stop = 47))
}
+
+ test("SPARK-41780: INVALID_PARAMETER_VALUE - invalid parameters `regexp` in regexp_replace") {
Review Comment:
Just found one more test maybe related to this changes:
https://github.com/apache/spark/blob/d0a598922e9df8ea90125d22c241810feab06017/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/RegexpExpressionsSuite.scala#L519-L536
Maybe can we address this together in this work as well ??
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] panbingkun commented on a diff in pull request #39383: [SPARK-41780][SQL] Should throw INVALID_PARAMETER_VALUE when the parameters `regexp` in regexp_replace is invalid
Posted by GitBox <gi...@apache.org>.
panbingkun commented on code in PR #39383:
URL: https://github.com/apache/spark/pull/39383#discussion_r1062048545
##########
sql/core/src/test/scala/org/apache/spark/sql/StringFunctionsSuite.scala:
##########
@@ -663,4 +664,18 @@ class StringFunctionsSuite extends QueryTest with SharedSparkSession {
start = 7,
stop = 47))
}
+
+ test("SPARK-41780: INVALID_PARAMETER_VALUE - invalid parameters `regexp` in regexp_replace") {
Review Comment:
Ok, Let me use `checkErrorInExpression` replace `checkExceptionInExpression ` to check exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #39383: [SPARK-41780][SQL] Should throw INVALID_PARAMETER_VALUE when the parameters `regexp` in regexp_replace is invalid
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #39383:
URL: https://github.com/apache/spark/pull/39383#issuecomment-1373442492
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] panbingkun commented on pull request #39383: [SPARK-41780][SQL] Should throw INVALID_PARAMETER_VALUE when the parameters `regexp` in regexp_replace is invalid
Posted by GitBox <gi...@apache.org>.
panbingkun commented on PR #39383:
URL: https://github.com/apache/spark/pull/39383#issuecomment-1370526974
cc @MaxGekk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gerashegalov commented on a diff in pull request #39383: [SPARK-41780][SQL] Should throw INVALID_PARAMETER_VALUE when the parameters `regexp` in regexp_replace is invalid
Posted by GitBox <gi...@apache.org>.
gerashegalov commented on code in PR #39383:
URL: https://github.com/apache/spark/pull/39383#discussion_r1061843543
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -634,7 +634,12 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
if (!p.equals(lastRegex)) {
// regex value changed
lastRegex = p.asInstanceOf[UTF8String].clone()
- pattern = Pattern.compile(lastRegex.toString)
+ try {
+ pattern = Pattern.compile(lastRegex.toString)
+ } catch {
+ case e: PatternSyntaxException =>
+ throw QueryExecutionErrors.invalidPatternError(prettyName, e.getPattern)
Review Comment:
it would be useful to have another version of this method passing `e` as the cause to SparkRuntimeException because it contains useful diagnostics for the user:
```
java.util.regex.PatternSyntaxException: Unclosed counted closure near index 8
[a\d]{0, 2}
```
##########
sql/core/src/test/scala/org/apache/spark/sql/StringFunctionsSuite.scala:
##########
@@ -663,4 +664,18 @@ class StringFunctionsSuite extends QueryTest with SharedSparkSession {
start = 7,
stop = 47))
}
+
+ test("SPARK-41780: INVALID_PARAMETER_VALUE - invalid parameters `regexp` in regexp_replace") {
+ checkError(
+ exception = intercept[SparkRuntimeException] {
+ sql("select regexp_replace('', '[a\\\\d]{0, 2}', 'x')").collect()
+ },
+ errorClass = "INVALID_PARAMETER_VALUE",
+ parameters = Map(
+ "parameter" -> "regexp",
+ "functionName" -> "`regexp_replace`",
+ "expected" -> "'[a\\\\d]{0, 2}'"
Review Comment:
Looks like the parameter name is misnamed in INVALID_PARAMETER_VALUE https://github.com/apache/spark/blob/f352f103ed512806abb3f642571a0c595b8b0509/core/src/main/resources/error/error-classes.json#L792
It should probably called something like `actual` or `invalid` , it's anything but `expected`
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -634,7 +634,12 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
if (!p.equals(lastRegex)) {
// regex value changed
lastRegex = p.asInstanceOf[UTF8String].clone()
- pattern = Pattern.compile(lastRegex.toString)
+ try {
+ pattern = Pattern.compile(lastRegex.toString)
+ } catch {
+ case e: PatternSyntaxException =>
+ throw QueryExecutionErrors.invalidPatternError(prettyName, e.getPattern)
Review Comment:
What about other instances of `Pattern.compile` in this class such as `regexp_extract`?
also `rlike` does not hide PatternSyntaxException but does not use QueryExecutionErrors.invalidPatternError
```
scala> sql("select rlike('', '[a\\\\d]{0, 2}')").collect()
java.util.regex.PatternSyntaxException: Unclosed counted closure near index 8
[a\d]{0, 2}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #39383: [SPARK-41780][SQL] Should throw INVALID_PARAMETER_VALUE.PATTERN when the parameters `regexp` is invalid
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #39383:
URL: https://github.com/apache/spark/pull/39383#issuecomment-1375268627
+1, LGTM. Merging to master.
Thank you, @panbingkun and @itholic @gerashegalov for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk closed pull request #39383: [SPARK-41780][SQL] Should throw INVALID_PARAMETER_VALUE.PATTERN when the parameters `regexp` is invalid
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #39383: [SPARK-41780][SQL] Should throw INVALID_PARAMETER_VALUE.PATTERN when the parameters `regexp` is invalid
URL: https://github.com/apache/spark/pull/39383
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org