From 22dc94f6c753a81d9362a937823a541fa037f67b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Rou=C3=A9l?= <andre.rouel@gmail.com>
Date: Fri, 30 Oct 2026 15:05:23 +0100
Subject: [PATCH] GH-3913: Avoid Comparing Equal Binary Statistics Values

BinaryStatistics.updateStats(Binary) compares each value with the current minimum and maximum. When the value is the same instance as one of these bounds, the result of the comparison is always equal. This occurs when a writer sends the same constant Binary instance again, for example values decoded from a dictionary.

Update BinaryStatistics in the parquet-column module to skip the comparisons when the value is the same instance as the minimum or the maximum. The resulting statistics do not change.

Add tests in TestStatistics that make sure the behavior does not change: a single constant instance that repeats, bound instances that repeat, and other instances with equal, lower, or higher values. Unit tests cannot detect the skipped comparisons, because the result is the same with or without them.
---
 .../column/statistics/BinaryStatistics.java   | 17 ++++---
 .../column/statistics/TestStatistics.java     | 46 +++++++++++++++++++
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java b/parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java
index b171106804..e31c170ecc 100644
--- a/parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java
+++ b/parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java
@@ -57,12 +57,17 @@ public void updateStats(Binary value) {
       min = value.copy();
       max = value.copy();
       this.markAsNotEmpty();
-    } else {
-      if (comparator().compare(min, value) > 0) {
-        min = value.copy();
-      } else if (comparator().compare(max, value) < 0) {
-        max = value.copy();
-      }
+      return;
+    }
+    // copy() keeps the same instance when the backing bytes are not reused.
+    // Thus, a value can be the same instance as min or max. That value is equal to the bound, so do not compare it.
+    if (value == min || value == max) {
+      return;
+    }
+    if (comparator().compare(min, value) > 0) {
+      min = value.copy();
+    } else if (comparator().compare(max, value) < 0) {
+      max = value.copy();
     }
   }
 
diff --git a/parquet-column/src/test/java/org/apache/parquet/column/statistics/TestStatistics.java b/parquet-column/src/test/java/org/apache/parquet/column/statistics/TestStatistics.java
index e4783b700e..83d59512db 100644
--- a/parquet-column/src/test/java/org/apache/parquet/column/statistics/TestStatistics.java
+++ b/parquet-column/src/test/java/org/apache/parquet/column/statistics/TestStatistics.java
@@ -521,6 +521,52 @@ public void testBinaryMinMaxForReusedBackingByteArray() {
     assertThat(stats.getMinBytes()).isEqualTo(new byte[] {10});
   }
 
+  @Test
+  public void testBinaryMinMaxForRepeatedSingleConstantInstance() {
+    BinaryStatistics stats = new BinaryStatistics();
+
+    Binary value = Binary.fromConstantByteArray(new byte[] {7});
+    stats.updateStats(value);
+    stats.updateStats(value);
+    stats.updateStats(value);
+
+    assertThat(stats.genericGetMin()).isSameAs(value);
+    assertThat(stats.genericGetMax()).isSameAs(value);
+  }
+
+  @Test
+  public void testBinaryMinMaxKeepsBoundInstancesForConstantValues() {
+    BinaryStatistics stats = new BinaryStatistics();
+
+    Binary low = Binary.fromConstantByteArray(new byte[] {10});
+    Binary high = Binary.fromConstantByteArray(new byte[] {30});
+    stats.updateStats(low);
+    stats.updateStats(high);
+    // The bounds keep the constant instances. An update with a bound instance does not change the bounds.
+    assertThat(stats.genericGetMin()).isSameAs(low);
+    assertThat(stats.genericGetMax()).isSameAs(high);
+    stats.updateStats(low);
+    assertThat(stats.genericGetMin()).isSameAs(low);
+    assertThat(stats.genericGetMax()).isSameAs(high);
+    stats.updateStats(high);
+    assertThat(stats.genericGetMin()).isSameAs(low);
+    assertThat(stats.genericGetMax()).isSameAs(high);
+    stats.updateStats(Binary.fromConstantByteArray(new byte[] {20}));
+    assertThat(stats.genericGetMin()).isSameAs(low);
+    assertThat(stats.genericGetMax()).isSameAs(high);
+
+    // A different instance with equal content does not change the bounds. Lower or higher values change them.
+    stats.updateStats(Binary.fromConstantByteArray(new byte[] {10}));
+    assertThat(stats.genericGetMin()).isSameAs(low);
+    assertThat(stats.genericGetMax()).isSameAs(high);
+    stats.updateStats(Binary.fromConstantByteArray(new byte[] {5}));
+    assertThat(stats.getMinBytes()).isEqualTo(new byte[] {5});
+    assertThat(stats.genericGetMax()).isSameAs(high);
+    stats.updateStats(Binary.fromConstantByteArray(new byte[] {40}));
+    assertThat(stats.getMinBytes()).isEqualTo(new byte[] {5});
+    assertThat(stats.getMaxBytes()).isEqualTo(new byte[] {40});
+  }
+
   @Test
   public void testMergingStatistics() {
     testMergingIntStats();