talatuyarer commented on code in PR #14266:
URL: https://github.com/apache/iceberg/pull/14266#discussion_r2408948602
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -281,7 +297,7 @@ public UpdateSchema updateColumn(String name,
Type.PrimitiveType newType) {
}
Preconditions.checkArgument(
- TypeUtil.isPromotionAllowed(field.type(), newType),
+ TypeUtil.isPromotionAllowed(field.type(), newType, formatVersion),
Review Comment:
You need to add Partition Field Validation too. [According to
spec](https://iceberg.apache.org/spec/#:~:text=Type%20promotion%20is,timestamp%20or%20timestamp_ns)
`Type promotion is not allowed for a field that is referenced by source-id or
source-ids of a partition field if the partition transform would produce a
different value after promoting the type.`
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -400,13 +400,31 @@ public static Type find(Type type, Predicate<Type>
predicate) {
}
public static boolean isPromotionAllowed(Type from, Type.PrimitiveType to) {
+ return TypeUtil.isPromotionAllowed(from, to, 2);
+ }
+
+ public static boolean isPromotionAllowed(
+ Type from, Type.PrimitiveType to, Integer formatVersion) {
// Warning! Before changing this function, make sure that the type change
doesn't introduce
// compatibility problems in partitioning.
if (from.equals(to)) {
return true;
}
switch (from.typeId()) {
+ case DATE:
+ if (formatVersion < 3) {
+ return false;
+ } else if (to.typeId() == Type.TypeID.TIMESTAMP) {
+ // Timezone types cannot be promoted.
+ Types.TimestampType toTs = (Types.TimestampType) to;
+ return Types.TimestampType.withoutZone().equals(toTs);
+ } else if (to.typeId() == Type.TypeID.TIMESTAMP_NANO) {
+ // Timezone types cannot be promoted.
+ Types.TimestampNanoType toTs = (Types.TimestampNanoType) to;
+ return Types.TimestampNanoType.withoutZone().equals(toTs);
+ }
+ // fall through
Review Comment:
You did not cover `Date → anything else` case. IMHO You should return false
explicitly for any non applicable case.
##########
api/src/main/java/org/apache/iceberg/types/CheckCompatibility.java:
##########
@@ -38,7 +38,20 @@ public class CheckCompatibility extends
TypeUtil.CustomOrderSchemaVisitor<List<S
* @return a list of error details, or an empty list if there are no
compatibility problems
*/
public static List<String> writeCompatibilityErrors(Schema readSchema,
Schema writeSchema) {
- return writeCompatibilityErrors(readSchema, writeSchema, true);
+ return writeCompatibilityErrors(readSchema, writeSchema, true, 2);
+ }
+
+ /**
+ * Returns a list of compatibility errors for writing with the given write
schema. This includes
+ * nullability: writing optional (nullable) values to a required field is an
error.
+ *
+ * @param readSchema a read schema
+ * @param writeSchema a write schema
+ * @return a list of error details, or an empty list if there are no
compatibility problems
+ */
+ public static List<String> writeCompatibilityErrors(
+ Schema readSchema, Schema writeSchema, int formatVersion) {
+ return writeCompatibilityErrors(readSchema, writeSchema, true,
formatVersion);
Review Comment:
Please update javadoc whereever you add formatversion.
##########
core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java:
##########
@@ -2204,8 +2222,7 @@ public void
testDeleteContainingNestedIdentifierFieldColumnsFails() {
new SchemaUpdate(newSchema, SCHEMA_LAST_COLUMN_ID +
2).deleteColumn("out").apply())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(
- "Cannot delete field 24: out: required struct<25: nested: required
string> "
- + "as it will delete nested identifier field 25: nested:
required string");
+ "Cannot delete field 26: out: required struct<27: nested: required
string> as it will delete nested identifier field 27: nested: required string");
}
Review Comment:
You introduce new functionally which should be covered by new test cases. At
least this can be good cases to start :
- v2 tables rejecting date→timestamp promotion
- v3 tables rejecting date→timestamptz promotion
- Partitioned date fields rejecting promotion
##########
api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java:
##########
@@ -70,7 +70,7 @@ public void testPrimitiveTypes() {
CheckCompatibility.writeCompatibilityErrors(
new Schema(required(1, "to_field", to)), fromSchema);
- if (TypeUtil.isPromotionAllowed(from, to)) {
+ if (TypeUtil.isPromotionAllowed(from, to, 2)) {
Review Comment:
This means date promotions will fail in this test (as expected). But should
there be a v3-specific test showing date promotions work?
--
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]
