Guosmilesmile commented on code in PR #15795:
URL: https://github.com/apache/iceberg/pull/15795#discussion_r3019371759
##########
data/src/test/java/org/apache/iceberg/data/DataGenerators.java:
##########
@@ -29,10 +29,52 @@
*/
class DataGenerators {
- static final DataGenerator[] ALL = new DataGenerator[] {new
StructOfPrimitive()};
+ static final DataGenerator[] ALL =
+ new DataGenerator[] {new StructOfPrimitive(), new DefaultSchema(), new
AllTypes()};
Review Comment:
If `AllTypes` already covers all types, then I think both `DefaultSchema`
and `StructOfPrimitive` are no longer needed.WDYT?
##########
data/src/test/java/org/apache/iceberg/data/DataGenerators.java:
##########
@@ -29,10 +29,52 @@
*/
class DataGenerators {
- static final DataGenerator[] ALL = new DataGenerator[] {new
StructOfPrimitive()};
+ static final DataGenerator[] ALL =
+ new DataGenerator[] {new StructOfPrimitive(), new DefaultSchema(), new
AllTypes()};
private DataGenerators() {}
+ static class AllTypes implements DataGenerator {
+ private final Schema schema =
+ new Schema(
+ Types.NestedField.required(1, "boolean_col",
Types.BooleanType.get()),
+ Types.NestedField.required(2, "int_col", Types.IntegerType.get()),
+ Types.NestedField.required(3, "long_col", Types.LongType.get()),
+ Types.NestedField.required(4, "float_col", Types.FloatType.get()),
+ Types.NestedField.required(5, "double_col",
Types.DoubleType.get()),
+ Types.NestedField.required(6, "decimal_col",
Types.DecimalType.of(9, 2)),
+ Types.NestedField.required(7, "date_col", Types.DateType.get()),
+ Types.NestedField.required(8, "time_col", Types.TimeType.get()),
+ Types.NestedField.required(9, "timestamp_col",
Types.TimestampType.withoutZone()),
+ Types.NestedField.required(10, "timestamp_tz_col",
Types.TimestampType.withZone()),
+ Types.NestedField.required(11, "string_col",
Types.StringType.get()),
+ Types.NestedField.required(12, "uuid_col", Types.UUIDType.get()),
+ Types.NestedField.required(13, "fixed_col",
Types.FixedType.ofLength(16)),
+ Types.NestedField.required(14, "binary_col",
Types.BinaryType.get()),
+ Types.NestedField.required(
+ 15, "list_col", Types.ListType.ofRequired(16,
Types.StringType.get())),
+ Types.NestedField.required(
+ 17,
+ "map_col",
+ Types.MapType.ofRequired(18, 19, Types.StringType.get(),
Types.IntegerType.get())),
+ Types.NestedField.required(
+ 20,
+ "struct_col",
+ Types.StructType.of(
+ Types.NestedField.required(21, "nested_int",
Types.IntegerType.get()),
+ Types.NestedField.required(22, "nested_string",
Types.StringType.get()))));
Review Comment:
Can we also add the `variant` type?
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -77,6 +77,14 @@ protected boolean supportsBatchReads() {
return false;
}
+ /**
+ * Hook for subclasses to declare whether they support all types in the
given schema. Default is
+ * to support all schemas. If false is returned, the test will be skipped
using assumeTrue.
+ */
+ protected boolean supports(Schema schema) {
+ return true;
+ }
+
Review Comment:
We put all types in a single schema, and if any type is not supported by the
engine, the entire test case will be skipped, which means the validation of
other types will also be skipped. Is this granularity too coarse?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
