drexler-sky commented on code in PR #18885:
URL: https://github.com/apache/iceberg/pull/18885#discussion_r4559701158
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -506,22 +506,33 @@ private TableMetadata
applyChangesToMetadata(TableMetadata metadata) {
// Transform the metrics if they exist
if (base != null && base.properties() != null) {
- Schema newSchema = newMetadata.schema();
- List<String> deletedColumns =
-
deletes.stream().map(schema::findColumnName).collect(Collectors.toList());
- Map<String, String> renamedColumns =
- updates.keySet().stream()
- .filter(id -> !addedNameToId.containsValue(id)) // remove added
columns
- .filter(id ->
!schema.findColumnName(id).equals(newSchema.findColumnName(id)))
- .collect(Collectors.toMap(schema::findColumnName,
newSchema::findColumnName));
+ Map<Integer, String> newIdToName = newMetadata.schema().idToName();
+ List<String> deletedColumns = Lists.newArrayList();
+ Map<String, String> renamedColumns = Maps.newHashMap();
+ // compare all fields so that nested fields of a deleted or renamed
struct are included
+ schema
+ .idToName()
+ .forEach(
+ (id, name) -> {
+ String newName = newIdToName.get(id);
+ if (newName == null) {
+ deletedColumns.add(name);
Review Comment:
Thanks for the review! This is still decided by field id. The loop goes
through each field id in the old schema and looks up the same id in the new
schema. Since field ids never change, an id is missing from the new schema only
when that field, or a parent of that field, was dropped. So the result is
deterministic, and the only new entries are the fields nested under a dropped
or renamed column.
Agreed that these properties are only meaningful for leaf columns. That is
why this change is needed. For example, with `location struct<lat, long>` and a
property on `location.lat`, `deleteColumn("location")` used to only look at
`location` itself, so the property on the leaf `location.lat` was left behind.
The new code includes the leaf columns, so it is removed. List element and map
key/value paths work the same way, e.g. a property on `points.element.x` is
removed when `points` is dropped.
Non-leaf names like `location` or `points.element` are also in the list, but
they don't match any property, so they have no effect.
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -506,22 +506,33 @@ private TableMetadata
applyChangesToMetadata(TableMetadata metadata) {
// Transform the metrics if they exist
if (base != null && base.properties() != null) {
- Schema newSchema = newMetadata.schema();
- List<String> deletedColumns =
-
deletes.stream().map(schema::findColumnName).collect(Collectors.toList());
- Map<String, String> renamedColumns =
- updates.keySet().stream()
- .filter(id -> !addedNameToId.containsValue(id)) // remove added
columns
- .filter(id ->
!schema.findColumnName(id).equals(newSchema.findColumnName(id)))
- .collect(Collectors.toMap(schema::findColumnName,
newSchema::findColumnName));
+ Map<Integer, String> newIdToName = newMetadata.schema().idToName();
+ List<String> deletedColumns = Lists.newArrayList();
+ Map<String, String> renamedColumns = Maps.newHashMap();
+ // compare all fields so that nested fields of a deleted or renamed
struct are included
+ schema
+ .idToName()
Review Comment:
Good point, thanks. Added a check so the schema comparison only runs when
there are deletes or renames. Add-only updates skip it now, same as before this
PR.
--
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]
