PARQUET-136: NPE thrown in StatisticsFilter when all values in a string/binary column trunk are null

In case of all nulls in a binary column, statistics object read from file metadata is empty, and should return true for all nulls check for the column. Even if column has no values, it can be ignored.

The other way is to fix this behaviour in the writer, but is that what we want ?

Author: Yash Datta <Yash.Datta@guavus.com>
Author: Alex Levenson <alexlevenson@twitter.com>
Author: Yash Datta <saucam@gmail.com>

Closes apache#99 from saucam/npe and squashes the following commits:

5138e44 [Yash Datta] PARQUET-136: Remove unreachable block
b17cd38 [Yash Datta] Revert "PARQUET-161: Trigger tests"
82209e6 [Yash Datta] PARQUET-161: Trigger tests
aab2f81 [Yash Datta] PARQUET-161: Review comments for the test case
2217ee2 [Yash Datta] PARQUET-161: Add a test case for checking the correct statistics info is recorded in case of all nulls in a column
c2f8d6f [Yash Datta] PARQUET-161: Fix the write path to write statistics object in case of only nulls in the column
97bb517 [Yash Datta] Revert "revert TestStatisticsFilter.java"
a06f0d0 [Yash Datta] Merge pull request apache#1 from isnotinvain/alexlevenson/PARQUET-161-136
b1001eb [Alex Levenson] Fix statistics isEmpty, handle more edge cases in statistics filter
0c88be0 [Alex Levenson] revert TestStatisticsFilter.java
1ac9192 [Yash Datta] PARQUET-136: Its better to not filter chunks for which empty statistics object is returned. Empty statistics can be read in case of 1. pre-statistics files, 2. files written from current writer that has a bug, as it does not write the statistics if column has all nulls
e5e924e [Yash Datta] Revert "PARQUET-136: In case of all nulls in a binary column, statistics object read from file metadata is empty, and should return true for all nulls check for the column"
8cc5106 [Yash Datta] Revert "PARQUET-136: fix hasNulls to cater to the case where all values are nulls"
c7c126f [Yash Datta] PARQUET-136: fix hasNulls to cater to the case where all values are nulls
974a22b [Yash Datta] PARQUET-136: In case of all nulls in a binary column, statistics object read from file metadata is empty, and should return true for all nulls check for the column

Author: Yash Datta

parquet-column/src/main/java/parquet/column/statistics/BinaryStatistics.java

@@ -24,7 +24,7 @@ public class BinaryStatistics extends Statistics<Binary> {
 
   @Override
   public void updateStats(Binary value) {
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(value, value);
     } else {
       updateStats(value, value);
@@ -34,7 +34,7 @@ public void updateStats(Binary value) {
   @Override
   public void mergeStatisticsMinMax(Statistics stats) {
     BinaryStatistics binaryStats = (BinaryStatistics)stats;
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(binaryStats.getMin(), binaryStats.getMax());
     } else {
       updateStats(binaryStats.getMin(), binaryStats.getMax());
@@ -60,9 +60,11 @@ public byte[] getMinBytes() {
 
   @Override
   public String toString() {
-    if(!this.isEmpty())
+    if (this.hasNonNullValue())
       return String.format("min: %s, max: %s, num_nulls: %d", min.toStringUsingUTF8(), max.toStringUsingUTF8(), this.getNumNulls());
-    else
+   else if (!this.isEmpty())
+      return String.format("num_nulls: %d, min/max not defined", this.getNumNulls());
+   else
       return "no stats for this column";
   }
 
@@ -100,4 +102,4 @@ public void setMinMax(Binary min, Binary max) {
     this.min = min;
     this.markAsNotEmpty();
   }
-}
+}

parquet-column/src/main/java/parquet/column/statistics/BooleanStatistics.java

@@ -24,7 +24,7 @@ public class BooleanStatistics extends Statistics<Boolean> {
 
   @Override
   public void updateStats(boolean value) {
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(value, value);
     } else {
       updateStats(value, value);
@@ -34,7 +34,7 @@ public void updateStats(boolean value) {
   @Override
   public void mergeStatisticsMinMax(Statistics stats) {
     BooleanStatistics boolStats = (BooleanStatistics)stats;
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(boolStats.getMin(), boolStats.getMax());
     } else {
       updateStats(boolStats.getMin(), boolStats.getMax());
@@ -60,9 +60,11 @@ public byte[] getMinBytes() {
 
   @Override
   public String toString() {
-    if(!this.isEmpty())
+    if (this.hasNonNullValue())
       return String.format("min: %b, max: %b, num_nulls: %d", min, max, this.getNumNulls());
-    else
+    else if(!this.isEmpty())
+      return String.format("num_nulls: %d, min/max not defined", this.getNumNulls());
+    else  
       return "no stats for this column";
   }
 

parquet-column/src/main/java/parquet/column/statistics/DoubleStatistics.java

@@ -24,7 +24,7 @@ public class DoubleStatistics extends Statistics<Double> {
 
   @Override
   public void updateStats(double value) {
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(value, value);
     } else {
       updateStats(value, value);
@@ -34,7 +34,7 @@ public void updateStats(double value) {
   @Override
   public void mergeStatisticsMinMax(Statistics stats) {
     DoubleStatistics doubleStats = (DoubleStatistics)stats;
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(doubleStats.getMin(), doubleStats.getMax());
     } else {
       updateStats(doubleStats.getMin(), doubleStats.getMax());
@@ -60,8 +60,10 @@ public byte[] getMinBytes() {
 
   @Override
   public String toString() {
-    if(!this.isEmpty())
+    if(this.hasNonNullValue())
       return String.format("min: %.5f, max: %.5f, num_nulls: %d", min, max, this.getNumNulls());
+    else if (!this.isEmpty())
+      return String.format("num_nulls: %d, min/max not defined", this.getNumNulls());
     else
       return "no stats for this column";
   }
@@ -100,4 +102,4 @@ public void setMinMax(double min, double max) {
     this.min = min;
     this.markAsNotEmpty();
   }
-}
+}

parquet-column/src/main/java/parquet/column/statistics/FloatStatistics.java

@@ -24,7 +24,7 @@ public class FloatStatistics extends Statistics<Float> {
 
   @Override
   public void updateStats(float value) {
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(value, value);
     } else {
       updateStats(value, value);
@@ -34,7 +34,7 @@ public void updateStats(float value) {
   @Override
   public void mergeStatisticsMinMax(Statistics stats) {
     FloatStatistics floatStats = (FloatStatistics)stats;
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(floatStats.getMin(), floatStats.getMax());
     } else {
       updateStats(floatStats.getMin(), floatStats.getMax());
@@ -60,8 +60,10 @@ public byte[] getMinBytes() {
 
   @Override
   public String toString() {
-    if(!this.isEmpty())
+    if (this.hasNonNullValue())
       return String.format("min: %.5f, max: %.5f, num_nulls: %d", min, max, this.getNumNulls());
+    else if (!this.isEmpty())
+      return String.format("num_nulls: %d, min/max not defined", this.getNumNulls());
     else
       return "no stats for this column";
   }

parquet-column/src/main/java/parquet/column/statistics/IntStatistics.java

@@ -24,7 +24,7 @@ public class IntStatistics extends Statistics<Integer> {
 
   @Override
   public void updateStats(int value) {
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(value, value);
     } else {
       updateStats(value, value);
@@ -34,7 +34,7 @@ public void updateStats(int value) {
   @Override
   public void mergeStatisticsMinMax(Statistics stats) {
     IntStatistics intStats = (IntStatistics)stats;
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(intStats.getMin(), intStats.getMax());
     } else {
       updateStats(intStats.getMin(), intStats.getMax());
@@ -60,8 +60,10 @@ public byte[] getMinBytes() {
 
   @Override
   public String toString() {
-    if(!this.isEmpty())
+    if (this.hasNonNullValue())
       return String.format("min: %d, max: %d, num_nulls: %d", min, max, this.getNumNulls());
+    else if (!this.isEmpty())
+      return String.format("num_nulls: %d, min/max is not defined", this.getNumNulls());
     else
       return "no stats for this column";
   }

parquet-column/src/main/java/parquet/column/statistics/LongStatistics.java

@@ -24,7 +24,7 @@ public class LongStatistics extends Statistics<Long> {
 
   @Override
   public void updateStats(long value) {
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(value, value);
     } else {
       updateStats(value, value);
@@ -34,7 +34,7 @@ public void updateStats(long value) {
   @Override
   public void mergeStatisticsMinMax(Statistics stats) {
     LongStatistics longStats = (LongStatistics)stats;
-    if (this.isEmpty()) {
+    if (!this.hasNonNullValue()) {
       initializeStats(longStats.getMin(), longStats.getMax());
     } else {
       updateStats(longStats.getMin(), longStats.getMax());
@@ -60,8 +60,10 @@ public byte[] getMinBytes() {
 
   @Override
   public String toString() {
-    if(!this.isEmpty())
+    if (this.hasNonNullValue())
       return String.format("min: %d, max: %d, num_nulls: %d", min, max, this.getNumNulls());
+    else if (!this.isEmpty())
+      return String.format("num_nulls: %d, min/max not defined", this.getNumNulls());
     else
       return "no stats for this column";
   }
@@ -100,4 +102,4 @@ public void setMinMax(long min, long max) {
     this.min = min;
     this.markAsNotEmpty();
   }
-}
+}

parquet-column/src/main/java/parquet/column/statistics/Statistics.java

@@ -28,11 +28,11 @@
  */
 public abstract class Statistics<T extends Comparable<T>> {
 
-  private boolean firstValueAccountedFor;
+  private boolean hasNonNullValue;
   private long num_nulls;
 
   public Statistics() {
-    firstValueAccountedFor = false;
+    hasNonNullValue = false;
     num_nulls = 0;
   }
 
@@ -142,7 +142,10 @@ public void mergeStatistics(Statistics stats) {
 
     if (this.getClass() == stats.getClass()) {
       incrementNumNulls(stats.getNumNulls());
-      mergeStatisticsMinMax(stats);
+      if (stats.hasNonNullValue()) {
+        mergeStatisticsMinMax(stats);
+        markAsNotEmpty();
+      }
     } else {
       throw new StatisticsClassException(this.getClass().toString(), stats.getClass().toString());
     }
@@ -220,11 +223,22 @@ public void setNumNulls(long nulls) {
    * @return true if object is empty, false otherwise
    */
   public boolean isEmpty() {
-    return !firstValueAccountedFor;
+    return !hasNonNullValue && num_nulls == 0;
   }
 
+  /**
+   * Returns whether there have been non-null values added to this statistics
+   */
+  public boolean hasNonNullValue() {
+    return hasNonNullValue;
+  }
+ 
+  /**
+   * Sets the page/column as having a valid non-null value
+   * kind of misnomer here
+   */ 
   protected void markAsNotEmpty() {
-    firstValueAccountedFor = true;
+    hasNonNullValue = true;
   }
 }