Core: Fix NPE caused by Unboxing a Null in ManifestFileUtil (#2492) #2495
Conversation
yyanyy
left a comment
yyanyy
left a comment
There was a problem hiding this comment.
Thanks for raising the PR!
| // if lower bound is null, then there are no non-null values | ||
| if (lowerBound == null) { | ||
| // the value is non-null, so it cannot match | ||
| return false; |
There was a problem hiding this comment.
I think if the input is NaN and the file is written with the new code (NaN flips containsNaN to true without touching upper/lower bound) then I think we will return false here which wouldn't be correct?
There was a problem hiding this comment.
I think @yyanyy is correct. It was my bad for suggesting it.
There was a problem hiding this comment.
Ok I think i understand the issue here,
Previously we would report "null" for the lowerBound when we meant "NaN" because we weren't recording NaNs, this means if we see a missing lowerBound we either could have a mix of NaN and Null
Previously the boxed value of contains NaN would be null if the table was made before NaN stats were implemented. This leads to an NPE when being unboxed. To fix this we use the Boxed boolean and on null report "true" since we do not know if the partition contains any NaN values.
RussellSpitzer
force-pushed
the
FixUnboxingNPEManifestUtil
branch
from
3d4c691 to
5c1e019
Compare
|
|
||
| if (NaNUtil.isNaN(value)) { | ||
| return containsNaN; | ||
| return containsNaN == null ? true : containsNaN; |
There was a problem hiding this comment.
I'd consider doing this in the constructor to avoid calling the ternary operation in each canContain.
this.containsNaN = summary.containsNaN() == null || summary.containsNaN();
Then this will also be a one line change. I'll leave this up to you, @RussellSpitzer @yyanyy.
There was a problem hiding this comment.
I'd rather keep it explicit
|
LGTM. I'd probably do the check once in the constructor but I am not sure it is a big deal. |
) (apache#2495) * Core: Fix NPE caused by Unboxing a Null in ManifestFileUtil Previously the boxed value of containsNaN would be null if the table was made before NaN stats were implemented. This leads to an NPE when being unboxed. To fix this on null we report "true" since we do not know if the partition contains any NaN values.
3 participants
Previously the boxed value of contains NaN would be null if the table
was made before NaN stats were implemented. This leads to an NPE when
being unboxed. To fix this we use the Boxed boolean and on null report
"true" since we do not know if the partition contains any NaN values. In
addition we move this below the "allValuesNull" check just in case that
would have allowed us to ignore the manifest since the values are all null.
Fixes (#2492 )