Guosmilesmile commented on code in PR #15795:
URL: https://github.com/apache/iceberg/pull/15795#discussion_r3200263625
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkFormatModel.java:
##########
@@ -31,6 +31,13 @@
public class TestSparkFormatModel extends BaseFormatModelTests<InternalRow> {
Review Comment:
We miss the override type support in `BaseFormatModelTests`.
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -102,6 +108,40 @@ protected boolean supportsBatchReads() {
@TempDir private File tableDir;
+ protected abstract boolean supportsTime();
+
+ protected abstract boolean supportsUUID();
+
+ protected abstract boolean supportsTimestampNano();
+
+ protected abstract boolean supportsVariant();
+
+ protected abstract boolean supportsUnknown();
Review Comment:
Can we set the default value to true in the base class, so that each engine
only needs to override the types it doesn't support?
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1518,18 +1571,44 @@ private static Object[] computeMinMax(
continue;
}
- if (min == null || cmp.compare(value, min) < 0) {
- min = value;
+ // Convert to internal Iceberg type for comparison
+ Object internalValue = toInternal(field.type(), value);
+
+ if (min == null || cmp.compare(internalValue, min) < 0) {
+ min = internalValue;
}
- if (max == null || cmp.compare(value, max) > 0) {
- max = value;
+ if (max == null || cmp.compare(internalValue, max) > 0) {
+ max = internalValue;
}
}
return new Object[] {min, max};
}
+ private static Object toInternal(Type type, Object value) {
+ if (value == null) {
+ return null;
+ }
+
+ switch (type.typeId()) {
+ case DATE:
+ return DateTimeUtil.daysFromDate((LocalDate) value);
+ case TIME:
+ return DateTimeUtil.microsFromTime((LocalTime) value);
Review Comment:
Should this be int or long instead of LocalDate?
##########
data/src/test/java/org/apache/iceberg/data/DataGenerators.java:
##########
@@ -29,10 +29,91 @@
*/
class DataGenerators {
- static final DataGenerator[] ALL = new DataGenerator[] {new
StructOfPrimitive()};
+ static final DataGenerator[] ALL =
+ new DataGenerator[] {
+ new Primitives(),
+ new UUID(),
+ new Fixed(),
+ new Binary(),
+ new StructOfPrimitive(),
+ new ListOfPrimitive(),
+ new MapOfPrimitive(),
+ new TimestampNano()
+ };
private DataGenerators() {}
+ static class Primitives implements DataGenerator {
Review Comment:
Do we need to expand the type coverage in defaultSchema?
--
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]
