[SPARK-33468][SQL] ParseUrl in ANSI mode should fail if input string is not a valid url #30399

ulysses-you · 2020-11-17T16:32:50Z

What changes were proposed in this pull request?

With ParseUrl, instead of return null we throw exception if input string is not a vaild url.

Why are the changes needed?

For ANSI mode.

Does this PR introduce any user-facing change?

Yes, user will get exception if set spark.sql.ansi.enabled=true.

How was this patch tested?

Add test.

ulysses-you · 2020-11-17T16:34:08Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala

-      case e: URISyntaxException => null
+      // We fail on error if in ansi mode.
+      case e: URISyntaxException if SQLConf.get.ansiEnabled =>
+        throw new IllegalArgumentException(s"Find an invaild url string ${url.toString}", e)


To clarify, the raw exception message is something like Illegal character in query at index 33: https://a.b.c/index.php?params1=a|b&params2=x.

dongjoon-hyun · 2020-11-17T17:12:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala

@@ -1404,7 +1404,10 @@ case class ParseUrl(children: Seq[Expression])
    try {
      new URI(url.toString)
    } catch {
-      case e: URISyntaxException => null
+      // We fail on error if in ansi mode.


We can remove this comment.

dongjoon-hyun · 2020-11-17T17:12:42Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala

@@ -943,6 +943,21 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
    GenerateUnsafeProjection.generate(ParseUrl(Seq(Literal("\"quote"), Literal("\"quote"))) :: Nil)
  }

+  test("SPARK-33468: ParseUrl should fail if input string is not a valid url") {
+    // fail on error if in ansi mode.


We can remove this.

dongjoon-hyun · 2020-11-17T17:13:19Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala

@@ -943,6 +943,21 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
    GenerateUnsafeProjection.generate(ParseUrl(Seq(Literal("\"quote"), Literal("\"quote"))) :: Nil)
  }

+  test("SPARK-33468: ParseUrl should fail if input string is not a valid url") {


ParseUrl -> ParseUrl in ANSI mode?

SparkQA · 2020-11-17T17:20:22Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35829/

ulysses-you · 2020-11-17T17:27:44Z

@dongjoon-hyun thanks for the review. Updated the comment.

SparkQA · 2020-11-17T17:44:26Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35829/

SparkQA · 2020-11-17T18:10:47Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35830/

SparkQA · 2020-11-17T18:41:08Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35830/

SparkQA · 2020-11-17T21:03:13Z

Test build #131226 has finished for PR 30399 at commit 62bf87f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-11-17T22:01:53Z

cc @cloud-fan and @maropu

SparkQA · 2020-11-17T22:15:30Z

Test build #131227 has finished for PR 30399 at commit a408734.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-11-17T23:05:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala

@@ -1404,7 +1404,9 @@ case class ParseUrl(children: Seq[Expression])
    try {
      new URI(url.toString)
    } catch {
-      case e: URISyntaxException => null
+      case e: URISyntaxException if SQLConf.get.ansiEnabled =>


Please update the ANSI doc, too. https://github.com/apache/spark/blame/master/docs/sql-ref-ansi-compliance.md#L110

Missed here, updated !

Can we follow other expressions and add a failOnError: Boolean parameter? See #30297 (comment)

maropu · 2020-11-17T23:06:29Z

Looks fine otherwise.

SparkQA · 2020-11-18T05:03:57Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35848/

SparkQA · 2020-11-18T05:27:22Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35848/

SparkQA · 2020-11-18T08:05:02Z

Test build #131245 has finished for PR 30399 at commit 2202ca4.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-11-20T00:16:27Z

docs/sql-ref-ansi-compliance.md

@@ -114,6 +114,7 @@ The behavior of some SQL functions can be different under ANSI mode (`spark.sql.
  - `element_at`: This function throws `ArrayIndexOutOfBoundsException` if using invalid indices. 
  - `element_at`: This function throws `NoSuchElementException` if key does not exist in map. 
  - `elt`: This function throws `ArrayIndexOutOfBoundsException` if using invalid indices.
+  - `parse_url`: This function throws `IllegalArgumentException` if input string is not a valid url.


nit: input string -> an input string

Sorry missed here. Updated !

maropu · 2020-11-20T00:16:39Z

Looks ok otherwise.

SparkQA · 2020-11-20T00:59:08Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35980/

SparkQA · 2020-11-20T01:30:55Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35980/

SparkQA · 2020-11-20T02:14:09Z

Test build #131377 has finished for PR 30399 at commit 497015c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ParseUrl(children: Seq[Expression], failOnError: Boolean = SQLConf.get.ansiEnabled)

SparkQA · 2020-11-20T03:21:49Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35989/

SparkQA · 2020-11-20T03:24:45Z

Test build #131387 has finished for PR 30399 at commit ecd2143.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-11-20T03:47:43Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35989/

SparkQA · 2020-11-20T04:11:37Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35995/

SparkQA · 2020-11-20T04:39:16Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35995/

SparkQA · 2020-11-20T07:17:10Z

Test build #131391 has finished for PR 30399 at commit d7e437b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-11-20T13:23:01Z

thanks, merging to master!

ulysses-you · 2020-11-20T14:19:41Z

thanks for merging !

init

62bf87f

github-actions bot added the SQL label Nov 17, 2020

ulysses-you commented Nov 17, 2020

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-33468][SQL] ParseUrl should fail if input string is not a valid url~~ [SPARK-33468][SQL] ParseUrl in ANSI mode should fail if input string is not a valid url Nov 17, 2020

dongjoon-hyun reviewed Nov 17, 2020

View reviewed changes

ulysses-you added 2 commits November 18, 2020 01:26

remove comment

5aec76a

update title

a408734

maropu reviewed Nov 17, 2020

View reviewed changes

doc

2202ca4

github-actions bot added the DOCS label Nov 18, 2020

add parameter

497015c

maropu reviewed Nov 20, 2020

View reviewed changes

doc

ecd2143

maropu approved these changes Nov 20, 2020

View reviewed changes

add constructor

d7e437b

cloud-fan closed this in 3384bda Nov 20, 2020

ulysses-you deleted the SPARK-33468 branch November 22, 2021 10:26

[SPARK-33468][SQL] ParseUrl in ANSI mode should fail if input string is not a valid url #30399

[SPARK-33468][SQL] ParseUrl in ANSI mode should fail if input string is not a valid url #30399

Uh oh!

Conversation

ulysses-you commented Nov 17, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 17, 2020

Uh oh!

ulysses-you commented Nov 17, 2020

Uh oh!

SparkQA commented Nov 17, 2020

Uh oh!

SparkQA commented Nov 17, 2020

Uh oh!

SparkQA commented Nov 17, 2020

Uh oh!

SparkQA commented Nov 17, 2020

Uh oh!

dongjoon-hyun commented Nov 17, 2020

Uh oh!

SparkQA commented Nov 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented Nov 17, 2020

Uh oh!

SparkQA commented Nov 18, 2020

Uh oh!

SparkQA commented Nov 18, 2020

Uh oh!

SparkQA commented Nov 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

cloud-fan commented Nov 20, 2020

Uh oh!

ulysses-you commented Nov 20, 2020

Uh oh!

Uh oh!