Fokko commented on code in PR #9347: URL: https://github.com/apache/iceberg/pull/9347#discussion_r1433693770
########## core/src/main/java/org/apache/iceberg/avro/ApplyNameMapping.java: ########## @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.avro; + +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; +import org.apache.avro.LogicalType; +import org.apache.avro.Schema; +import org.apache.iceberg.mapping.MappedField; +import org.apache.iceberg.mapping.NameMapping; +import org.apache.iceberg.relocated.com.google.common.collect.Lists; + +/** + * An Avro Schema visitor to apply a name mapping to add Iceberg field IDs. + * + * <p>Methods return null when a schema has no ID and cannot be projected. + */ +public class ApplyNameMapping extends AvroSchemaVisitor<Schema> { + private final NameMapping nameMapping; + + public ApplyNameMapping(NameMapping nameMapping) { + this.nameMapping = nameMapping; + } + + @Override + public Schema record(Schema record, List<String> names, List<Schema> fields) { + List<Schema.Field> originalFields = record.getFields(); + + List<Schema.Field> newFields = Lists.newArrayList(); + for (int i = 0; i < originalFields.size(); i += 1) { + Schema newSchema = fields.get(i); + if (newSchema != null) { + Schema.Field field = originalFields.get(i); + Integer fieldId = AvroSchemaUtil.getFieldId(field, nameMapping, fieldNames()); + if (fieldId != null) { + // always copy because fields can't be reused + newFields.add(copyField(field, newSchema, fieldId)); + } + } + } + + return copyRecord(record, newFields); + } + + @Override + public Schema union(Schema union, List<Schema> options) { + if (options.equals(union.getTypes())) { + return union; + } + + List<Schema> validOptions = + options.stream().filter(Objects::nonNull).collect(Collectors.toList()); + + return copyProps(union, Schema.createUnion(validOptions)); + } + + @Override + public Schema array(Schema array, Schema element) { + if (array.getLogicalType() instanceof LogicalMap + || (isKeyValueMapping(fieldNames()) + && AvroSchemaUtil.isKeyValueSchema(array.getElementType()))) { + return copyProps(array, Schema.createArray(element)); + } + + Integer elementId = AvroSchemaUtil.elementId(array); + if (elementId != null) { + if (array.getElementType().equals(element)) { + return array; + } + + return copyProps(array, createArray(element, elementId)); + } + + MappedField mapping = nameMapping.find(fieldNames(), "element"); + if (mapping != null) { + return copyProps(array, createArray(element, mapping.id())); + } + + return null; + } + + private boolean isKeyValueMapping(Iterable<String> names) { + return nameMapping.find(names, "key") != null && nameMapping.find(names, "value") != null; Review Comment: Should we move these `key` and `value` to constants? We also use them in the `map(` below. ########## core/src/main/java/org/apache/iceberg/avro/ApplyNameMapping.java: ########## @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.avro; + +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; +import org.apache.avro.LogicalType; +import org.apache.avro.Schema; +import org.apache.iceberg.mapping.MappedField; +import org.apache.iceberg.mapping.NameMapping; +import org.apache.iceberg.relocated.com.google.common.collect.Lists; + +/** + * An Avro Schema visitor to apply a name mapping to add Iceberg field IDs. + * + * <p>Methods return null when a schema has no ID and cannot be projected. + */ +public class ApplyNameMapping extends AvroSchemaVisitor<Schema> { + private final NameMapping nameMapping; + + public ApplyNameMapping(NameMapping nameMapping) { + this.nameMapping = nameMapping; + } + + @Override + public Schema record(Schema record, List<String> names, List<Schema> fields) { + List<Schema.Field> originalFields = record.getFields(); + + List<Schema.Field> newFields = Lists.newArrayList(); + for (int i = 0; i < originalFields.size(); i += 1) { + Schema newSchema = fields.get(i); + if (newSchema != null) { + Schema.Field field = originalFields.get(i); + Integer fieldId = AvroSchemaUtil.getFieldId(field, nameMapping, fieldNames()); + if (fieldId != null) { + // always copy because fields can't be reused + newFields.add(copyField(field, newSchema, fieldId)); + } + } + } + + return copyRecord(record, newFields); + } + + @Override + public Schema union(Schema union, List<Schema> options) { + if (options.equals(union.getTypes())) { + return union; + } + + List<Schema> validOptions = + options.stream().filter(Objects::nonNull).collect(Collectors.toList()); + + return copyProps(union, Schema.createUnion(validOptions)); + } + + @Override + public Schema array(Schema array, Schema element) { + if (array.getLogicalType() instanceof LogicalMap + || (isKeyValueMapping(fieldNames()) + && AvroSchemaUtil.isKeyValueSchema(array.getElementType()))) { + return copyProps(array, Schema.createArray(element)); + } + + Integer elementId = AvroSchemaUtil.elementId(array); + if (elementId != null) { + if (array.getElementType().equals(element)) { + return array; + } + + return copyProps(array, createArray(element, elementId)); + } + + MappedField mapping = nameMapping.find(fieldNames(), "element"); + if (mapping != null) { + return copyProps(array, createArray(element, mapping.id())); + } + + return null; + } + + private boolean isKeyValueMapping(Iterable<String> names) { + return nameMapping.find(names, "key") != null && nameMapping.find(names, "value") != null; + } + + @Override + public Schema map(Schema map, Schema value) { + Integer keyId = AvroSchemaUtil.keyId(map); + Integer valueId = AvroSchemaUtil.valueId(map); + if (keyId != null && valueId != null) { + if (map.getValueType().equals(value)) { + return map; + } + + return copyProps(map, createMap(value, keyId, valueId)); + } Review Comment: What do you think of: ```suggestion if (isKeyValueMapping(fieldNames())) { return copyProps( map, createMap(value, AvroSchemaUtil.getKeyId(map), AvroSchemaUtil.getValueId(map))); } ``` This way we re-use `isKeyValueMapping`, and we can get rid of the `keyId` and `valueId` methods. ########## .palantir/revapi.yml: ########## @@ -874,6 +874,10 @@ acceptedBreaks: justification: "Static utility class - should not have public constructor" "1.4.0": org.apache.iceberg:iceberg-core: + - code: "java.class.defaultSerializationChanged" + old: "class org.apache.iceberg.mapping.NameMapping" + new: "class org.apache.iceberg.mapping.NameMapping" + justification: "Serialization across verisons is not guaranteed" Review Comment: ```suggestion justification: "Serialization across versions is not guaranteed" ``` ########## core/src/main/java/org/apache/iceberg/avro/ApplyNameMapping.java: ########## @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.avro; + +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; +import org.apache.avro.LogicalType; +import org.apache.avro.Schema; +import org.apache.iceberg.mapping.MappedField; +import org.apache.iceberg.mapping.NameMapping; +import org.apache.iceberg.relocated.com.google.common.collect.Lists; + +/** + * An Avro Schema visitor to apply a name mapping to add Iceberg field IDs. + * Review Comment: ```suggestion * An Avro Schema visitor to apply a name mapping to add Iceberg field IDs. * * <p> Fields that are not present in the name mapping will be excluded. * ``` ########## core/src/main/java/org/apache/iceberg/avro/ApplyNameMapping.java: ########## @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.avro; + +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; +import org.apache.avro.LogicalType; +import org.apache.avro.Schema; +import org.apache.iceberg.mapping.MappedField; +import org.apache.iceberg.mapping.NameMapping; +import org.apache.iceberg.relocated.com.google.common.collect.Lists; + +/** + * An Avro Schema visitor to apply a name mapping to add Iceberg field IDs. + * + * <p>Methods return null when a schema has no ID and cannot be projected. + */ +public class ApplyNameMapping extends AvroSchemaVisitor<Schema> { + private final NameMapping nameMapping; + + public ApplyNameMapping(NameMapping nameMapping) { + this.nameMapping = nameMapping; + } + + @Override + public Schema record(Schema record, List<String> names, List<Schema> fields) { + List<Schema.Field> originalFields = record.getFields(); + + List<Schema.Field> newFields = Lists.newArrayList(); + for (int i = 0; i < originalFields.size(); i += 1) { + Schema newSchema = fields.get(i); + if (newSchema != null) { + Schema.Field field = originalFields.get(i); + Integer fieldId = AvroSchemaUtil.getFieldId(field, nameMapping, fieldNames()); Review Comment: The `fieldNames()` is a handy method, and might be interesting for Python as well. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
