From 6df6c2f9a0533bb0f741cca7600579ace7d3f1cf Mon Sep 17 00:00:00 2001 From: Russell Spitzer Date: Fri, 1 Nov 2024 17:15:57 -0500 Subject: [PATCH 1/5] Removes Explicit Parameterization of Schema Tests --- .../java/org/apache/iceberg/TestSchema.java | 124 +++++++++++++----- 1 file changed, 91 insertions(+), 33 deletions(-) diff --git a/api/src/test/java/org/apache/iceberg/TestSchema.java b/api/src/test/java/org/apache/iceberg/TestSchema.java index fec7343c5cbc..e0c5e9586eb3 100644 --- a/api/src/test/java/org/apache/iceberg/TestSchema.java +++ b/api/src/test/java/org/apache/iceberg/TestSchema.java @@ -21,29 +21,30 @@ import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.util.List; +import java.util.Map; +import java.util.stream.IntStream; +import java.util.stream.Stream; +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.Type; import org.apache.iceberg.types.Types; -import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.FieldSource; +import org.junit.jupiter.params.provider.MethodSource; public class TestSchema { - private static final Schema TS_NANO_CASES = - new Schema( - Types.NestedField.required(1, "id", Types.LongType.get()), - Types.NestedField.optional(2, "ts", Types.TimestampNanoType.withZone()), - Types.NestedField.optional( - 3, "arr", Types.ListType.ofRequired(4, Types.TimestampNanoType.withoutZone())), - Types.NestedField.required( - 5, - "struct", - Types.StructType.of( - Types.NestedField.optional(6, "inner_ts", Types.TimestampNanoType.withZone()), - Types.NestedField.required(7, "data", Types.StringType.get()))), - Types.NestedField.optional( - 8, - "struct_arr", - Types.StructType.of( - Types.NestedField.optional(9, "ts", Types.TimestampNanoType.withoutZone())))); + + private static final List TESTTYPES = + ImmutableList.of(Types.TimestampNanoType.withoutZone(), Types.TimestampNanoType.withZone()); + + private static final Map MIN_FORMAT_VERSIONS = + ImmutableMap.of(Type.TypeID.TIMESTAMP_NANO, 3); + + private static final Integer MIN_FORMAT_INITIAL_DEFAULT = 3; + + private static final Integer MAX_FORMAT_VERSION = 3; private static final Schema INITIAL_DEFAULT_SCHEMA = new Schema( @@ -64,27 +65,77 @@ public class TestSchema { .withWriteDefault("--") .build()); + private Schema generateTypeSchema(Type type) { + return new Schema( + Types.NestedField.required(1, "id", Types.LongType.get()), + Types.NestedField.optional(2, "top", type), + Types.NestedField.optional(3, "arr", Types.ListType.ofRequired(4, type)), + Types.NestedField.required( + 5, + "struct", + Types.StructType.of( + Types.NestedField.optional(6, "inner_op", type), + Types.NestedField.required(7, "inner_req", type), + Types.NestedField.optional( + 8, + "struct_arr", + Types.StructType.of(Types.NestedField.optional(9, "deep", type)))))); + } + + private static Stream testTypeUnsupported() { + return TESTTYPES.stream() + .flatMap( + type -> + IntStream.range(1, MIN_FORMAT_VERSIONS.get(type.typeId())) + .mapToObj(unsupportedVersion -> Arguments.of(type, unsupportedVersion))); + } + @ParameterizedTest - @ValueSource(ints = {1, 2}) - public void testUnsupportedTimestampNano(int formatVersion) { - assertThatThrownBy(() -> Schema.checkCompatibility(TS_NANO_CASES, formatVersion)) + @MethodSource + public void testTypeUnsupported(Type type, int unsupportedVersion) { + assertThatThrownBy( + () -> Schema.checkCompatibility(generateTypeSchema(type), unsupportedVersion)) .isInstanceOf(IllegalStateException.class) .hasMessage( "Invalid schema for v%s:\n" - + "- Invalid type for ts: timestamptz_ns is not supported until v3\n" - + "- Invalid type for arr.element: timestamp_ns is not supported until v3\n" - + "- Invalid type for struct.inner_ts: timestamptz_ns is not supported until v3\n" - + "- Invalid type for struct_arr.ts: timestamp_ns is not supported until v3", - formatVersion); + + "- Invalid type for top: %s is not supported until v%s\n" + + "- Invalid type for arr.element: %s is not supported until v%s\n" + + "- Invalid type for struct.inner_op: %s is not supported until v%s\n" + + "- Invalid type for struct.inner_req: %s is not supported until v%s\n" + + "- Invalid type for struct.struct_arr.deep: %s is not supported until v%s", + unsupportedVersion, + type, + MIN_FORMAT_VERSIONS.get(type.typeId()), + type, + MIN_FORMAT_VERSIONS.get(type.typeId()), + type, + MIN_FORMAT_VERSIONS.get(type.typeId()), + type, + MIN_FORMAT_VERSIONS.get(type.typeId()), + type, + MIN_FORMAT_VERSIONS.get(type.typeId())); } - @Test - public void testSupportedTimestampNano() { - assertThatCode(() -> Schema.checkCompatibility(TS_NANO_CASES, 3)).doesNotThrowAnyException(); + private static Stream testTypeSupported() { + return TESTTYPES.stream() + .flatMap( + type -> + IntStream.range(MIN_FORMAT_VERSIONS.get(type.typeId()), MAX_FORMAT_VERSION + 1) + .mapToObj(unsupportedVersion -> Arguments.of(type, unsupportedVersion))); } @ParameterizedTest - @ValueSource(ints = {1, 2}) + @MethodSource + public void testTypeSupported(Type type, int supportedVersion) { + assertThatCode(() -> Schema.checkCompatibility(generateTypeSchema(type), supportedVersion)) + .doesNotThrowAnyException(); + } + + private static int[] testUnsupportedInitialDefault = + IntStream.range(1, MIN_FORMAT_INITIAL_DEFAULT).toArray(); + + @ParameterizedTest + @FieldSource public void testUnsupportedInitialDefault(int formatVersion) { assertThatThrownBy(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, formatVersion)) .isInstanceOf(IllegalStateException.class) @@ -95,14 +146,21 @@ public void testUnsupportedInitialDefault(int formatVersion) { formatVersion); } - @Test + private static int[] testSupportedInitialDefault = + IntStream.rangeClosed(MIN_FORMAT_INITIAL_DEFAULT, MAX_FORMAT_VERSION).toArray(); + + @ParameterizedTest + @FieldSource public void testSupportedInitialDefault() { assertThatCode(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, 3)) .doesNotThrowAnyException(); } + private static int[] testSupportedWriteDefault = + IntStream.rangeClosed(1, MAX_FORMAT_VERSION).toArray(); + @ParameterizedTest - @ValueSource(ints = {1, 2, 3}) + @FieldSource public void testSupportedWriteDefault(int formatVersion) { // only the initial default is a forward-incompatible change assertThatCode(() -> Schema.checkCompatibility(WRITE_DEFAULT_SCHEMA, formatVersion)) From c08f9b81b76a59d9e752e033ede18485b09cf322 Mon Sep 17 00:00:00 2001 From: Russell Spitzer Date: Fri, 1 Nov 2024 17:23:23 -0500 Subject: [PATCH 2/5] Forgot to update one test --- api/src/test/java/org/apache/iceberg/TestSchema.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/test/java/org/apache/iceberg/TestSchema.java b/api/src/test/java/org/apache/iceberg/TestSchema.java index e0c5e9586eb3..a0c18fb5988b 100644 --- a/api/src/test/java/org/apache/iceberg/TestSchema.java +++ b/api/src/test/java/org/apache/iceberg/TestSchema.java @@ -151,8 +151,8 @@ public void testUnsupportedInitialDefault(int formatVersion) { @ParameterizedTest @FieldSource - public void testSupportedInitialDefault() { - assertThatCode(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, 3)) + public void testSupportedInitialDefault(int formatVersion) { + assertThatCode(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, formatVersion)) .doesNotThrowAnyException(); } From 3bafca969ffc69e1cc173ea25eed7c35b7192749 Mon Sep 17 00:00:00 2001 From: Russell Spitzer Date: Mon, 4 Nov 2024 10:44:00 -0600 Subject: [PATCH 3/5] Reviewer Comments, Extract Constants --- .../main/java/org/apache/iceberg/Schema.java | 8 ++++++-- .../java/org/apache/iceberg/TestHelpers.java | 2 ++ .../java/org/apache/iceberg/TestSchema.java | 20 +++++++------------ 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/Schema.java b/api/src/main/java/org/apache/iceberg/Schema.java index 44f65ff56a54..a94e8771875a 100644 --- a/api/src/main/java/org/apache/iceberg/Schema.java +++ b/api/src/main/java/org/apache/iceberg/Schema.java @@ -28,6 +28,7 @@ import java.util.Map; import java.util.Set; import java.util.stream.Collectors; +import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting; import org.apache.iceberg.relocated.com.google.common.base.Joiner; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.collect.BiMap; @@ -54,8 +55,11 @@ public class Schema implements Serializable { private static final Joiner NEWLINE = Joiner.on('\n'); private static final String ALL_COLUMNS = "*"; private static final int DEFAULT_SCHEMA_ID = 0; - private static final int DEFAULT_VALUES_MIN_FORMAT_VERSION = 3; - private static final Map MIN_FORMAT_VERSIONS = + + @VisibleForTesting static final int DEFAULT_VALUES_MIN_FORMAT_VERSION = 3; + + @VisibleForTesting + static final Map MIN_FORMAT_VERSIONS = ImmutableMap.of(Type.TypeID.TIMESTAMP_NANO, 3); private final StructType struct; diff --git a/api/src/test/java/org/apache/iceberg/TestHelpers.java b/api/src/test/java/org/apache/iceberg/TestHelpers.java index ca3b1a908ac6..ecdf985bb2e2 100644 --- a/api/src/test/java/org/apache/iceberg/TestHelpers.java +++ b/api/src/test/java/org/apache/iceberg/TestHelpers.java @@ -53,6 +53,8 @@ public class TestHelpers { private TestHelpers() {} + public static final int MAX_FORMAT_VERSION = 3; + /** Wait in a tight check loop until system clock is past {@code timestampMillis} */ public static long waitUntilAfter(long timestampMillis) { long current = System.currentTimeMillis(); diff --git a/api/src/test/java/org/apache/iceberg/TestSchema.java b/api/src/test/java/org/apache/iceberg/TestSchema.java index a0c18fb5988b..2d50415464fd 100644 --- a/api/src/test/java/org/apache/iceberg/TestSchema.java +++ b/api/src/test/java/org/apache/iceberg/TestSchema.java @@ -18,15 +18,16 @@ */ package org.apache.iceberg; +import static org.apache.iceberg.Schema.DEFAULT_VALUES_MIN_FORMAT_VERSION; +import static org.apache.iceberg.Schema.MIN_FORMAT_VERSIONS; +import static org.apache.iceberg.TestHelpers.MAX_FORMAT_VERSION; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.util.List; -import java.util.Map; import java.util.stream.IntStream; import java.util.stream.Stream; 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.Type; import org.apache.iceberg.types.Types; import org.junit.jupiter.params.ParameterizedTest; @@ -39,13 +40,6 @@ public class TestSchema { private static final List TESTTYPES = ImmutableList.of(Types.TimestampNanoType.withoutZone(), Types.TimestampNanoType.withZone()); - private static final Map MIN_FORMAT_VERSIONS = - ImmutableMap.of(Type.TypeID.TIMESTAMP_NANO, 3); - - private static final Integer MIN_FORMAT_INITIAL_DEFAULT = 3; - - private static final Integer MAX_FORMAT_VERSION = 3; - private static final Schema INITIAL_DEFAULT_SCHEMA = new Schema( Types.NestedField.required(1, "id", Types.LongType.get()), @@ -120,8 +114,8 @@ private static Stream testTypeSupported() { return TESTTYPES.stream() .flatMap( type -> - IntStream.range(MIN_FORMAT_VERSIONS.get(type.typeId()), MAX_FORMAT_VERSION + 1) - .mapToObj(unsupportedVersion -> Arguments.of(type, unsupportedVersion))); + IntStream.rangeClosed(MIN_FORMAT_VERSIONS.get(type.typeId()), MAX_FORMAT_VERSION) + .mapToObj(supportedVersion -> Arguments.of(type, supportedVersion))); } @ParameterizedTest @@ -132,7 +126,7 @@ public void testTypeSupported(Type type, int supportedVersion) { } private static int[] testUnsupportedInitialDefault = - IntStream.range(1, MIN_FORMAT_INITIAL_DEFAULT).toArray(); + IntStream.range(1, DEFAULT_VALUES_MIN_FORMAT_VERSION).toArray(); @ParameterizedTest @FieldSource @@ -147,7 +141,7 @@ public void testUnsupportedInitialDefault(int formatVersion) { } private static int[] testSupportedInitialDefault = - IntStream.rangeClosed(MIN_FORMAT_INITIAL_DEFAULT, MAX_FORMAT_VERSION).toArray(); + IntStream.rangeClosed(DEFAULT_VALUES_MIN_FORMAT_VERSION, MAX_FORMAT_VERSION).toArray(); @ParameterizedTest @FieldSource From 11ac16686012d5e5d3209098ae92c93d44c59b32 Mon Sep 17 00:00:00 2001 From: Russell Spitzer Date: Wed, 6 Nov 2024 15:59:46 -0600 Subject: [PATCH 4/5] Use Expliclty Named Annotations --- .../java/org/apache/iceberg/TestHelpers.java | 1 + .../java/org/apache/iceberg/TestSchema.java | 28 +++++++++---------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/api/src/test/java/org/apache/iceberg/TestHelpers.java b/api/src/test/java/org/apache/iceberg/TestHelpers.java index ecdf985bb2e2..003e7835ed4b 100644 --- a/api/src/test/java/org/apache/iceberg/TestHelpers.java +++ b/api/src/test/java/org/apache/iceberg/TestHelpers.java @@ -54,6 +54,7 @@ public class TestHelpers { private TestHelpers() {} public static final int MAX_FORMAT_VERSION = 3; + public static final int[] ALL_VERSIONS = IntStream.rangeClosed(1, MAX_FORMAT_VERSION).toArray(); /** Wait in a tight check loop until system clock is past {@code timestampMillis} */ public static long waitUntilAfter(long timestampMillis) { diff --git a/api/src/test/java/org/apache/iceberg/TestSchema.java b/api/src/test/java/org/apache/iceberg/TestSchema.java index 2d50415464fd..834d9a15560a 100644 --- a/api/src/test/java/org/apache/iceberg/TestSchema.java +++ b/api/src/test/java/org/apache/iceberg/TestSchema.java @@ -37,7 +37,7 @@ public class TestSchema { - private static final List TESTTYPES = + private static final List TEST_TYPES = ImmutableList.of(Types.TimestampNanoType.withoutZone(), Types.TimestampNanoType.withZone()); private static final Schema INITIAL_DEFAULT_SCHEMA = @@ -76,8 +76,8 @@ private Schema generateTypeSchema(Type type) { Types.StructType.of(Types.NestedField.optional(9, "deep", type)))))); } - private static Stream testTypeUnsupported() { - return TESTTYPES.stream() + private static Stream unsupportedTypes() { + return TEST_TYPES.stream() .flatMap( type -> IntStream.range(1, MIN_FORMAT_VERSIONS.get(type.typeId())) @@ -85,8 +85,8 @@ private static Stream testTypeUnsupported() { } @ParameterizedTest - @MethodSource - public void testTypeUnsupported(Type type, int unsupportedVersion) { + @MethodSource("unsupportedTypes") + public void testUnsupportedTypes(Type type, int unsupportedVersion) { assertThatThrownBy( () -> Schema.checkCompatibility(generateTypeSchema(type), unsupportedVersion)) .isInstanceOf(IllegalStateException.class) @@ -110,8 +110,8 @@ public void testTypeUnsupported(Type type, int unsupportedVersion) { MIN_FORMAT_VERSIONS.get(type.typeId())); } - private static Stream testTypeSupported() { - return TESTTYPES.stream() + private static Stream supportedTypes() { + return TEST_TYPES.stream() .flatMap( type -> IntStream.rangeClosed(MIN_FORMAT_VERSIONS.get(type.typeId()), MAX_FORMAT_VERSION) @@ -119,17 +119,17 @@ private static Stream testTypeSupported() { } @ParameterizedTest - @MethodSource + @MethodSource("supportedTypes") public void testTypeSupported(Type type, int supportedVersion) { assertThatCode(() -> Schema.checkCompatibility(generateTypeSchema(type), supportedVersion)) .doesNotThrowAnyException(); } - private static int[] testUnsupportedInitialDefault = + private static int[] unsupportedInitialDefault = IntStream.range(1, DEFAULT_VALUES_MIN_FORMAT_VERSION).toArray(); @ParameterizedTest - @FieldSource + @FieldSource("unsupportedInitialDefault") public void testUnsupportedInitialDefault(int formatVersion) { assertThatThrownBy(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, formatVersion)) .isInstanceOf(IllegalStateException.class) @@ -140,21 +140,19 @@ public void testUnsupportedInitialDefault(int formatVersion) { formatVersion); } - private static int[] testSupportedInitialDefault = + private static int[] supportedInitialDefault = IntStream.rangeClosed(DEFAULT_VALUES_MIN_FORMAT_VERSION, MAX_FORMAT_VERSION).toArray(); @ParameterizedTest - @FieldSource + @FieldSource("supportedInitialDefault") public void testSupportedInitialDefault(int formatVersion) { assertThatCode(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, formatVersion)) .doesNotThrowAnyException(); } - private static int[] testSupportedWriteDefault = - IntStream.rangeClosed(1, MAX_FORMAT_VERSION).toArray(); @ParameterizedTest - @FieldSource + @FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS") public void testSupportedWriteDefault(int formatVersion) { // only the initial default is a forward-incompatible change assertThatCode(() -> Schema.checkCompatibility(WRITE_DEFAULT_SCHEMA, formatVersion)) From 8d0f548196c40eb2c1258a36e71b3459d8f947d9 Mon Sep 17 00:00:00 2001 From: Russell Spitzer Date: Wed, 6 Nov 2024 16:15:55 -0600 Subject: [PATCH 5/5] Spotless Apply --- api/src/test/java/org/apache/iceberg/TestSchema.java | 1 - 1 file changed, 1 deletion(-) diff --git a/api/src/test/java/org/apache/iceberg/TestSchema.java b/api/src/test/java/org/apache/iceberg/TestSchema.java index 834d9a15560a..e79adbd09fb7 100644 --- a/api/src/test/java/org/apache/iceberg/TestSchema.java +++ b/api/src/test/java/org/apache/iceberg/TestSchema.java @@ -150,7 +150,6 @@ public void testSupportedInitialDefault(int formatVersion) { .doesNotThrowAnyException(); } - @ParameterizedTest @FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS") public void testSupportedWriteDefault(int formatVersion) {