[GitHub] [iceberg] yyanyy commented on a change in pull request #2495: Core: Fix NPE caused by Unboxing a Null in ManifestFileUtil (#2492)

[GitBox <gi...@apache.org>]

yyanyy commented on a change in pull request #2495:
URL: https://github.com/apache/iceberg/pull/2495#discussion_r616182133



##########
File path: core/src/main/java/org/apache/iceberg/util/ManifestFileUtil.java
##########
@@ -59,16 +59,16 @@ boolean canContain(Object value) {
         return containsNull;
       }
 
-      if (NaNUtil.isNaN(value)) {
-        return containsNaN;
-      }
-
       // 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;

Review 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? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org