-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
HADOOP-18471. Fixed ArrayIndexOutOfBoundsException in class DefaultStringifier #4957
Conversation
🎊 +1 overall
This message was automatically generated. |
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 @FuzzingTeam for your report.
@throws IndexOutOfBoundsException if the items array is empty
Maybe throw IndexOutOfBoundsException
as expected.
🎊 +1 overall
This message was automatically generated. |
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.
commented. i'd expect the original code to raise the same exception on the [0] index reference; the new test run against the old code should verify that.
regarding the new test, use our `intercept()' method, e.g.
intercept(IndexOutOfBoundsException.class, () ->
DefaultStringifier.storeArray(conf, new Integer[] {}, keyName));
@@ -107,6 +108,12 @@ public void testStoreLoadArray() throws IOException { | |||
Integer[] array = new Integer[] {1,2,3,4,5}; | |||
|
|||
|
|||
try { |
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.
use our LambdaTestUtils.intercept()
here; it's a reimplementation of scalatest's and straightforward to use it.
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.
we have updated the code, please review.
@@ -158,6 +158,9 @@ public static <K> K load(Configuration conf, String keyName, | |||
public static <K> void storeArray(Configuration conf, K[] items, | |||
String keyName) throws IOException { | |||
|
|||
if (items.length == 0) { | |||
throw new IndexOutOfBoundsException(); | |||
} | |||
DefaultStringifier<K> stringifier = new DefaultStringifier<K>(conf, | |||
GenericsUtil.getClass(items[0])); |
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.
won't this trigger the same exception automatically? what does the test do before the new lines are added
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 @steveloughran for the review.
Yes, the original code would trigger ArrayIndexOutOfBoundsException automatically, and the new test passes without the new lines.
But we found the same exception has been handled in other places in the same module and hence, propose to handle it.
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.
ok
🎊 +1 overall
This message was automatically generated. |
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.
+1
@@ -158,6 +158,9 @@ public static <K> K load(Configuration conf, String keyName, | |||
public static <K> void storeArray(Configuration conf, K[] items, | |||
String keyName) throws IOException { | |||
|
|||
if (items.length == 0) { | |||
throw new IndexOutOfBoundsException(); | |||
} | |||
DefaultStringifier<K> stringifier = new DefaultStringifier<K>(conf, | |||
GenericsUtil.getClass(items[0])); |
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.
ok
…ier (#4957) Contributed by FuzzingTeam
thanks, in trunk and branch-3.3 |
…ier (apache#4957) Contributed by FuzzingTeam
Description of PR
JIRA: HADOOP-18471
How was this patch tested?
Modified an existing unit test
testStoreLoadArray
of test classTestDefaultStringifier
to verify.For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?