Improved null check for varargs #12673
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12673 +/- ##
============================================
- Coverage 61.75% 61.52% -0.23%
+ Complexity 207 198 -9
============================================
Files 2436 2457 +21
Lines 133233 134417 +1184
Branches 20636 20804 +168
============================================
+ Hits 82274 82701 +427
- Misses 44911 45542 +631
- Partials 6048 6174 +126
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -230,7 +230,7 @@ public static String arrayElementAtString(String[] arr, int idx) { | |||
|
|
|||
| @ScalarFunction(names = {"array", "arrayValueConstructor"}, isVarArg = true) | |||
| public static Object arrayValueConstructor(Object... arr) { | |||
| if (arr.length == 0) { | |||
| if (arr == null || arr.length == 0 || arr[0] == null) { | |||
There was a problem hiding this comment.
Here we are changing the semantic, right? We are treating [null] in the same way we treated [] in the past.
I would feel more confident with something like if (arr == null || arr.length == 0), which is what we may wanted to do in the first place
There was a problem hiding this comment.
Firstly, to give the context:
foo(String... args) {
args == null // foo(null)
args.length == 0 // foo()
args[0] == null // String bar = null; foo(bar)
In this specific case, if arr[0] == null, the code below (arr[0].getClass()) will cause null pointer exception, hence the additional checks is an improvement. I didn't follow why it would change the semantics.
I have tried to only add the additional checks in places which can cause NPE, vs changing all varargs usages in the codebase.
There was a problem hiding this comment.
It changes semantics because foo(null, 1, 2, 3) will behave differently foo(1, 2, 3, null). Do we want to enter on this if when any value in arr is null? Ok, but doing so only when the first value is null looks very strange.
There was a problem hiding this comment.
foo(null, 1, 2, 3) will cause NPE below (arr[0].getClass()), are we ok with that? This PR address that.
if (arr == null || arr.length == 0) check will not prevent the NPE.
There was a problem hiding this comment.
My point is that foo(1, null, 2, 3) will also cause a NPE when doing intArr[i] = (Integer) arr[i];.
It is very strange to do not throw NPE when the first element is null but do it when the second is. We should either always throw NPE when any value in the array is null (like before) or never do that
There was a problem hiding this comment.
Correct. I missed (Integer) arr[i] code, let me modify the PR.
soumitra-st
force-pushed
the
null-check-varargs
branch
from
e236fab
to
b6b5fbf
Compare
| @@ -190,7 +190,7 @@ protected void updateBrokerData() | |||
|
|
|||
| public String getBroker(String... tableNames) { | |||
| List<String> brokers = null; | |||
| if (tableNames != null) { | |||
| if (!(tableNames == null || tableNames.length == 0 || tableNames[0] == null)) { | |||
There was a problem hiding this comment.
Sorry for not saying that before, but I think the same can be said here. Either all or none of the actual elements should be compared to null.
There was a problem hiding this comment.
Pushed a change to filter out nulls if the tableNames is non-null.
soumitra-st
force-pushed
the
null-check-varargs
branch
from
b1ffce5
to
0e835db
Compare
Adding null checks for cases:
cleanup