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 2022/09/12 00:51:51 UTC
[GitHub] [spark] bersprockets opened a new pull request, #37852: [SPARK-40403][SQL] Calculate unsafe array size using longs to avoid negative size in error message
bersprockets opened a new pull request, #37852:
URL: https://github.com/apache/spark/pull/37852
### What changes were proposed in this pull request?
Change `UnsafeArrayWriter#initialize` to use longs rather than ints when calculating the initial size of the array.
### Why are the changes needed?
When calculating the initial size in bytes needed for the array, `UnsafeArrayWriter#initialize` uses an int expression, which can overflow. The initialize method then passes the negative size to `BufferHolder#grow`, which complains about the negative size.
Example (the following will run just fine on a 16GB laptop, despite the large driver size setting):
```
bin/spark-sql --driver-memory 22g --master "local[1]"
create or replace temp view data1 as
select 0 as key, id as val
from range(0, 268271216);
create or replace temp view data2 as
select key as lkey, collect_list(val) as bigarray
from data1
group by key;
-- the below cache forces Spark to create unsafe rows
cache lazy table data2;
select count(*) from data2;
```
After a few minutes, `BufferHolder#grow` will throw the following exception:
```
java.lang.IllegalArgumentException: Cannot grow BufferHolder by size -2115263656 because the size is negative
at org.apache.spark.sql.catalyst.expressions.codegen.BufferHolder.grow(BufferHolder.java:67)
at org.apache.spark.sql.catalyst.expressions.codegen.UnsafeArrayWriter.initialize(UnsafeArrayWriter.java:61)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$SpecificUnsafeProjection.apply(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.aggregate.Collect.serialize(collect.scala:73)
at org.apache.spark.sql.catalyst.expressions.aggregate.Collect.serialize(collect.scala:37)
```
This query was going to fail anyway, but the message makes it looks like a bug in Spark rather than a user problem. `UnsafeArrayWriter#initialize` should calculate using a long expression and fail if the size exceeds `Integer.MAX_VALUE`, showing the actual initial size in the error message.
Note: This issue is not related to SPARK-39608, as far as I can tell, despite having the same symptom
### Does this PR introduce _any_ user-facing change?
Other than a better error message, no.
### How was this patch tested?
New unit test.
--
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 #37852: [SPARK-40403][SQL] Calculate unsafe array size using longs to avoid negative size in error message
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #37852: [SPARK-40403][SQL] Calculate unsafe array size using longs to avoid negative size in error message
URL: https://github.com/apache/spark/pull/37852
--
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 a diff in pull request #37852: [SPARK-40403][SQL] Calculate unsafe array size using longs to avoid negative size in error message
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #37852:
URL: https://github.com/apache/spark/pull/37852#discussion_r967979263
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/UnsafeArrayWriter.java:
##########
@@ -55,10 +55,19 @@ public void initialize(int numElements) {
this.startingOffset = cursor();
+ long fixedPartInBytesLong =
+ ByteArrayMethods.roundNumberOfBytesToNearestWord((long) elementSize * numElements);
+ long totalInitialSize = headerInBytes + fixedPartInBytesLong;
+
+ if (totalInitialSize > Integer.MAX_VALUE) {
+ throw new IllegalArgumentException(
Review Comment:
Is it possible to trigger the error from user space (from SQL for instance). If so, please, introduce an error class and place it to error-classes.json.
--
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 #37852: [SPARK-40403][SQL] Calculate unsafe array size using longs to avoid negative size in error message
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #37852:
URL: https://github.com/apache/spark/pull/37852#issuecomment-1246162583
Merged to master.
--
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 #37852: [SPARK-40403][SQL] Calculate unsafe array size using longs to avoid negative size in error message
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #37852:
URL: https://github.com/apache/spark/pull/37852#issuecomment-1244884508
cc @cloud-fan FYI
--
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] cloud-fan commented on a diff in pull request #37852: [SPARK-40403][SQL] Calculate unsafe array size using longs to avoid negative size in error message
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37852:
URL: https://github.com/apache/spark/pull/37852#discussion_r969142012
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/UnsafeArrayWriter.java:
##########
@@ -55,10 +55,19 @@ public void initialize(int numElements) {
this.startingOffset = cursor();
+ long fixedPartInBytesLong =
+ ByteArrayMethods.roundNumberOfBytesToNearestWord((long) elementSize * numElements);
+ long totalInitialSize = headerInBytes + fixedPartInBytesLong;
+
+ if (totalInitialSize > Integer.MAX_VALUE) {
+ throw new IllegalArgumentException(
Review Comment:
I think so, let's make the error more user-facing.
error class: TOO_MANY_ARRAY_ELEMENTS
message: "Cannot initialize array with %numElements elements"
--
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 #37852: [SPARK-40403][SQL] Calculate unsafe array size using longs to avoid negative size in error message
Posted by GitBox <gi...@apache.org>.
bersprockets commented on code in PR #37852:
URL: https://github.com/apache/spark/pull/37852#discussion_r970067289
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/UnsafeArrayWriter.java:
##########
@@ -55,10 +55,19 @@ public void initialize(int numElements) {
this.startingOffset = cursor();
+ long fixedPartInBytesLong =
+ ByteArrayMethods.roundNumberOfBytesToNearestWord((long) elementSize * numElements);
+ long totalInitialSize = headerInBytes + fixedPartInBytesLong;
+
+ if (totalInitialSize > Integer.MAX_VALUE) {
+ throw new IllegalArgumentException(
Review Comment:
Just a point of clarification. The issue isn't too many elements per se. It's too many elements for the given element size.
If, in the above example, I had cast `val` as int, the `collect_list` would have succeeded (i.e., 268271216 int elements is just fine, but the same number of bigint elements is not).
How about this for a message?:
"Cannot initialize array with %numElements elements of size %size"
Caveat: my suggested message will be a little confusing for arrays of non-primitives, where the element size is always 8 (for the size/offset).
--
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