You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "bersprockets (via GitHub)" <gi...@apache.org> on 2023/02/10 23:35:40 UTC
[GitHub] [spark] bersprockets opened a new pull request, #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
bersprockets opened a new pull request, #39970:
URL: https://github.com/apache/spark/pull/39970
### What changes were proposed in this pull request?
In the `DataType` instance returned by `ArrayInsert#dataType` and `ArrayAppend#dataType`, set `containsNull` to true if either
- the input array has `containsNull` set to true
- the expression to be inserted/appended is nullable.
### Why are the changes needed?
The following two queries return the wrong answer:
```
spark-sql> select array_insert(array(1, 2, 3, 4), 5, cast(null as int));
[1,2,3,4,0] <== should be [1,2,3,4,null]
Time taken: 3.879 seconds, Fetched 1 row(s)
spark-sql> select array_append(array(1, 2, 3, 4), cast(null as int));
[1,2,3,4,0] <== should be [1,2,3,4,null]
Time taken: 0.068 seconds, Fetched 1 row(s)
spark-sql>
```
The following two queries throw a `NullPointerException`:
```
spark-sql> select array_insert(array('1', '2', '3', '4'), 5, cast(null as string));
23/02/10 11:24:59 ERROR Executor: Exception in task 0.0 in stage 2.0 (TID 2)
java.lang.NullPointerException
at org.apache.spark.sql.catalyst.expressions.codegen.UnsafeWriter.write(UnsafeWriter.java:110)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.project_doConsume_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.processNext(Unknown Source)
...
spark-sql> select array_append(array('1', '2', '3', '4'), cast(null as string));
23/02/10 11:25:10 ERROR Executor: Exception in task 0.0 in stage 3.0 (TID 3)
java.lang.NullPointerException
at org.apache.spark.sql.catalyst.expressions.codegen.UnsafeWriter.write(UnsafeWriter.java:110)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.project_doConsume_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.processNext(Unknown Source)
...
spark-sql>
```
The bug arises because both `ArrayInsert` and `ArrayAppend` use the first child's data type as the function's data type. That is, it uses the first child's `containsNull` setting, regardless of whether the insert/append operation might produce an array containing a null value.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New unit tests.
--
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] HyukjinKwon commented on a diff in pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #39970:
URL: https://github.com/apache/spark/pull/39970#discussion_r1103909400
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -4840,7 +4840,10 @@ case class ArrayInsert(srcArrayExpr: Expression, posExpr: Expression, itemExpr:
override def third: Expression = itemExpr
override def prettyName: String = "array_insert"
- override def dataType: DataType = first.dataType
+ override def dataType: DataType = third.nullable match {
Review Comment:
Why don't we just do if-else :-)?
--
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] bersprockets commented on a diff in pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "bersprockets (via GitHub)" <gi...@apache.org>.
bersprockets commented on code in PR #39970:
URL: https://github.com/apache/spark/pull/39970#discussion_r1106529574
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -4840,7 +4840,7 @@ case class ArrayInsert(srcArrayExpr: Expression, posExpr: Expression, itemExpr:
override def third: Expression = itemExpr
override def prettyName: String = "array_insert"
- override def dataType: DataType = first.dataType
+ override def dataType: DataType = if (third.nullable) first.dataType.asNullable else first.dataType
Review Comment:
Edit: Adding comment to the correct line of code.
Turns out that `array_insert` can implicitly insert nulls into the array even when the value getting inserted is not null, e.g.:
```
spark-sql> select array_insert(array('1', '2', '3', '4'), -6, '5');
23/02/14 16:10:19 ERROR Executor: Exception in task 0.0 in stage 0.0 (TID 0)
java.lang.NullPointerException
at org.apache.spark.sql.catalyst.expressions.codegen.UnsafeWriter.write(UnsafeWriter.java:110)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.project_doConsume_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.processNext(Unknown Source)
at org.apache.spark.sql.execution.BufferedRowIterator.hasNext(BufferedRowIterator.java:43)
```
My current fix only covers the case where the user is explicitly inserting a null, not when nulls are being implicitly added.
I will follow up.
--
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] HyukjinKwon commented on pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39970:
URL: https://github.com/apache/spark/pull/39970#issuecomment-1427432990
Merged to master and branch-3.4.
--
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] HyukjinKwon commented on a diff in pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #39970:
URL: https://github.com/apache/spark/pull/39970#discussion_r1103909400
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -4840,7 +4840,10 @@ case class ArrayInsert(srcArrayExpr: Expression, posExpr: Expression, itemExpr:
override def third: Expression = itemExpr
override def prettyName: String = "array_insert"
- override def dataType: DataType = first.dataType
+ override def dataType: DataType = third.nullable match {
Review Comment:
Why don't we just do if-else :-)? e.g., `if (third.nullable) first.dataType.asNullable else first.dataType`
--
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] bersprockets commented on a diff in pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "bersprockets (via GitHub)" <gi...@apache.org>.
bersprockets commented on code in PR #39970:
URL: https://github.com/apache/spark/pull/39970#discussion_r1103391612
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala:
##########
@@ -2752,4 +2752,18 @@ class CollectionExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper
)
}
+
Review Comment:
Btw, there is an existing test to verify insert of a null value: https://github.com/apache/spark/blob/26f2026c8bb11064695ebf87b198a7dedf5bb73e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala#L2332
It succeeds only because the input array (`a1`) already has `containsNull` set to true:
https://github.com/apache/spark/blob/26f2026c8bb11064695ebf87b198a7dedf5bb73e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala#L46
--
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] HyukjinKwon commented on a diff in pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #39970:
URL: https://github.com/apache/spark/pull/39970#discussion_r1106536193
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -4840,7 +4840,7 @@ case class ArrayInsert(srcArrayExpr: Expression, posExpr: Expression, itemExpr:
override def third: Expression = itemExpr
override def prettyName: String = "array_insert"
- override def dataType: DataType = first.dataType
+ override def dataType: DataType = if (third.nullable) first.dataType.asNullable else first.dataType
Review Comment:
Thanks man!
--
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] HyukjinKwon closed pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
URL: https://github.com/apache/spark/pull/39970
--
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] bersprockets commented on a diff in pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "bersprockets (via GitHub)" <gi...@apache.org>.
bersprockets commented on code in PR #39970:
URL: https://github.com/apache/spark/pull/39970#discussion_r1103391612
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala:
##########
@@ -2752,4 +2752,18 @@ class CollectionExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper
)
}
+
Review Comment:
There is an existing test to verify insert of a null value: https://github.com/apache/spark/blob/26f2026c8bb11064695ebf87b198a7dedf5bb73e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala#L2332
It succeeds only because the input array (`a1`) already has `containsNull` set to true:
https://github.com/apache/spark/blob/26f2026c8bb11064695ebf87b198a7dedf5bb73e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala#L46
--
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] bersprockets commented on a diff in pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "bersprockets (via GitHub)" <gi...@apache.org>.
bersprockets commented on code in PR #39970:
URL: https://github.com/apache/spark/pull/39970#discussion_r1103936402
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -4840,7 +4840,10 @@ case class ArrayInsert(srcArrayExpr: Expression, posExpr: Expression, itemExpr:
override def third: Expression = itemExpr
override def prettyName: String = "array_insert"
- override def dataType: DataType = first.dataType
+ override def dataType: DataType = third.nullable match {
Review Comment:
Sure, I can change it.
--
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] bersprockets commented on a diff in pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "bersprockets (via GitHub)" <gi...@apache.org>.
bersprockets commented on code in PR #39970:
URL: https://github.com/apache/spark/pull/39970#discussion_r1106529574
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -4840,7 +4840,7 @@ case class ArrayInsert(srcArrayExpr: Expression, posExpr: Expression, itemExpr:
override def third: Expression = itemExpr
override def prettyName: String = "array_insert"
- override def dataType: DataType = first.dataType
+ override def dataType: DataType = if (third.nullable) first.dataType.asNullable else first.dataType
Review Comment:
Turns out that `array_insert` can implicitly insert nulls into the array even when the value getting inserted is not null, e.g.:
```
spark-sql> select array_insert(array('1', '2', '3', '4'), -6, '5');
23/02/14 16:10:19 ERROR Executor: Exception in task 0.0 in stage 0.0 (TID 0)
java.lang.NullPointerException
at org.apache.spark.sql.catalyst.expressions.codegen.UnsafeWriter.write(UnsafeWriter.java:110)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.project_doConsume_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.processNext(Unknown Source)
at org.apache.spark.sql.execution.BufferedRowIterator.hasNext(BufferedRowIterator.java:43)
```
My current fix only covers the case where the user is explicitly inserting a null, not when nulls are being implicitly added.
I will follow up.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -5024,7 +5024,7 @@ case class ArrayAppend(left: Expression, right: Expression)
* Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
* the dataType of an unresolved expression (i.e., when `resolved` == false).
*/
- override def dataType: DataType = left.dataType
+ override def dataType: DataType = if (right.nullable) left.dataType.asNullable else left.dataType
Review Comment:
Turns out that `array_insert` can implicitly insert nulls into the array even when the value getting inserted is not null, e.g.:
```
spark-sql> select array_insert(array('1', '2', '3', '4'), -6, '5');
23/02/14 16:10:19 ERROR Executor: Exception in task 0.0 in stage 0.0 (TID 0)
java.lang.NullPointerException
at org.apache.spark.sql.catalyst.expressions.codegen.UnsafeWriter.write(UnsafeWriter.java:110)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.project_doConsume_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.processNext(Unknown Source)
at org.apache.spark.sql.execution.BufferedRowIterator.hasNext(BufferedRowIterator.java:43)
```
My current fix only covers the case where the user is explicitly inserting a null, not when nulls are being implicitly added.
I will follow up.
--
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] bersprockets commented on pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "bersprockets (via GitHub)" <gi...@apache.org>.
bersprockets commented on PR #39970:
URL: https://github.com/apache/spark/pull/39970#issuecomment-1427398680
Re-running failed job ("No golden file found. Please re-run this test with the SPARK_GENERATE_GOLDEN_FILES=1 environment variable set"... which is odd).
--
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] bersprockets commented on a diff in pull request #39970: [SPARK-42401][SQL] Set `containsNull` correctly in the data type for array_insert/array_append
Posted by "bersprockets (via GitHub)" <gi...@apache.org>.
bersprockets commented on code in PR #39970:
URL: https://github.com/apache/spark/pull/39970#discussion_r1106520056
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -5024,7 +5024,7 @@ case class ArrayAppend(left: Expression, right: Expression)
* Returns the [[DataType]] of the result of evaluating this expression. It is invalid to query
* the dataType of an unresolved expression (i.e., when `resolved` == false).
*/
- override def dataType: DataType = left.dataType
+ override def dataType: DataType = if (right.nullable) left.dataType.asNullable else left.dataType
Review Comment:
Turns out that `array_insert` can implicitly insert nulls into the array even when the value getting inserted is not null, e.g.:
```
spark-sql> select array_insert(array('1', '2', '3', '4'), -6, '5');
23/02/14 16:10:19 ERROR Executor: Exception in task 0.0 in stage 0.0 (TID 0)
java.lang.NullPointerException
at org.apache.spark.sql.catalyst.expressions.codegen.UnsafeWriter.write(UnsafeWriter.java:110)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.project_doConsume_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.processNext(Unknown Source)
at org.apache.spark.sql.execution.BufferedRowIterator.hasNext(BufferedRowIterator.java:43)
```
My current fix only covers the case where the user is explicitly inserting a null, not when nulls are being implicitly added.
I will follow up.
--
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