szehon-ho commented on code in PR #17957:
URL: https://github.com/apache/iceberg/pull/17957#discussion_r4170027177
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkTable.java:
##########
@@ -158,6 +173,90 @@ public Set<TableCapability> capabilities() {
return capabilities;
}
+ @Override
+ public boolean supportsColumnChange(TableChange.ColumnChange change) {
+ if (isMapKeyChange(change)) {
+ return false;
+ }
+
+ if (change instanceof TableChange.AddColumn) {
+ TableChange.AddColumn add = (TableChange.AddColumn) change;
+ return add.isNullable() && add.defaultValue() == null &&
canConvert(add.dataType());
+ } else if (change instanceof TableChange.UpdateColumnType) {
+ return supportsTypeUpdate((TableChange.UpdateColumnType) change);
+ } else if (change instanceof TableChange.UpdateColumnNullability) {
+ return ((TableChange.UpdateColumnNullability) change).nullable();
Review Comment:
Please also reject making an identifier field, or a struct containing one,
nullable. `SchemaUpdate.apply()` rejects both through
`Schema.validateIdentifierField`. Spark 4.2 currently generates only additions
and type updates, so this is a hook-contract edge case that could be covered
with a unit test.
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkTable.java:
##########
@@ -125,6 +139,7 @@ private SparkTable(
Table table, Schema schema, Snapshot snapshot, String branch, TimeTravel
timeTravel) {
super(table, schema);
this.schema = schema;
+ this.mapKeyFieldIds = mapKeyFieldIds(schema);
Review Comment:
Could we lazily compute and cache `mapKeyFieldIds` when it is first needed?
Eager initialization traverses the schema and allocates indexes for every
`SparkTable` load, including reads and tables with schema evolution disabled.
The schema is pinned, so the cache needs no invalidation.
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkTable.java:
##########
@@ -158,6 +173,90 @@ public Set<TableCapability> capabilities() {
return capabilities;
}
+ @Override
+ public boolean supportsColumnChange(TableChange.ColumnChange change) {
+ if (isMapKeyChange(change)) {
+ return false;
+ }
+
+ if (change instanceof TableChange.AddColumn) {
+ TableChange.AddColumn add = (TableChange.AddColumn) change;
+ return add.isNullable() && add.defaultValue() == null &&
canConvert(add.dataType());
Review Comment:
Please check the converted type against the table’s format version before
returning true, including nested types. `VariantType` and `NullType` convert
successfully, but v1/v2 tables reject their Iceberg types in
`Schema.checkCompatibility` during commit. Tests for v2 rejection and v3
acceptance would cover this.
--
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]
