-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-35041][SQL] Revise the overflow in UTF8String #32142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
MaxGekk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
byte[] newBytes = new byte[numBytes * times]; - I would add an assert here to be sure that size doesn't overflow
spark/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java
Lines 886 to 887 in 1be1012
int size = numBytes - delim.numBytes - idx; byte[] bytes = new byte[size]; - Replace Ints.checkedCast at
final byte[] result = new byte[Ints.checkedCast(totalLength)];
The second place seems will never overflow. Any reason to assert not overflow here ? |
ok. It seems we can guarantee that |
| int resultSize = Math.toIntExact((long)numBytes * times); | ||
| byte[] newBytes = new byte[resultSize]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| int resultSize = Math.toIntExact((long)numBytes * times); | |
| byte[] newBytes = new byte[resultSize]; | |
| byte[] newBytes = new byte[Math.multiplyExact(numBytes, times)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the carefully review !
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #137274 has finished for PR 32142 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #137283 has finished for PR 32142 at commit
|
|
+1, LGTM. Merging to master. |
|
thanks for merging |
What changes were proposed in this pull request?
Add overflow check before do
new byte[].Why are the changes needed?
Avoid overflow in extreme case.
Does this PR introduce any user-facing change?
Maybe yes, the error msg changed if overflow.
How was this patch tested?
Pass CI.