Parquet: Fixes Incorrect Skipping of RowGroups with NaNs #6517

RussellSpitzer · 2023-01-03T19:13:04Z

Previously, the RowGroupFilter would skip any RowGroups whose min or max value was set as NaN because Parquet would report that those row groups "hasNonNull" as false. When the statistics are not empty and hasNonNull is false this actually indicates that the min and max are undefined, not that there are no non nulls.

Fixes #6516

Previously, the RowGroupFilter would skip any RowGroups whose min or max value was set as NaN because Parquet would report that those row groups "hasNonNull" as false. When the statistics are not empty and hasNonNull is false this actually indicates that the min and max are undefined, not that there are no non nulls.

zhongyujiang · 2023-01-04T12:46:13Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetricsRowGroupFilter.java

@@ -580,6 +608,10 @@ static boolean hasNonNullButNoMinMax(Statistics statistics, long valueCount) {
        && (statistics.getMaxBytes() == null || statistics.getMinBytes() == null);
  }

+  static boolean minMaxUndefined(Statistics statistics) {
+    return !statistics.isEmpty() && !statistics.hasNonNullValue();


This unit test fails, I think it's because when all values are all null , min max will be null(undefined) too, seems we should handle the case of all values are null first.

I am thinking we can simplify the handling of unreliable statistics by moving the judgment of all-null situation forward, as I mentioned here，we can use Statistics#getNumNulls() = ColumnChunkMetadata#getValueCount() to determine if all values are null (which will not be affected by null or undinfined min, max statistics); Then we handle the case of statistics.hasNonNullValue() is false(statistics.hasNonNullValue() will return false when statistics has null min, max or unreliable min, max), we should always return ROWS_MIGHT_MATCH when statistics.hasNonNullValue() is false bacause that means statistics is not reliable; Finally we can safely use min, max for comparative evaluation.

I think that sounds good, I was basing this determination off of the code here from Statistics.java in Parquet

@Override public String toString() { if (this.hasNonNullValue()) { if (isNumNullsSet()) { return String.format("min: %s, max: %s, num_nulls: %d", minAsString(), maxAsString(), this.getNumNulls()); } else { return String.format("min: %s, max: %s, num_nulls not defined", minAsString(), maxAsString()); } } else if (!this.isEmpty()) return String.format("num_nulls: %d, min/max not defined", this.getNumNulls()); else return "no stats for this column"; }

Which similarly checks for the numNulls stat first

I simplified this a bit further since I don't think we have to handle the CDH special case in the same way. Now we basically just have a two step check

Are all the values null? This is only true if nullCount is set and it is equal to the value count from the chunk stats.

We check if min/max are defined for the stats
a. For normal parquet stats this means that the stats are not empty && hasNonNull is false
b. For the old CDH version this means the stats are not empty but min and max are set to null. I think the "a" case may also cover this based on my old notes but I figured since this check will be short circuited it doesn't hurt to leave it in.

Reordered checks, we now check for the ALL_NULL case before checking before seeing if min/max values are undefined. Since the behavior of "hasNonNullValues" cannot really be trusted in any circumstance we stop using it entirely. This also removes the workaround for older CDH parquet files which could similarly rewrite metrics with a null value count but undefined min/max statistics.

zhongyujiang

Looks great to me, thanks for fixing this!

szehon-ho · 2023-01-06T03:17:29Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetricsRowGroupFilter.java

+  }
+
+  static boolean minMaxUndefined(Statistics statistics) {
+    return (!statistics.isEmpty() && !statistics.hasNonNullValue()) || nullMinMax(statistics);


So for my understanding its more like other stats are defined but not min/max? Its not strictly what the method name says, so maybe worth a javadoc?

Also I spent some time to explore this:

(!statistics.isEmpty() && !statistics.hasNonNullValue())

I noticed statistics.isEmpty() internally is (!statistics.hasNonNullValue() && !isNumNullsSet), so I think after some math it simplifies down then to :
(stats.isNumNullSet && !stats.hasNonNullValue)

It could be clearer this way, although I guess its safer to use isEmpty if statistitcs.isEmpty definition ever changes.

This is actually a direct reference to the logic in Parquet Statistics.java . I'm doing the same check they do which is "!IsEmpty && !nonNullValue" see

https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/column/statistics/Statistics.java#L454

I added a comment to explain this, but I want to stick to parquet-mr at the moment. Ideally we would have some well-defined api for this but I don't believe we do.

Sounds good to me, I was just doing a sanity check by exploding the logic (!statistics.hasNonNullValue() && !isNumNullsSet) || !statistics.hasNonNullValue() , and it checks out. I think I saw your comment of toString() and was not sure if its that reliable as a source of truth.

I agree with @szehon-ho's simplification of the logic, and I'm debating whether I prefer that or using isEmpty, which is really unclear.

I also think that we may be adding a case to unknown when it is actually known. Since the check becomes isNumNullSet and not hasNonNullValue because NaN values trigger hasNonNullValue = false, are we including cases where hasNonNullValue is actually false because the null count is zero? This would be good to make sure tests cover.

Otherwise I think the logic in this PR looks good.

I'm not sure what you mean by
are we including cases where hasNonNullValue is actually false because the null count is zero?

There are only four assignments of hasNonNullValue

In the Constructor for Statistics : false

& 3. In FloatStats and DoubleStats when a NaN is seen : false

In markAsNonEmpty() : true

/** * Sets the page/column as having a valid non-null value * kind of misnomer here */ protected void markAsNotEmpty() { hasNonNullValue = true; }

This method is invoked in all of the Statistics sub classes when "setMinMax", "initializeStats" or "setMinMaxFromBytes" are called.

So i'm not sure how hasNonNullValue could be actually false unless we see a NaN or no min and max are set

Sounds good. It's odd that this is not based on the number of values and number of nulls. Okay though, I guess it's just a strange API.

It is quite strange

RussellSpitzer · 2023-01-06T20:54:54Z

Retest this please

aokolnychyi

This seems correct to me but it is really unfortunate we inherit this unreliable Parquet logic.

RussellSpitzer · 2023-01-12T22:13:58Z

If no one has any more comments on this PR I'd like to merge it

RussellSpitzer requested review from rdblue and shardulm94 January 3, 2023 19:13

github-actions bot added data parquet labels Jan 3, 2023

RussellSpitzer mentioned this pull request Jan 3, 2023

Improve Test Coverage of RowGroupFilter Code with Nans #6518

Closed

zhongyujiang reviewed Jan 4, 2023

View reviewed changes

RussellSpitzer added 2 commits January 4, 2023 12:38

Spotless Apply

eeeac8b

zhongyujiang approved these changes Jan 5, 2023

View reviewed changes

szehon-ho reviewed Jan 6, 2023

View reviewed changes

Add Comment on undefined

431e76e

Spotless Apply

f26a359

aokolnychyi approved these changes Jan 6, 2023

View reviewed changes

szehon-ho approved these changes Jan 7, 2023

View reviewed changes

Switch from using "isEmpty"

6aa92cd

szehon-ho approved these changes Jan 9, 2023

View reviewed changes

rdblue merged commit e5ea152 into apache:master Jan 13, 2023

RussellSpitzer deleted the ParquetFixNaNRowGroupSkipping branch January 13, 2023 17:59

youngxinler mentioned this pull request Jan 17, 2023

Parquet: Improve Test Coverage of RowGroupFilter Code with Nans #6518 #6554

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parquet: Fixes Incorrect Skipping of RowGroups with NaNs #6517

Parquet: Fixes Incorrect Skipping of RowGroups with NaNs #6517

RussellSpitzer commented Jan 3, 2023 •

edited

Loading

zhongyujiang Jan 4, 2023 •

edited

Loading

zhongyujiang Jan 4, 2023 •

edited

Loading

RussellSpitzer Jan 4, 2023

RussellSpitzer Jan 4, 2023

zhongyujiang left a comment

szehon-ho Jan 6, 2023

RussellSpitzer Jan 6, 2023

RussellSpitzer Jan 6, 2023

szehon-ho Jan 7, 2023

rdblue Jan 7, 2023

RussellSpitzer Jan 9, 2023

rdblue Jan 13, 2023

RussellSpitzer Jan 13, 2023

RussellSpitzer commented Jan 6, 2023

aokolnychyi left a comment

RussellSpitzer commented Jan 12, 2023

Parquet: Fixes Incorrect Skipping of RowGroups with NaNs #6517

Parquet: Fixes Incorrect Skipping of RowGroups with NaNs #6517

Conversation

RussellSpitzer commented Jan 3, 2023 • edited Loading

zhongyujiang Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

zhongyujiang Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongyujiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer commented Jan 6, 2023

aokolnychyi left a comment

Choose a reason for hiding this comment

RussellSpitzer commented Jan 12, 2023

RussellSpitzer commented Jan 3, 2023 •

edited

Loading

zhongyujiang Jan 4, 2023 •

edited

Loading

zhongyujiang Jan 4, 2023 •

edited

Loading