dramaticlly commented on code in PR #18885:
URL: https://github.com/apache/iceberg/pull/18885#discussion_r4568274253
##########
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:
Thanks for the check, it also get me thinking maybe instead o relying on the
internal state such as deleted or renamed, maybe we can iterate the existing
table properties instead. This help ensure the new schema indexing is only
triggered when there's existing column level configurable table properties set
on the table.
So the basic idea is to find all existing properties with column properties
prefix, to get the existing field names, then use old schema to find the field
id and then look up the field id in the new schema to see if rename or drop
happens.
- if leaf field or parent struct renamed, then the new property key will be
`prefix + newName`
- if leaf field or parent struct dropped, then can skip this line in new
properties
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -51,6 +51,16 @@
class SchemaUpdate implements UpdateSchema {
private static final Logger LOG =
LoggerFactory.getLogger(SchemaUpdate.class);
private static final int TABLE_ROOT_ID = -1;
+ private static final Set<String> COLUMN_PROPERTY_PREFIXES =
Review Comment:
I am wondering if we shall move COLUMN_PROPERTY_PREFIXES set to
TableProperties instead, the reason is that more inclusive for future new
column level configurable properties
--
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]
