PARQUET-1338: Fix PrimitiveType.equals throw NPE #498
Conversation
| @Override | ||
| protected boolean equals(Type other) { | ||
| if (!other.isPrimitive()) { | ||
| if (other == null || !other.isPrimitive()) { |
There was a problem hiding this comment.
How come the other is null? Adding a null check sounds reasonable for me, though I wonder in what situation does one call equals with a null parameter.
There was a problem hiding this comment.
It's part of the method contract: https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#equals(java.lang.Object)
This basically eliminates the need to add two checks when you want to check equality and the other object may be null. There are lots of cases where equals is used and the code doesn't need to handle null checks in addition.
|
|
||
| @Override | ||
| public boolean equals(Object other) { | ||
| if (!(other instanceof Type) || other == null) { |
There was a problem hiding this comment.
Why is this required? The test you wrote fails when I don't apply the change on PrimitiveType, but doesn't fail if I don't apply this.
There was a problem hiding this comment.
Actually I think null check is not needed here at all: https://docs.oracle.com/javase/specs/jls/se10/html/jls-15.html#jls-15.20.2
|
It seems that my usage is wrong. |
|
@wangyum, the PrimitiveType's equals method still seems wrong. I think the Type implementation that uses instanceof is fine (because null instanceof Anything is always false) but PrimitiveType is definitely wrong. |
|
Thanks @rdblue. I reopened this. |
|
It seems to me that this equals method is called from the public equals method of Type, where the null check is already present. Since null check is already done before (the 'real' equals), what's the advantage of doing it here again? |
| import static org.apache.parquet.schema.Type.Repetition.*; | ||
|
|
||
| public class TestTypeBuilders { | ||
|
|
There was a problem hiding this comment.
Can you remove this? It is a non-functional change.
|
@nandorKollar, any |
|
@rdblue yes, and a null check is absolutely fine for me! I just wanted to point out, that the method what we just modify is actually not the overridden version of Object#equals (it is protected, not public, therefore we have a method with more restricted scope which happen to have the same name as Object#equals), and Type already has an equals method, which already does null check before casting to Type and calling the protected method. Anyway, I'm fine with this. One minor thing: if we add the null check here, should we consider adding it to GroupType#equals too? |
3 participants
Fix
PrimitiveType.equalsthrow NPE.