NullPointerException on containsNan()

[RussellSpitzer]

ContainsNan is a primitive boolean in ManifestFileUtil

iceberg/core/src/main/java/org/apache/iceberg/util/ManifestFileUtil.java

Line 45 in 6c6096d

private final boolean containsNaN;

But a boxed Boolean in GenericPartitionFieldSummary

iceberg/core/src/main/java/org/apache/iceberg/GenericPartitionFieldSummary.java

Line 45 in 6c6096d

private Boolean containsNaN = null;

This is also a problem because the interface for partition field summary also declares the return type to be boxed and returns null by default

iceberg/api/src/main/java/org/apache/iceberg/ManifestFile.java

Lines 209 to 211 in 6c6096d

	default Boolean containsNaN() {
	return null;
	}

Which can lead to an NPE if the GPFS = null

        Boolean x = null;
        boolean y = x; // NullPointerException

Caused by: java.lang.NullPointerException
	at org.apache.iceberg.util.ManifestFileUtil$FieldSummary.<init>(ManifestFileUtil.java:54)
	at org.apache.iceberg.util.ManifestFileUtil.summaries(ManifestFileUtil.java:150)
	at org.apache.iceberg.util.ManifestFileUtil.canContainAny(ManifestFileUtil.java:131)
	at org.apache.iceberg.ManifestFilterManager.canContainDeletedFiles(ManifestFilterManager.java:329)
	at org.apache.iceberg.ManifestFilterManager.filterManifest(ManifestFilterManager.java:285)
	at org.apache.iceberg.ManifestFilterManager.lambda$filterManifests$0(ManifestFilterManager.java:182)
	at org.apache.iceberg.util.Tasks$Builder.runTaskWithRetry(Tasks.java:404)
	at org.apache.iceberg.util.Tasks$Builder.access$300(Tasks.java:70)
	at org.apache.iceberg.util.Tasks$Builder$1.run(Tasks.java:310)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	... 3 more

#1872

[RussellSpitzer]

@yyanyy I think i understand this one and I think we just need to force contains NaN to true (i guess i'm not sure what the right value when missing is) if it is missing in the Manifest file read. This is I believe a backwards compatibility issue?

[aokolnychyi]

It looks like @RussellSpitzer's analysis is correct.

[yyanyy]

Sorry for the late response, I think you are right. I think we should default it to true if this value is missing, meanwhile I think we may need to modify the other parts of this class to account for the possibility that min/max could be NaN and thus comparison with the input value produces incorrect outcome. Thanks for looking into it and raising the PR to fix it!

[RussellSpitzer]

@yyanyy NP I put a PR up ^ so you could just check that if you like :) Let me know if we have to handle NAN in min/max but I think that's ok since comparisons with nan are valid

[yyanyy]

@RussellSpitzer Thank you for the quick fix! Regarding handling NaN in min/max, I think you are right that they seem fine today if they are created by iceberg library:

• compare(NaN, xxx) = 1; compare(xxx, NaN) = -1
• when we create them before handling NaN, if input is NaN, min will never be NaN but max will always be NaN
• In ManifestFileUtil now, input as NaN should be intercepted before reaching these comparison evaluation, and we can ignore comparison with lower bound since it won't be NaN. When comparison with upper bound, comparator.compare(typedValue, NaN) > 0 will always be false and thus the method canContain will reach true.

However I guess this is when it's guaranteed that the summary is generated by iceberg library. Since direct comparison with NaN will also be false (e.g. 3D > Double.NaN -> false, Double.NaN > 3D -> false), I think if there are custom implementations for this summary info that uses this approach, the assumption that lowerBound will not be NaN may not hold. and in this case compare(xxx, lowerBound) < 0 will lead to true which lets the canContain return false which may not be correct. In this case I think it might be safer if we add a logic to explicitly check for lowerbound for NaN value. What do you think?

[RussellSpitzer]

I'm a little confused. Are you saying that lowerBound could be NaN? If that's the case then that should mean that no values are less than NaN, which is only true if all values are NaN. Since NaN > all values. If NaN is set as the lower bound and there are values which are non NaN then the lower bound has been set incorrectly I think,

[yyanyy]

Sorry let me make it more clear: I think it could happen if people have their custom implementation for summary info, and I'm not sure if it would be a bug on their side if they assign the lower bound in some way similar to the following:

if (isGreaterThanBound(max, val)) {
  max = val;
}

if (!isGreaterThanBound(min, val)) { <--
  min = val;
}
...
private boolean isGreaterThanBound(int bound, int val) {
  return val > bound;
}

I guess the code seems unnatural, but an additional check on lower bound may not be that expensive to account for this?

[RussellSpitzer]

Fair enough, i'll add that in tonight! Thanks for reviewing.

[RussellSpitzer]

Actually i'm not sure how we safely check this, are we saying that if the lower bound is NaN then we can't trust the lower bound check? Seems like we are doing unnecessary reads of files that are all NaN then?

[yyanyy]

Yeah I was thinking to include the file if lower bound is NaN, and you are right we will do unnecessary reads for files only contain NaN for the given column. Do you think it's unlikely/unnecessary?

[RussellSpitzer]

Do you think if lower and upper bounds are NaN, then we are probably safe right? We could just add in a bail out if (lowerBounds == upperBounds && value < lowerBounds ) return False ?

[yyanyy]

Sorry to make sure I understand, are you suggesting if both upper/lower bounds are NaN, the column will only contain NaN so that we can return false? I think it's possible that a column may contain a mix of NaN and normal values but still report min/max as both NaN, which is what ORC files metric collection does today: https://github.com/apache/iceberg/blob/master/core/src/test/java/org/apache/iceberg/TestMetrics.java#L373-L374

But after thinking about it for a bit, I think it may not worth adding this logic to avoid including unnecessary files, as 1) the chance might be low if people to do their own implementation on it since implementing manifest list may be non-trivial, 2) it's not a new thing we introduced, and 3) we start to mention in spec that upper/lower shouldn't include NaN: https://iceberg.apache.org/spec/#snapshots:

510 lower_bound
...
Lower bound for the non-null, non-NaN values in the partition field, or null if all values are null or NaN [2]

[RussellSpitzer]

That makes me more cautious about adding in the check anyway, I'm not sure we want defensive coding against things that are outside of spec. If the lower_bound is NaN then isn't the file invalid compared to the spec?

[yyanyy]

You are right, the only caveat is that I think this requirement of not including NaN is only introduced not long ago. But yeah, I think we probably shouldn't alter the current behavior.