[FLINK-4609] Remove redundant check for null in CrossOperator #2490
Conversation
| if (input1 == null || input2 == null) { | ||
| throw new NullPointerException(); | ||
| } | ||
|
|
There was a problem hiding this comment.
we could instead move the null check into the super() call using Preconditions.checkNotNull()
There was a problem hiding this comment.
if input1 or/and input2 are null DefaultCross will throw NPE on line 133
I also added null check to TwoInputOperator (grand grand parent of DefaultCross)
There was a problem hiding this comment.
yes, but if you used the preconditions check you could supply a useful error message, for example stating which input was actually null.
There was a problem hiding this comment.
Ok, I added null check for input1 and input2 with a message inline with calling super in DefaultCross
apivovarov
force-pushed
the
FLINK-4609
branch
3 times, most recently
from
85b227a
to
e61ee24
Compare
| @@ -129,14 +129,11 @@ private String getDefaultName() { | |||
|
|
|||
| public DefaultCross(DataSet<I1> input1, DataSet<I2> input2, CrossHint hint, String defaultName) { | |||
|
|
|||
| super(input1, input2, new DefaultCrossFunction<I1, I2>(), | |||
| super(Preconditions.checkNotNull(input1, "input1 is null"), | |||
There was a problem hiding this comment.
Can we do the preconditions check in TwoInputOperator rather than the subclasses?
There was a problem hiding this comment.
DefaultCross calls input1.getType() and input2.getType() before calling super() on line 134. So, if we add null check to super class (e.g. TwoInputOperator) it will not work for DefaultCross
There was a problem hiding this comment.
I also added null check to TwoInputOperator
And removed input1 and input2 fields from DefaultCross because TwoInputOperator already has them
apivovarov
force-pushed
the
FLINK-4609
branch
2 times, most recently
from
47ad988
to
e8e4f72
Compare
| @@ -971,10 +971,7 @@ public void testForwardWithAtLeastOneIterationAssumptionForJavac() { | |||
| public void reduce(Iterable<Tuple2<Long, Long>> values, Collector<Long> out) throws Exception { | |||
| Long id = 0L; | |||
| for (Tuple2<Long, Long> value : values) { | |||
| id = value.f0; | |||
There was a problem hiding this comment.
This change causes a test to fail and should be reverted.
The UdfAnalyzer is a component that inspects the bytecode of user-defined functions to infer how they access input values. By removing the if condition (which is not evaluated by the analyzer), the analyzer can better infer the behavior of the function produces a different output.
There was a problem hiding this comment.
Just reverted this change. Thank you!
|
Thanks for the contribution @apivovarov. The PR looks good except for the modified test which fails. |
apivovarov
force-pushed
the
FLINK-4609
branch
from
e8e4f72
to
09f26b3
Compare
|
Thanks for the fast update @apivovarov. |
|
Playing Devil's advocate here: Does this really improve anything meaningful? This is an API-level operator, not anything runtime related, so the additional null check is not really important. The code is not getting simpler in my opinion as well. It worked well before, and every change may introduce another bug. Why not focus energies on improvements where the system really gains? |
|
This isn't improving performance but moving the null checks before first access and removing the duplicate |
|
+1 to what Greg said. It also gets rid of a compiler warning. |
|
All right, +1 then |
|
Merging ... |
https://issues.apache.org/jira/browse/FLINK-4609