huan233usc commented on code in PR #17119:
URL: https://github.com/apache/iceberg/pull/17119#discussion_r3582656374
##########
core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java:
##########
@@ -264,6 +264,11 @@ public Schema primitive(Type.PrimitiveType primitive) {
case BINARY:
primitiveSchema = BINARY_SCHEMA;
break;
+ case GEOMETRY:
+ case GEOGRAPHY:
Review Comment:
Done in 64993fe2c — `GEOMETRY`/`GEOGRAPHY` now share the `BINARY` case block
and the restating comment is dropped. Thanks!
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroDataWriter.java:
##########
@@ -110,4 +112,56 @@ public void testDataWriter() throws IOException {
assertThat(writtenRecords).as("Written records should
match").isEqualTo(records);
}
+
+ @Test
+ public void testGeospatialWkbRoundTrip() throws IOException {
+ Schema schema =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "geom", Types.GeometryType.crs84()),
+ Types.NestedField.optional(3, "geog",
Types.GeographyType.crs84()));
+
+ GenericRecord record = GenericRecord.create(schema);
+ List<Record> geoRecords =
+ ImmutableList.of(
+ record.copy(
+ ImmutableMap.of("id", 1L, "geom", wkbPoint(30, 10), "geog",
wkbPoint(-5, 40))),
+ // geog is left null
+ record.copy(ImmutableMap.of("id", 2L, "geom", wkbPoint(0, 0))),
+ // both geo columns are left null
+ record.copy(ImmutableMap.of("id", 3L)));
+
+ OutputFile file = Files.localOutput(temp.toFile());
+ DataWriter<Record> dataWriter =
+ Avro.writeData(file)
+ .schema(schema)
+ .createWriterFunc(org.apache.iceberg.data.avro.DataWriter::create)
+ .overwrite()
+ .withSpec(PartitionSpec.unpartitioned())
+ .build();
+ try (dataWriter) {
+ for (Record geoRecord : geoRecords) {
+ dataWriter.write(geoRecord);
+ }
+ }
+
+
assertThat(dataWriter.toDataFile().recordCount()).isEqualTo(geoRecords.size());
+
+ List<Record> writtenRecords;
+ try (AvroIterable<Record> reader =
+ Avro.read(file.toInputFile())
+ .project(schema)
+ .createResolvingReader(PlannedDataReader::create)
+ .build()) {
+ writtenRecords = Lists.newArrayList(reader);
+ }
+
+ assertThat(writtenRecords)
+ .as("Geometry and geography WKB should round-trip through Avro")
+ .isEqualTo(geoRecords);
+ }
+
+ private static ByteBuffer wkbPoint(double xCoord, double yCoord) {
Review Comment:
Leaving the local `wkbPoint` helper as-is: it delegates to
`RandomUtil.wkbPoint` and matches the merged `TestParquetDataWriter.wkbPoint`
precedent (the same one-place-for-WKB consistency wombatu-kun pointed to
above), and it keeps the three call sites in the record list readable. Happy to
inline if you feel strongly.
--
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]
