[SPARK-33386][SQL] Accessing array elements in ElementAt/Elt/GetArrayItem should failed if index is out of bound #30297

leanken-zz · 2020-11-09T11:36:30Z

What changes were proposed in this pull request?

Instead of returning NULL, throws runtime ArrayIndexOutOfBoundsException when ansiMode is enable for element_at，elt, GetArrayItem functions.

Why are the changes needed?

For ansiMode.

Does this PR introduce any user-facing change?

When spark.sql.ansi.enabled = true, Spark will throw ArrayIndexOutOfBoundsException if out-of-range index when accessing array elements

How was this patch tested?

Added UT and existing UT.

SparkQA · 2020-11-09T12:31:40Z

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

SparkQA · 2020-11-09T12:54:25Z

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

SparkQA · 2020-11-09T16:10:17Z

Test build #130781 has finished for PR 30297 at commit d1e0ae3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ElementAt(
case class GetArrayItem(
case class Elt(

dongjoon-hyun · 2020-11-10T03:14:33Z

cc @maropu

maropu · 2020-11-10T05:55:29Z

I think we need to update the ANSI doc accordingly if we change the behaviours: https://github.com/apache/spark/blame/master/docs/sql-ref-ansi-compliance.md#L110-L114 btw, is this change related to ANSI? The ANSI standard says something about these kinds of behaviours? @gengliangwang

maropu · 2020-11-10T05:58:40Z

Also, its better to update the doc of the ANSI config (e.g., 3. Spark will change the behavior of some SQL functions, e.g., size, ...), too.

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

Lines 2145 to 2149 in 90f6f39

    
           val ANSI_ENABLED = buildConf("spark.sql.ansi.enabled") 
        
             .doc("When true, Spark tries to conform to the ANSI SQL specification: 1. Spark will " + 
        
               "throw a runtime exception if an overflow occurs in any operation on integral/decimal " + 
        
               "field. 2. Spark will forbid using the reserved keywords of ANSI SQL as identifiers in " + 
        
               "the SQL parser.")

maropu · 2020-11-10T05:59:11Z

sql/core/src/test/scala/org/apache/spark/sql/StringFunctionsSuite.scala

+          }
+          assert(ex.getMessage.contains("Invalid index: 4"))
+        } else {
+          checkAnswer(


nit: could you put this in a single line?

checkAnswer(df, Row(null))

Could you add tests for -1 or 0?

maropu · 2020-11-10T05:59:25Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

+          }
+          assert(ex.getMessage.contains("Invalid index: 5"))
+        } else {
+          checkAnswer(


The same: https://github.com/apache/spark/pull/30297/files#r520305448

Could you add fine-grained tests for element_at and elt via checkExceptionInExpression?

maropu · 2020-11-10T06:00:41Z

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

@@ -1919,9 +1919,14 @@ case class ArrayPosition(left: Expression, right: Expression)
       b
  """,
  since = "2.4.0")
-case class ElementAt(left: Expression, right: Expression)
+case class ElementAt(


Please update the usage above of ExpressionDescription , too?

maropu · 2020-11-10T06:05:15Z

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

@@ -239,7 +240,11 @@ case class ConcatWs(children: Seq[Expression])
  """,
  since = "2.0.0")
 // scalastyle:on line.size.limit
-case class Elt(children: Seq[Expression]) extends Expression {
+case class Elt(


The same: https://github.com/apache/spark/pull/30297/files#r520305929

maropu · 2020-11-10T06:06:58Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala

+    Seq(true, false).foreach { ansiEnabled =>
+      withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+        val typeA = ArrayType(StringType)
+        val array = Literal.create(Seq("a", "b"), typeA)


nit: val array = Literal.create(Seq("a", "b"), ArrayType(StringType))?

maropu · 2020-11-10T06:09:55Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala

+          val ex = intercept[Exception] {
+            checkEvaluation(GetArrayItem(array, Literal(5)), null)
+          }
+          assert(stackTraceToString(ex).contains("Invalid index: 5"))


Could you add tests for -1?

maropu · 2020-11-10T06:13:48Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala

+
+        if (ansiEnabled) {
+          val ex = intercept[Exception] {
+            checkEvaluation(GetArrayItem(array, Literal(5)), null)


Could we use checkExceptionInExpression instead?

Change-Id: If7650cc45aa30fd3d6549536a8af6ca01a746c39

2. add more UT scenario. 3. update nullability calculation for ElementAt and update UT. Change-Id: Ieec69fe5171dfee2863ad93f84bb660076214de0

leanken-zz · 2020-11-10T12:45:31Z

@maropu updated with all your comments, plz have a look when you have time. ^_^
And also FYI, @cloud-fan

cloud-fan · 2020-11-10T13:02:47Z

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

-      accesses elements from the last to the first. Returns NULL if the index exceeds the length
-      of the array.
+      accesses elements from the last to the first. If the index exceeds the length of the array,
+      Returns NULL if Ansi mode is off; Throws ArrayIndexOutOfBoundsException when Ansi mode is on.


Ansi -> ANSI

cloud-fan · 2020-11-10T13:03:22Z

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

@@ -1969,7 +1974,7 @@ case class ElementAt(left: Expression, right: Expression)
    if (ordinal == 0) {
      false
    } else if (elements.length < math.abs(ordinal)) {
-      true
+      if (failOnError) false else true


nit: !failOnFalse

cloud-fan · 2020-11-10T13:05:38Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

-        if ($index >= $eval1.numElements() || $index < 0$nullCheck) {
+        if ($index >= $eval1.numElements() || $index < 0) {
+          $failOnErrorBranch
+        } else if (false$nullCheck) {


false$nullCheck -> $nullCheck

OK, we can get rid of the entire else if (false$nullCheck) if containsNull == false.

cloud-fan · 2020-11-10T13:07:12Z

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

-  usage = "_FUNC_(n, input1, input2, ...) - Returns the `n`-th input, e.g., returns `input2` when `n` is 2.",
+  usage = """
+    _FUNC_(n, input1, input2, ...) - Returns the `n`-th input, e.g., returns `input2` when `n` is 2.
+    If the index exceeds the length of the array, Returns NULL if Ansi mode is off;


ditto, Ansi -> ANSI

cloud-fan · 2020-11-10T13:11:11Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -2146,7 +2146,9 @@ object SQLConf {
    .doc("When true, Spark tries to conform to the ANSI SQL specification: 1. Spark will " +
      "throw a runtime exception if an overflow occurs in any operation on integral/decimal " +
      "field. 2. Spark will forbid using the reserved keywords of ANSI SQL as identifiers in " +
-      "the SQL parser.")
+      "the SQL parser. 3. Spark will returns null for null input for function `size`. " +
+      "4. Spark will throw ArrayIndexOutOfBoundsException if invalid indices " +


We can merge 1 and 4: 1. Spark will throw an exception at runtime if the inputs to a SQL operator/function are invalid, e.g. overflow in arithmetic operations, out-of-range index when accessing array elements.

cloud-fan · 2020-11-10T13:13:27Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

+
+        // CreateArray case invalid indices
+        assert(!ElementAt(array, Literal(0)).nullable)
+        if (ansiEnabled) {


nit: assert(ElementAt(array, Literal(4)).nullable == !ansiEnabled)

cloud-fan · 2020-11-10T13:13:40Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

+
+        // GetArrayStructFields case invalid indices
+        assert(!ElementAt(stArray3, Literal(0)).nullable)
+        if (ansiEnabled) {


cloud-fan · 2020-11-10T13:25:45Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

@@ -3621,6 +3621,40 @@ class DataFrameFunctionsSuite extends QueryTest with SharedSparkSession {
      df.select(map(map_entries($"m"), lit(1))),
      Row(Map(Seq(Row(1, "a")) -> 1)))
  }
+
+  test("SPARK-33391: element_at ArrayIndexOutOfBoundsException") {


Let's not mix unit test and end-to-end test. Let's add UT in CollectionExpressionsSuite(for ElementAt) and StringExpressionsSuite (for Elt).

For end-to-end test, we can put them in DataFrameSuite for all the three functions/operators.

SparkQA · 2020-11-10T13:39:14Z

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

SparkQA · 2020-11-10T14:02:38Z

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

SparkQA · 2020-11-10T16:57:10Z

Test build #130868 has finished for PR 30297 at commit 7dcba56.

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

cloud-fan · 2020-11-11T14:16:36Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

@@ -2567,6 +2567,97 @@ class DataFrameSuite extends QueryTest
    val df = l.join(r, $"col2" === $"col4", "LeftOuter")
    checkAnswer(df, Row("2", "2"))
  }
+
+  test("SPARK-33391: element_at ArrayIndexOutOfBoundsException") {


Actually, can we follow datetime.sql, which is run twice with and without ANSI mode? array.sql may be a good place to put the new end-to-end tests.

+1 for @cloud-fan 's suggestion.

If we move to array.sql, SPARK-33391 will be removed accordingly.

Sounds good to me too.

gengliangwang · 2020-11-11T15:03:52Z

@leanken could you update the PR description. I think there is user-facing change when ANSI mode is enabled.

dongjoon-hyun · 2020-11-11T15:23:42Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

@@ -1883,4 +1887,32 @@ class CollectionExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper
      Literal(stringToInterval("interval 1 year"))),
      Seq(Date.valueOf("2018-01-01")))
  }
+
+  test("SPARK-33391: element_at ArrayIndexOutOfBoundsException") {


Maybe, SPARK-33386 is better than SPARK-33391 because this test name has ArrayIndexOutOfBoundsException.

dongjoon-hyun · 2020-11-11T15:24:19Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala

@@ -62,6 +62,29 @@ class ComplexTypeSuite extends SparkFunSuite with ExpressionEvalHelper {
    checkEvaluation(GetArrayItem(nestedArray, Literal(0)), Seq(1))
  }

+  test("SPARK-33391: GetArrayItem ArrayIndexOutOfBoundsException") {


ditto SPARK-33386.

dongjoon-hyun · 2020-11-11T15:24:27Z

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

@@ -968,4 +968,34 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
    GenerateUnsafeProjection.generate(
      Sentences(Literal("\"quote"), Literal("\"quote"), Literal("\"quote")) :: Nil)
  }
+
+  test("SPARK-33391: elt ArrayIndexOutOfBoundsException") {


ditto SPARK-33386.

dongjoon-hyun · 2020-11-11T15:27:50Z

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

@@ -2042,10 +2053,17 @@ case class ElementAt(left: Expression, right: Expression)
          } else {
            ""
          }
+
+          val failOnErrorBranch = if (failOnError) {
+            s"""throw new ArrayIndexOutOfBoundsException("Invalid index: " + $index);""".stripMargin


Shall we remove .stripMargin because this is a single line?

dongjoon-hyun · 2020-11-11T15:28:06Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

      } else {
        ""
      }
+
+      val failOnErrorBranch = if (failOnError) {
+        s"""throw new ArrayIndexOutOfBoundsException("Invalid index: " + $index);""".stripMargin


Shall we remove .stripMargin because this is a single line?

viirya · 2020-11-11T17:25:21Z

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

+      accesses elements from the last to the first. The function returns NULL
+      if the index exceeds the length of the array and `spark.sql.ansi.enabled` is set to false.
+      If `spark.sql.ansi.enabled` is set to true, it throws ArrayIndexOutOfBoundsException
+      for invalid indices.

    _FUNC_(map, key) - Returns value for given key, or NULL if the key is not contained in the map


Do we want to make ElementAtbehavior consistent on map type?

it's about to support ANSI mode for map type in next PR.

sunchao · 2020-11-11T18:08:30Z

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

@@ -2008,7 +2015,11 @@ case class ElementAt(left: Expression, right: Expression)
        val array = value.asInstanceOf[ArrayData]
        val index = ordinal.asInstanceOf[Int]
        if (array.numElements() < math.abs(index)) {
-          null
+          if (failOnError) {
+            throw new ArrayIndexOutOfBoundsException(s"Invalid index: $index")


nit: can we include the total number of elements too in the error message? sometimes that is helpful for debugging.

sunchao · 2020-11-11T18:11:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+      "throw an exception at runtime if the inputs to a SQL operator/function are invalid, " +
+      "e.g. overflow in arithmetic operations, out-of-range index when accessing array elements. " +
+      "2. Spark will forbid using the reserved keywords of ANSI SQL as identifiers in " +
+      "the SQL parser. 3. Spark will returns null for null input for function `size`.")


will returns -> will return, and perhaps capital NULL?

2. move DataFrameSuite code into array.sql and ansi/array.sql 3. add numElements to exception message. 4. other code refine. Change-Id: Ieb322ed7b036fc3322fd3b814c8508bfef266378

leanken-zz · 2020-11-12T03:55:22Z

@sunchao @dongjoon-hyun @cloud-fan updated, resolved your comments.

1. fix GetArrayItem nullablity issue.
2. move DataFrameSuite code into array.sql and ansi/array.sql
3. add numElements to exception message.
4. other code refine.

leanken-zz · 2020-11-12T03:55:47Z

@leanken could you update the PR description. I think there is user-facing change when ANSI mode is enabled.

updated.

SparkQA · 2020-11-12T04:44:14Z

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

cloud-fan · 2020-11-12T04:49:26Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

      } else {
        ""
      }
+
+      val failOnErrorBranch = if (failOnError) {


nit: it should be indexOutOfBoundBranch

cloud-fan · 2020-11-12T04:49:36Z

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

@@ -2042,10 +2039,20 @@ case class ElementAt(left: Expression, right: Expression)
          } else {
            ""
          }
+
+          val failOnErrorBranch = if (failOnError) {


nit: it should be indexOutOfBoundBranch

cloud-fan · 2020-11-12T04:50:23Z

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

@@ -323,6 +338,17 @@ case class Elt(children: Seq[Expression]) extends Expression {
         """.stripMargin
      }.mkString)

+    val failOnErrorBranch = if (failOnError) {


nit: it should be indexOutOfBoundBranch

SparkQA · 2020-11-12T05:14:06Z

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

Change-Id: I719078352ab04f3ab096e62fd2b6a1c06bbdffd3

SparkQA · 2020-11-12T06:16:45Z

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

SparkQA · 2020-11-12T06:41:32Z

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

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

Test build #130962 has finished for PR 30297 at commit 36d235a.

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

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

Test build #130968 has finished for PR 30297 at commit 4b161a4.

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

cloud-fan · 2020-11-12T08:50:23Z

GA passed, merging to master, thanks!

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

maropu reviewed Nov 10, 2020

View reviewed changes

leanken-zz added 2 commits November 10, 2020 18:46

Accessing array elements should failed if index is out of bound.

da4f5ee

Change-Id: If7650cc45aa30fd3d6549536a8af6ca01a746c39

1. update doc.

7dcba56

2. add more UT scenario. 3. update nullability calculation for ElementAt and update UT. Change-Id: Ieec69fe5171dfee2863ad93f84bb660076214de0

leanken-zz force-pushed the leanken-SPARK-33386 branch from d1e0ae3 to 7dcba56 Compare November 10, 2020 12:44

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

cloud-fan reviewed Nov 10, 2020

View reviewed changes

cloud-fan reviewed Nov 11, 2020

View reviewed changes

cloud-fan approved these changes Nov 11, 2020

View reviewed changes

dongjoon-hyun reviewed Nov 11, 2020

View reviewed changes

viirya reviewed Nov 11, 2020

View reviewed changes

sunchao reviewed Nov 11, 2020

View reviewed changes

1. fix GetArrayItem nullablity issue.

36d235a

2. move DataFrameSuite code into array.sql and ansi/array.sql 3. add numElements to exception message. 4. other code refine. Change-Id: Ieb322ed7b036fc3322fd3b814c8508bfef266378

cloud-fan reviewed Nov 12, 2020

View reviewed changes

cloud-fan approved these changes Nov 12, 2020

View reviewed changes

naming.

4b161a4

Change-Id: I719078352ab04f3ab096e62fd2b6a1c06bbdffd3

cloud-fan closed this in 6d31dae Nov 12, 2020

cloud-fan mentioned this pull request Nov 19, 2020

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

Closed

[SPARK-33386][SQL] Accessing array elements in ElementAt/Elt/GetArrayItem should failed if index is out of bound #30297

[SPARK-33386][SQL] Accessing array elements in ElementAt/Elt/GetArrayItem should failed if index is out of bound #30297

Conversation

leanken-zz commented Nov 9, 2020 • edited Loading

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?

SparkQA commented Nov 9, 2020

SparkQA commented Nov 9, 2020

SparkQA commented Nov 9, 2020

dongjoon-hyun commented Nov 10, 2020

maropu commented Nov 10, 2020

maropu commented Nov 10, 2020

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

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

leanken-zz commented Nov 10, 2020

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

cloud-fan Nov 10, 2020 • edited Loading

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

SparkQA commented Nov 10, 2020

SparkQA commented Nov 10, 2020

SparkQA commented Nov 10, 2020

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

gengliangwang commented Nov 11, 2020

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

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

leanken-zz commented Nov 12, 2020

leanken-zz commented Nov 12, 2020

SparkQA commented Nov 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 12, 2020

SparkQA commented Nov 12, 2020

SparkQA commented Nov 12, 2020

SparkQA commented Nov 12, 2020

SparkQA commented Nov 12, 2020

cloud-fan commented Nov 12, 2020

leanken-zz commented Nov 9, 2020 •

edited

Loading

cloud-fan Nov 10, 2020 •

edited

Loading