szehon-ho commented on code in PR #17119:
URL: https://github.com/apache/iceberg/pull/17119#discussion_r3581784894
##########
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:
🟢 **Nit** — The local `wkbPoint` wrapper is fine (matches
`TestParquetDataWriter`), but you could inline
`ByteBuffer.wrap(RandomUtil.wkbPoint(...))` the way
`TestParquet.testGeospatialWkbRoundTrip` does and drop the helper. Optional
either way.
##########
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:
🟢 **Nit** — `GEOMETRY`/`GEOGRAPHY` could share the `BINARY` case block
instead of a separate block with a comment:
```java
case BINARY:
case GEOMETRY:
case GEOGRAPHY:
primitiveSchema = BINARY_SCHEMA;
break;
```
The spec mapping is already documented in the PR description; this matches
how `RandomInternalData` groups these types and avoids a comment that mostly
restates the code.
--
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]
