uros-b commented on code in PR #17493:
URL: https://github.com/apache/iceberg/pull/17493#discussion_r3705154996
##########
core/src/test/java/org/apache/iceberg/TestContentStatsBackedMap.java:
##########
@@ -99,6 +100,124 @@ public void testUpperBounds() {
Map.entry(3, Conversions.toByteBuffer(Types.DoubleType.get(),
9.0)));
}
+ @Test
+ public void testGeoBoundsUseSinglePointEncoding() {
+ Schema geoSchema =
+ new Schema(
+ required(1, "id", Types.LongType.get()),
+ optional(10, "geom", Types.GeometryType.crs84()),
+ optional(11, "geog", Types.GeographyType.crs84()));
+ Types.StructType statsType = StatsUtil.statsReadSchema(geoSchema,
List.of(1, 10, 11));
+
+ ContentStatsStruct stats = new ContentStatsStruct(statsType);
+ stats.setStats(
+ 1,
+ StatsTestUtil.mockFieldStats(
+ statsType.field("id").type().asStructType(), 1, 1L, 5L, 26L, null,
null));
+ stats.setStats(
+ 10,
+ StatsTestUtil.mockFieldStats(
+ statsType.field("geom").type().asStructType(),
+ 10,
+ TestHelpers.Row.of(1.0, 2.0, null, null),
+ TestHelpers.Row.of(5.0, 6.0, null, null),
+ 26L,
+ 2L,
+ null));
+ stats.setStats(
+ 11,
+ StatsTestUtil.mockFieldStats(
+ statsType.field("geog").type().asStructType(),
+ 11,
+ TestHelpers.Row.of(-1.0, -2.0, 3.0, 4.0),
+ TestHelpers.Row.of(7.0, 8.0, 9.0, 10.0),
+ 26L,
+ 0L,
+ null));
+
+ // geometry and geography bounds are stored as bounding-box structs (x, y,
z, m) but must be
+ // presented in the legacy maps using the spec's single-point encoding
+ Map<Integer, ByteBuffer> lower = ContentStatsBackedMap.lowerBounds(stats);
+ assertThat(lower)
+ .containsOnly(
+ Map.entry(1, Conversions.toByteBuffer(Types.LongType.get(), 1L)),
+ Map.entry(10, GeospatialBound.createXY(1.0, 2.0).toByteBuffer()),
+ Map.entry(11, GeospatialBound.createXYZM(-1.0, -2.0, 3.0,
4.0).toByteBuffer()));
+
+ Map<Integer, ByteBuffer> upper = ContentStatsBackedMap.upperBounds(stats);
+ assertThat(upper)
+ .containsOnly(
+ Map.entry(1, Conversions.toByteBuffer(Types.LongType.get(), 5L)),
+ Map.entry(10, GeospatialBound.createXY(5.0, 6.0).toByteBuffer()),
+ Map.entry(11, GeospatialBound.createXYZM(7.0, 8.0, 9.0,
10.0).toByteBuffer()));
+
+ // the encoding must round-trip through the geo conversion used by legacy
readers
Review Comment:
The Conversions.fromByteBuffer round-trip assertion covers only the XY
(geometry lower) and XYZM (geography upper) variants; the XYZ (24-byte) and XYM
(32-byte, NaN z-slot) encodings are asserted only at raw ByteBuffer
byte-equality via .isEqualTo(GeospatialBound.createXYZ(...).toByteBuffer()),
not round-tripped back through Conversions.fromByteBuffer.
GeospatialBound.toByteBuffer/fromByteBuffer are separately tested so the risk
is low, but XYM is the encoding most likely to harbor a NaN-slot/endianness
edge case and is the exact integration point the fix depends on; adding
fromByteBuffer round-trips for XYZ and XYM would close the integration coverage
gap. This goes for testGeoBoundsUseSinglePointEncoding,
testGeoBoundWithZOnlyAndMOnly, etc.
##########
core/src/test/java/org/apache/iceberg/TestContentStatsBackedMap.java:
##########
@@ -99,6 +100,124 @@ public void testUpperBounds() {
Map.entry(3, Conversions.toByteBuffer(Types.DoubleType.get(),
9.0)));
}
+ @Test
+ public void testGeoBoundsUseSinglePointEncoding() {
+ Schema geoSchema =
+ new Schema(
+ required(1, "id", Types.LongType.get()),
+ optional(10, "geom", Types.GeometryType.crs84()),
+ optional(11, "geog", Types.GeographyType.crs84()));
+ Types.StructType statsType = StatsUtil.statsReadSchema(geoSchema,
List.of(1, 10, 11));
+
+ ContentStatsStruct stats = new ContentStatsStruct(statsType);
+ stats.setStats(
+ 1,
+ StatsTestUtil.mockFieldStats(
+ statsType.field("id").type().asStructType(), 1, 1L, 5L, 26L, null,
null));
+ stats.setStats(
+ 10,
+ StatsTestUtil.mockFieldStats(
+ statsType.field("geom").type().asStructType(),
+ 10,
+ TestHelpers.Row.of(1.0, 2.0, null, null),
+ TestHelpers.Row.of(5.0, 6.0, null, null),
+ 26L,
+ 2L,
+ null));
+ stats.setStats(
+ 11,
+ StatsTestUtil.mockFieldStats(
+ statsType.field("geog").type().asStructType(),
+ 11,
+ TestHelpers.Row.of(-1.0, -2.0, 3.0, 4.0),
+ TestHelpers.Row.of(7.0, 8.0, 9.0, 10.0),
+ 26L,
+ 0L,
+ null));
+
+ // geometry and geography bounds are stored as bounding-box structs (x, y,
z, m) but must be
+ // presented in the legacy maps using the spec's single-point encoding
+ Map<Integer, ByteBuffer> lower = ContentStatsBackedMap.lowerBounds(stats);
+ assertThat(lower)
+ .containsOnly(
+ Map.entry(1, Conversions.toByteBuffer(Types.LongType.get(), 1L)),
+ Map.entry(10, GeospatialBound.createXY(1.0, 2.0).toByteBuffer()),
+ Map.entry(11, GeospatialBound.createXYZM(-1.0, -2.0, 3.0,
4.0).toByteBuffer()));
+
+ Map<Integer, ByteBuffer> upper = ContentStatsBackedMap.upperBounds(stats);
+ assertThat(upper)
+ .containsOnly(
+ Map.entry(1, Conversions.toByteBuffer(Types.LongType.get(), 5L)),
+ Map.entry(10, GeospatialBound.createXY(5.0, 6.0).toByteBuffer()),
+ Map.entry(11, GeospatialBound.createXYZM(7.0, 8.0, 9.0,
10.0).toByteBuffer()));
+
+ // the encoding must round-trip through the geo conversion used by legacy
readers
Review Comment:
also cc @szehon-ho
--
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]
