You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "RussellSpitzer (via GitHub)" <gi...@apache.org> on 2024/11/01 22:16:22 UTC
[PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
RussellSpitzer opened a new pull request, #11444:
URL: https://github.com/apache/iceberg/pull/11444
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1830494527
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -18,32 +18,27 @@
*/
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.stream.IntStream;
+import java.util.stream.Stream;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+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<Type> TESTTYPES =
Review Comment:
```suggestion
private static final List<Type> TS_NANO_TYPES =
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1830490067
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -95,14 +140,21 @@ public void testUnsupportedInitialDefault(int formatVersion) {
formatVersion);
}
- @Test
- public void testSupportedInitialDefault() {
- assertThatCode(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, 3))
+ private static int[] testSupportedInitialDefault =
+ IntStream.rangeClosed(DEFAULT_VALUES_MIN_FORMAT_VERSION, MAX_FORMAT_VERSION).toArray();
+
+ @ParameterizedTest
+ @FieldSource
+ public void testSupportedInitialDefault(int formatVersion) {
+ assertThatCode(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, formatVersion))
.doesNotThrowAnyException();
}
+ private static int[] testSupportedWriteDefault =
Review Comment:
```suggestion
private static int[] supportedWriteDefault =
```
I would also move this to the top
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1826341709
##########
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<Type> TESTTYPES =
+ ImmutableList.of(Types.TimestampNanoType.withoutZone(), Types.TimestampNanoType.withZone());
+
+ private static final Map<Type.TypeID, Integer> 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;
Review Comment:
This is a test constant i'd really like to have in just one file somewhere in API, placed here now to demonstrate what I want to do
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1830492332
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -64,27 +59,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<Arguments> 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
Review Comment:
```suggestion
@MethodSource(value = "unsupportedTypes")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1830491512
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -64,27 +59,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<Arguments> 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<Arguments> testTypeSupported() {
Review Comment:
```suggestion
private static Stream<Arguments> supportedTypes() {
```
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -64,27 +59,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<Arguments> 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<Arguments> testTypeSupported() {
+ return TESTTYPES.stream()
+ .flatMap(
+ type ->
+ IntStream.rangeClosed(MIN_FORMAT_VERSIONS.get(type.typeId()), MAX_FORMAT_VERSION)
+ .mapToObj(supportedVersion -> Arguments.of(type, supportedVersion)));
}
@ParameterizedTest
- @ValueSource(ints = {1, 2})
+ @MethodSource
Review Comment:
```suggestion
@MethodSource(value = "supportedTypes")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer merged PR #11444:
URL: https://github.com/apache/iceberg/pull/11444
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#issuecomment-2452666774
Test results look like
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1826341442
##########
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<Type> TESTTYPES =
+ ImmutableList.of(Types.TimestampNanoType.withoutZone(), Types.TimestampNanoType.withZone());
+
+ private static final Map<Type.TypeID, Integer> MIN_FORMAT_VERSIONS =
+ ImmutableMap.of(Type.TypeID.TIMESTAMP_NANO, 3);
+
+ private static final Integer MIN_FORMAT_INITIAL_DEFAULT = 3;
Review Comment:
I think maybe I should just be extracting these from Schema rather than redeclaring them here ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1826485532
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -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<Arguments> 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<Arguments> testTypeSupported() {
+ return TESTTYPES.stream()
+ .flatMap(
+ type ->
+ IntStream.range(MIN_FORMAT_VERSIONS.get(type.typeId()), MAX_FORMAT_VERSION + 1)
+ .mapToObj(unsupportedVersion -> Arguments.of(type, unsupportedVersion)));
Review Comment:
unsupportedVersion => supportedVersion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1830492172
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -64,27 +59,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<Arguments> testTypeUnsupported() {
Review Comment:
```suggestion
private static Stream<Arguments> unsupportedTypes() {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#issuecomment-2462576360
Thanks @aihuaxu , @Fokko and @nastra for reviews!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1830490735
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -95,14 +140,21 @@ public void testUnsupportedInitialDefault(int formatVersion) {
formatVersion);
}
- @Test
- public void testSupportedInitialDefault() {
- assertThatCode(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, 3))
+ private static int[] testSupportedInitialDefault =
+ IntStream.rangeClosed(DEFAULT_VALUES_MIN_FORMAT_VERSION, MAX_FORMAT_VERSION).toArray();
+
+ @ParameterizedTest
+ @FieldSource
+ 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
- @ValueSource(ints = {1, 2, 3})
+ @FieldSource
Review Comment:
```suggestion
@FieldSource(value = "supportedWriteDefault")
```
this makes it clearer which field parameters are actually being used. I would also update all the others
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1826348277
##########
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<Type> TESTTYPES =
+ ImmutableList.of(Types.TimestampNanoType.withoutZone(), Types.TimestampNanoType.withZone());
+
+ private static final Map<Type.TypeID, Integer> MIN_FORMAT_VERSIONS =
+ ImmutableMap.of(Type.TypeID.TIMESTAMP_NANO, 3);
+
+ private static final Integer MIN_FORMAT_INITIAL_DEFAULT = 3;
Review Comment:
+1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1828047216
##########
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<Type> TESTTYPES =
+ ImmutableList.of(Types.TimestampNanoType.withoutZone(), Types.TimestampNanoType.withZone());
+
+ private static final Map<Type.TypeID, Integer> MIN_FORMAT_VERSIONS =
+ ImmutableMap.of(Type.TypeID.TIMESTAMP_NANO, 3);
+
+ private static final Integer MIN_FORMAT_INITIAL_DEFAULT = 3;
Review Comment:
My only worry here is we won't catch bugs in bad constants in Schema.java, probably not a real worry
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1831723193
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -18,32 +18,27 @@
*/
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.stream.IntStream;
+import java.util.stream.Stream;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+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<Type> TESTTYPES =
Review Comment:
This is about to have Variant in it as well, so I was keeping it generic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Removes Explicit Parameterization of Schema Tests [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11444:
URL: https://github.com/apache/iceberg/pull/11444#discussion_r1831723691
##########
api/src/test/java/org/apache/iceberg/TestSchema.java:
##########
@@ -95,14 +140,21 @@ public void testUnsupportedInitialDefault(int formatVersion) {
formatVersion);
}
- @Test
- public void testSupportedInitialDefault() {
- assertThatCode(() -> Schema.checkCompatibility(INITIAL_DEFAULT_SCHEMA, 3))
+ private static int[] testSupportedInitialDefault =
+ IntStream.rangeClosed(DEFAULT_VALUES_MIN_FORMAT_VERSION, MAX_FORMAT_VERSION).toArray();
+
+ @ParameterizedTest
+ @FieldSource
+ 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
- @ValueSource(ints = {1, 2, 3})
+ @FieldSource
Review Comment:
That's fine, I kind of like how it just uses the exact same name though
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org