wombatu-kun commented on code in PR #17119:
URL: https://github.com/apache/iceberg/pull/17119#discussion_r3565422074
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -330,6 +330,39 @@ public void testGeospatialFooterMetricsSkipParquetBounds()
throws IOException {
}
}
+ @Test
+ public void testGeospatialWkbRoundTrip() throws IOException {
Review Comment:
The test plan in the PR body still describes the previous commit: it lists
only the `supportsGeospatial()` flips plus the RandomAvroData/AvroTestHelpers
cases, and does not mention TestAvroEncoderUtil,
TestAvroDataWriter.testGeospatialWkbRoundTrip, or this test - so a PR titled
`Core:` silently adds parquet-module coverage. Worth saying in the description
(it becomes the squash commit message) that this closes the parquet-avro object
model gap (`ParquetAvroWriter` / `ParquetAvroValueReaders`), which was
unreachable for geo because `AvroSchemaUtil.convert` threw on geo types - the
follow-up szehon-ho asked about on #16982.
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroDataWriter.java:
##########
@@ -110,4 +112,64 @@ 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:
`RandomUtil.wkbPoint(double, double)` in
api/src/test/java/org/apache/iceberg/util/RandomUtil.java already encodes a
little-endian WKB point, and it is on core's test classpath (`RandomAvroData`
imports it). szehon-ho raised this exact duplication on the sibling Parquet PR
#16982, and the merged `TestParquetDataWriter.wkbPoint` delegates: `return
ByteBuffer.wrap(RandomUtil.wkbPoint(xCoord, yCoord));`. Doing the same here
keeps WKB encoding in one place and drops the new `java.nio.ByteOrder` import.
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -330,6 +330,39 @@ public void testGeospatialFooterMetricsSkipParquetBounds()
throws IOException {
}
}
+ @Test
+ public void testGeospatialWkbRoundTrip() throws IOException {
+ Schema schema =
+ new Schema(
+ optional(1, "geom", Types.GeometryType.crs84()),
+ optional(2, "geog", Types.GeographyType.crs84()));
+
+ ByteBuffer geomWkb = ByteBuffer.wrap(new byte[] {0x01, 0x02, 0x03});
Review Comment:
These bytes are not WKB. That is harmless in
`testGeospatialFooterMetricsSkipParquetBounds`, which writes them through a
`Types.BinaryType` schema, but this test writes them through the
geometry/geography schema, so `TypeToMessageType` annotates the column with
`LogicalTypeAnnotation.geometryType(crs)`. Parquet then attaches a real
`GeospatialStatistics.Builder` to that column (`ColumnValueCollector`), and its
`update(Binary)` runs JTS `WKBReader.read` and catches `ParseException` with a
"Failed to parse WKB geometry, omit it from stats" warning. So the test passes
while writing a file whose geometry values are unparseable and whose geospatial
statistics are empty. `RandomUtil.wkbPoint(...)` produces real WKB here too and
makes the test name accurate.
--
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]
