[iceberg] branch master updated: Core: Make all of the BaseFile stat maps immutable (#7643)

[pv...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

pvary pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/iceberg.git


The following commit(s) were added to refs/heads/master by this push:
     new b69e84b6cf Core: Make all of the BaseFile stat maps immutable (#7643)
b69e84b6cf is described below

commit b69e84b6cf93a330adb31bf4a46d09c98202873d
Author: pvary <pe...@gmail.com>
AuthorDate: Fri May 26 11:24:46 2023 +0200

    Core: Make all of the BaseFile stat maps immutable (#7643)
---
 .../src/main/java/org/apache/iceberg/BaseFile.java | 21 +++++++++---
 .../apache/iceberg/SerializableByteBufferMap.java  | 14 ++++++++
 .../apache/iceberg/TestManifestReaderStats.java    | 37 ++++++++++++++++++++++
 3 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/core/src/main/java/org/apache/iceberg/BaseFile.java b/core/src/main/java/org/apache/iceberg/BaseFile.java
index 3ecf68f9ac..72f7579995 100644
--- a/core/src/main/java/org/apache/iceberg/BaseFile.java
+++ b/core/src/main/java/org/apache/iceberg/BaseFile.java
@@ -21,6 +21,7 @@ package org.apache.iceberg;
 import java.io.Serializable;
 import java.nio.ByteBuffer;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -444,12 +445,12 @@ abstract class BaseFile<F>
 
   @Override
   public Map<Integer, ByteBuffer> lowerBounds() {
-    return toReadableMap(lowerBounds);
+    return toReadableByteBufferMap(lowerBounds);
   }
 
   @Override
   public Map<Integer, ByteBuffer> upperBounds() {
-    return toReadableMap(upperBounds);
+    return toReadableByteBufferMap(upperBounds);
   }
 
   @Override
@@ -473,10 +474,22 @@ abstract class BaseFile<F>
   }
 
   private static <K, V> Map<K, V> toReadableMap(Map<K, V> map) {
-    if (map instanceof SerializableMap) {
+    if (map == null) {
+      return null;
+    } else if (map instanceof SerializableMap) {
       return ((SerializableMap<K, V>) map).immutableMap();
     } else {
-      return map;
+      return Collections.unmodifiableMap(map);
+    }
+  }
+
+  private static Map<Integer, ByteBuffer> toReadableByteBufferMap(Map<Integer, ByteBuffer> map) {
+    if (map == null) {
+      return null;
+    } else if (map instanceof SerializableByteBufferMap) {
+      return ((SerializableByteBufferMap) map).immutableMap();
+    } else {
+      return Collections.unmodifiableMap(map);
     }
   }
 
diff --git a/core/src/main/java/org/apache/iceberg/SerializableByteBufferMap.java b/core/src/main/java/org/apache/iceberg/SerializableByteBufferMap.java
index 3aabefa23b..a8bf61e9ee 100644
--- a/core/src/main/java/org/apache/iceberg/SerializableByteBufferMap.java
+++ b/core/src/main/java/org/apache/iceberg/SerializableByteBufferMap.java
@@ -22,6 +22,7 @@ import java.io.ObjectStreamException;
 import java.io.Serializable;
 import java.nio.ByteBuffer;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Map;
 import java.util.Set;
 import org.apache.iceberg.relocated.com.google.common.collect.Maps;
@@ -29,6 +30,7 @@ import org.apache.iceberg.util.ByteBuffers;
 
 class SerializableByteBufferMap implements Map<Integer, ByteBuffer>, Serializable {
   private final Map<Integer, ByteBuffer> wrapped;
+  private transient volatile Map<Integer, ByteBuffer> immutableMap;
 
   static Map<Integer, ByteBuffer> wrap(Map<Integer, ByteBuffer> map) {
     if (map == null) {
@@ -88,6 +90,18 @@ class SerializableByteBufferMap implements Map<Integer, ByteBuffer>, Serializabl
     return new MapSerializationProxy(keys, values);
   }
 
+  public Map<Integer, ByteBuffer> immutableMap() {
+    if (immutableMap == null) {
+      synchronized (this) {
+        if (immutableMap == null) {
+          immutableMap = Collections.unmodifiableMap(wrapped);
+        }
+      }
+    }
+
+    return immutableMap;
+  }
+
   @Override
   public int size() {
     return wrapped.size();
diff --git a/core/src/test/java/org/apache/iceberg/TestManifestReaderStats.java b/core/src/test/java/org/apache/iceberg/TestManifestReaderStats.java
index 53ec3c40cc..e794539d6d 100644
--- a/core/src/test/java/org/apache/iceberg/TestManifestReaderStats.java
+++ b/core/src/test/java/org/apache/iceberg/TestManifestReaderStats.java
@@ -27,6 +27,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
 import org.apache.iceberg.types.Conversions;
 import org.apache.iceberg.types.Types;
+import org.assertj.core.api.Assertions;
 import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -220,6 +221,42 @@ public class TestManifestReaderStats extends TableTestBase {
     Assert.assertEquals(LOWER_BOUNDS, dataFile.lowerBounds());
     Assert.assertEquals(UPPER_BOUNDS, dataFile.upperBounds());
 
+    if (dataFile.valueCounts() != null) {
+      Assertions.assertThatThrownBy(
+              () -> dataFile.valueCounts().clear(), "Should not be modifiable")
+          .isInstanceOf(UnsupportedOperationException.class);
+    }
+
+    if (dataFile.nullValueCounts() != null) {
+      Assertions.assertThatThrownBy(
+              () -> dataFile.nullValueCounts().clear(), "Should not be modifiable")
+          .isInstanceOf(UnsupportedOperationException.class);
+    }
+
+    if (dataFile.nanValueCounts() != null) {
+      Assertions.assertThatThrownBy(
+              () -> dataFile.nanValueCounts().clear(), "Should not be modifiable")
+          .isInstanceOf(UnsupportedOperationException.class);
+    }
+
+    if (dataFile.upperBounds() != null) {
+      Assertions.assertThatThrownBy(
+              () -> dataFile.upperBounds().clear(), "Should not be modifiable")
+          .isInstanceOf(UnsupportedOperationException.class);
+    }
+
+    if (dataFile.lowerBounds() != null) {
+      Assertions.assertThatThrownBy(
+              () -> dataFile.lowerBounds().clear(), "Should not be modifiable")
+          .isInstanceOf(UnsupportedOperationException.class);
+    }
+
+    if (dataFile.columnSizes() != null) {
+      Assertions.assertThatThrownBy(
+              () -> dataFile.columnSizes().clear(), "Should not be modifiable")
+          .isInstanceOf(UnsupportedOperationException.class);
+    }
+
     Assert.assertEquals(FILE_PATH, dataFile.path()); // always select file path in all test cases
   }