rdblue commented on a change in pull request #207: Add external schema mappings for files written with name-based schemas #40 URL: https://github.com/apache/incubator-iceberg/pull/207#discussion_r337276240
########## File path: core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java ########## @@ -0,0 +1,257 @@ +/* + * 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 com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; +import java.io.File; +import java.io.IOException; +import java.util.List; +import java.util.Map; +import org.apache.avro.file.DataFileWriter; +import org.apache.avro.generic.GenericData; +import org.apache.avro.generic.GenericDatumWriter; +import org.apache.avro.io.DatumWriter; +import org.apache.iceberg.AssertHelpers; +import org.apache.iceberg.Files; +import org.apache.iceberg.Schema; +import org.apache.iceberg.mapping.MappedField; +import org.apache.iceberg.mapping.MappedFields; +import org.apache.iceberg.mapping.MappingUtil; +import org.apache.iceberg.mapping.NameMapping; +import org.apache.iceberg.types.Types; +import org.junit.Assert; +import org.junit.Test; + +import static org.apache.avro.generic.GenericData.Record; + +public class TestAvroNameMapping extends TestAvroReadProjection { + @Test + public void testMapProjections() throws IOException { + Schema writeSchema = new Schema( + Types.NestedField.required(0, "id", Types.LongType.get()), + Types.NestedField.optional(5, "location", Types.MapType.ofOptional(6, 7, + Types.StringType.get(), + Types.StructType.of( + Types.NestedField.required(1, "lat", Types.FloatType.get()), + Types.NestedField.optional(2, "long", Types.FloatType.get()) + ) + ))); + + Record record = new Record(AvroSchemaUtil.convert(writeSchema, "table")); + record.put("id", 34L); + Record location = new Record(AvroSchemaUtil.fromOption( + AvroSchemaUtil.fromOption(record.getSchema().getField("location").schema()) + .getValueType())); + location.put("lat", 52.995143f); + location.put("long", -1.539054f); + record.put("location", ImmutableMap.of("l1", location)); + + // Table mapping does not project `location` map + NameMapping nameMapping = MappingUtil.create(new Schema( + Types.NestedField.required(0, "id", Types.LongType.get()))); + + Schema readSchema = writeSchema; + + Record projected = writeAndRead(writeSchema, readSchema, record, nameMapping); + // field id 5 comes from read schema + Assert.assertNotNull("Field missing from table mapping is renamed", projected.getSchema().getField("location_r5")); + Assert.assertNull("location field should not be read", projected.get("location_r5")); Review comment: Overall, tests look good. My remaining concern about this is that these names have changed and would break access by name like you use here. The schema has the same shape, but the requested names don't match. Updating the schema produced by `BuildAvroProjection` is what causes this. I'm wondering if we could do slightly better, by passing in a different schema when building the [resolving decoder](https://github.com/apache/incubator-iceberg/blob/master/data/src/main/java/org/apache/iceberg/data/avro/DataReader.java#L89-L95). We can do that fairly easy for iceberg-data generic classes and the Spark support since we can update the reader builder to do these renames just before creating the resolving decoder. Fixing Avro generics would be a bit more difficult, but not bad. Basically, we'd want to build a converter structure like the ones we use for Iceberg generics and Spark so we can use a different schema for the decoder but still create the records with the unmodified read schema. I think this would perform better than the normal path, too. I don't think we need to do that before getting this in, but it would be nice to fix that before the next release, at least for Iceberg generics. Could you open an issue to track it and then we can get this one in without modification? ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
