From 24917055b7068545a339179daa76b4e917671f88 Mon Sep 17 00:00:00 2001 From: Hongyue Zhang Date: Mon, 23 Mar 2026 14:45:57 -0700 Subject: [PATCH 1/2] [Core] Fix NPE of generateRandomMetrics in FileGenerationUtil --- .../apache/iceberg/FileGenerationUtil.java | 4 ++ .../iceberg/TestFileGenerationUtil.java | 52 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/core/src/test/java/org/apache/iceberg/FileGenerationUtil.java b/core/src/test/java/org/apache/iceberg/FileGenerationUtil.java index b08d43e572a2..84aff4279e74 100644 --- a/core/src/test/java/org/apache/iceberg/FileGenerationUtil.java +++ b/core/src/test/java/org/apache/iceberg/FileGenerationUtil.java @@ -280,6 +280,10 @@ private static long generateContentLength() { } private static Pair generateBounds(PrimitiveType type, MetricsMode mode) { + if (mode instanceof None || mode instanceof Counts) { + return Pair.of(null, null); + } + Comparator cmp = Comparators.forType(type); Object value1 = generateBound(type, mode); Object value2 = generateBound(type, mode); diff --git a/core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java b/core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java index ea44aa73c6d6..4b61e47ab23d 100644 --- a/core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java +++ b/core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java @@ -31,6 +31,8 @@ import org.apache.iceberg.types.Types; import org.apache.iceberg.types.Types.NestedField; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; public class TestFileGenerationUtil { @@ -44,6 +46,21 @@ public class TestFileGenerationUtil { required(6, "timestamp_tz_col", Types.TimestampType.withZone()), required(7, "str_col", Types.StringType.get())); + enum MetricsModeCase { + NONE("none", false), + COUNTS("counts", false), + TRUNCATE("truncate(16)", true), + FULL("full", true); + + final String modeName; + final boolean hasBounds; + + MetricsModeCase(String modeName, boolean hasBounds) { + this.modeName = modeName; + this.hasBounds = hasBounds; + } + } + @Test public void testBoundsWithDefaultMetricsConfig() { MetricsConfig metricsConfig = MetricsConfig.getDefault(); @@ -85,6 +102,41 @@ public void testBoundsWithSpecificValues() { assertThat(actualIntUpper).isEqualTo(intUpper); } + @ParameterizedTest + @EnumSource(MetricsModeCase.class) + @SuppressWarnings("deprecation") + void testBoundsForAllMetricsModes(MetricsModeCase modeCase) { + MetricsConfig metricsConfig = + MetricsConfig.fromProperties( + ImmutableMap.of(TableProperties.DEFAULT_WRITE_METRICS_MODE, modeCase.modeName)); + Metrics metrics = + FileGenerationUtil.generateRandomMetrics( + SCHEMA, + metricsConfig, + ImmutableMap.of() /* no lower bounds */, + ImmutableMap.of() /* no upper bounds */); + + for (NestedField field : SCHEMA.columns()) { + ByteBuffer lower = metrics.lowerBounds().get(field.fieldId()); + ByteBuffer upper = metrics.upperBounds().get(field.fieldId()); + if (modeCase.hasBounds) { + assertThat(lower) + .as("lower bound for %s in mode %s", field.name(), modeCase.modeName) + .isNotNull(); + assertThat(upper) + .as("upper bound for %s in mode %s", field.name(), modeCase.modeName) + .isNotNull(); + } else { + assertThat(lower) + .as("lower bound for %s in mode %s", field.name(), modeCase.modeName) + .isNull(); + assertThat(upper) + .as("upper bound for %s in mode %s", field.name(), modeCase.modeName) + .isNull(); + } + } + } + private void checkBounds(Metrics metrics, MetricsConfig metricsConfig) { for (NestedField field : SCHEMA.columns()) { MetricsMode mode = metricsConfig.columnMode(field.name()); From 4ce4055bede77f3624a472ae7efb4cdb4f7bcb37 Mon Sep 17 00:00:00 2001 From: Hongyue Zhang Date: Tue, 24 Mar 2026 15:42:59 -0700 Subject: [PATCH 2/2] Address review feedback --- .../apache/iceberg/FileGenerationUtil.java | 8 ++-- .../iceberg/TestFileGenerationUtil.java | 43 +++---------------- 2 files changed, 9 insertions(+), 42 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/FileGenerationUtil.java b/core/src/test/java/org/apache/iceberg/FileGenerationUtil.java index 84aff4279e74..1d57d96203b9 100644 --- a/core/src/test/java/org/apache/iceberg/FileGenerationUtil.java +++ b/core/src/test/java/org/apache/iceberg/FileGenerationUtil.java @@ -280,13 +280,13 @@ private static long generateContentLength() { } private static Pair generateBounds(PrimitiveType type, MetricsMode mode) { - if (mode instanceof None || mode instanceof Counts) { + Object value1 = generateBound(type, mode); + Object value2 = generateBound(type, mode); + + if (value1 == null || value2 == null) { return Pair.of(null, null); } - Comparator cmp = Comparators.forType(type); - Object value1 = generateBound(type, mode); - Object value2 = generateBound(type, mode); if (cmp.compare(value1, value2) > 0) { ByteBuffer lowerBuffer = Conversions.toByteBuffer(type, value2); ByteBuffer upperBuffer = Conversions.toByteBuffer(type, value1); diff --git a/core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java b/core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java index 4b61e47ab23d..f533df159893 100644 --- a/core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java +++ b/core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java @@ -32,7 +32,7 @@ import org.apache.iceberg.types.Types.NestedField; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.ValueSource; public class TestFileGenerationUtil { @@ -46,21 +46,6 @@ public class TestFileGenerationUtil { required(6, "timestamp_tz_col", Types.TimestampType.withZone()), required(7, "str_col", Types.StringType.get())); - enum MetricsModeCase { - NONE("none", false), - COUNTS("counts", false), - TRUNCATE("truncate(16)", true), - FULL("full", true); - - final String modeName; - final boolean hasBounds; - - MetricsModeCase(String modeName, boolean hasBounds) { - this.modeName = modeName; - this.hasBounds = hasBounds; - } - } - @Test public void testBoundsWithDefaultMetricsConfig() { MetricsConfig metricsConfig = MetricsConfig.getDefault(); @@ -103,12 +88,12 @@ public void testBoundsWithSpecificValues() { } @ParameterizedTest - @EnumSource(MetricsModeCase.class) + @ValueSource(strings = {"none", "counts", "truncate(16)", "full"}) @SuppressWarnings("deprecation") - void testBoundsForAllMetricsModes(MetricsModeCase modeCase) { + void testBoundsForAllMetricsModes(String metricsMode) { MetricsConfig metricsConfig = MetricsConfig.fromProperties( - ImmutableMap.of(TableProperties.DEFAULT_WRITE_METRICS_MODE, modeCase.modeName)); + ImmutableMap.of(TableProperties.DEFAULT_WRITE_METRICS_MODE, metricsMode)); Metrics metrics = FileGenerationUtil.generateRandomMetrics( SCHEMA, @@ -116,25 +101,7 @@ void testBoundsForAllMetricsModes(MetricsModeCase modeCase) { ImmutableMap.of() /* no lower bounds */, ImmutableMap.of() /* no upper bounds */); - for (NestedField field : SCHEMA.columns()) { - ByteBuffer lower = metrics.lowerBounds().get(field.fieldId()); - ByteBuffer upper = metrics.upperBounds().get(field.fieldId()); - if (modeCase.hasBounds) { - assertThat(lower) - .as("lower bound for %s in mode %s", field.name(), modeCase.modeName) - .isNotNull(); - assertThat(upper) - .as("upper bound for %s in mode %s", field.name(), modeCase.modeName) - .isNotNull(); - } else { - assertThat(lower) - .as("lower bound for %s in mode %s", field.name(), modeCase.modeName) - .isNull(); - assertThat(upper) - .as("upper bound for %s in mode %s", field.name(), modeCase.modeName) - .isNull(); - } - } + checkBounds(metrics, metricsConfig); } private void checkBounds(Metrics metrics, MetricsConfig metricsConfig) {