You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "huan233usc (via GitHub)" <gi...@apache.org> on 2026/05/29 06:40:05 UTC
[PR] API: Single-value binary serialization for geometry and geography [iceberg]
huan233usc opened a new pull request, #16607:
URL: https://github.com/apache/iceberg/pull/16607
## Summary
Hook `GeospatialBound` into `Conversions.toByteBuffer` / `fromByteBuffer` so that v3 single-value binary serialization for `geometry` and `geography` actually round-trips through Iceberg's central type conversion API.
The encoding itself is already implemented in `GeospatialBound` per the [Bound Serialization](https://iceberg.apache.org/spec/#bound-serialization) section of the spec (Appendix D): an x:y:z:m concatenation of 8-byte little-endian IEEE 754 doubles, with x:y when both z and m are unset, x:y:z when only m is unset, and x:y:NaN:m when only z is unset.
Today `Conversions.toByteBuffer(GeometryType, ...)` and `fromByteBuffer(GeometryType, ...)` fall through to the `default` case and throw `UnsupportedOperationException`, so any caller that goes through `Conversions` (manifest writers/readers, `ContentFileParser`, metric helpers, etc.) cannot handle geo lower/upper bounds. This PR adds the missing cases. It is purely additive — no behavior changes for non-geo types and no downstream code is silently activated, since the file readers/writers and evaluators still need their own follow-ups.
## Test plan
- [x] New `testByteBufferConversionsForGeometry` covers `geometry` and `geography` × {XY (16B), XYZ (24B), XYM (32B with NaN-Z slot), XYZM (32B)} round-trips with explicit byte layouts, plus a non-default CRS to confirm the encoding is CRS-independent.
- [x] New `testNullByteBufferConversionsForGeometry` covers null in both directions for both types.
- [x] `./gradlew :iceberg-api:test --tests "org.apache.iceberg.types.*" --tests "org.apache.iceberg.geospatial.*"` — all green, no regressions in `TestGeospatialBound` / `TestBoundingBox`.
- [x] `./gradlew :iceberg-api:checkstyleMain :iceberg-api:checkstyleTest :iceberg-api:spotlessCheck` — clean.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Single-value binary serialization for geometry and geography [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #16607:
URL: https://github.com/apache/iceberg/pull/16607#discussion_r3392299617
##########
api/src/test/java/org/apache/iceberg/types/TestConversions.java:
##########
@@ -191,6 +194,70 @@ public void testByteBufferConversions() {
.isEqualTo(new byte[] {11});
}
+ @Test
+ public void testByteBufferConversionsForGeometry() {
+ // geometry and geography lower/upper bounds are points encoded as an x:y:z:m concatenation
+ // of 8-byte little-endian IEEE 754 doubles. x and y are mandatory; the encoding is x:y when
+ // both z and m are unset, x:y:z when only m is unset, and x:y:NaN:m when only z is unset.
+
+ // x=10.0 -> 0, 0, 0, 0, 0, 0, 36, 64 (little-endian IEEE 754)
+ // y=13.0 -> 0, 0, 0, 0, 0, 0, 42, 64
+ // z=15.0 -> 0, 0, 0, 0, 0, 0, 46, 64
+ // m=20.0 -> 0, 0, 0, 0, 0, 0, 52, 64
+ // NaN -> 0, 0, 0, 0, 0, 0, -8, 127
+
+ byte[] xyBytes = new byte[] {0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 42, 64};
+ byte[] xyzBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64
+ };
+ byte[] xymBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, -8, 127,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+ byte[] xyzmBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeometryType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeometryType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeometryType.crs84(), xyzmBytes);
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeographyType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeographyType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeographyType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeographyType.crs84(), xyzmBytes);
+
+ // a non-default CRS must not change the binary encoding
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.of("EPSG:3857"), xyBytes);
+ }
+
+ @Test
+ public void testNullByteBufferConversionsForGeometry() {
+ // Null is handled by the shared guards in toByteBuffer / fromByteBuffer before the type
+ // switch, so this does not exercise the GEOMETRY / GEOGRAPHY arms (those are covered by
+ // testByteBufferConversionsForGeometry). It pins the null contract for geo types so a future
+ // change to the guards cannot silently alter null handling.
+ assertThat(Conversions.toByteBuffer(GeometryType.crs84(), null)).isNull();
+ assertThat(Conversions.toByteBuffer(GeographyType.crs84(), null)).isNull();
+ GeospatialBound geometryBound = Conversions.fromByteBuffer(GeometryType.crs84(), null);
+ GeospatialBound geographyBound = Conversions.fromByteBuffer(GeographyType.crs84(), null);
+ assertThat(geometryBound).isNull();
+ assertThat(geographyBound).isNull();
+ }
Review Comment:
suggestion to make it easier to read
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Single-value binary serialization for geometry and geography [iceberg]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #16607:
URL: https://github.com/apache/iceberg/pull/16607#discussion_r3375412201
##########
api/src/test/java/org/apache/iceberg/types/TestConversions.java:
##########
@@ -191,6 +194,66 @@ public void testByteBufferConversions() {
.isEqualTo(new byte[] {11});
}
+ @Test
+ public void testByteBufferConversionsForGeometry() {
+ // geometry and geography lower/upper bounds are points encoded as an x:y:z:m concatenation
+ // of 8-byte little-endian IEEE 754 doubles. x and y are mandatory; the encoding is x:y when
+ // both z and m are unset, x:y:z when only m is unset, and x:y:NaN:m when only z is unset.
+
+ // x=10.0 -> 0, 0, 0, 0, 0, 0, 36, 64 (little-endian IEEE 754)
+ // y=13.0 -> 0, 0, 0, 0, 0, 0, 42, 64
+ // z=15.0 -> 0, 0, 0, 0, 0, 0, 46, 64
+ // m=20.0 -> 0, 0, 0, 0, 0, 0, 52, 64
+ // NaN -> 0, 0, 0, 0, 0, 0, -8, 127
+
+ byte[] xyBytes = new byte[] {0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 42, 64};
+ byte[] xyzBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64
+ };
+ byte[] xymBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, -8, 127,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+ byte[] xyzmBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeometryType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeometryType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeometryType.crs84(), xyzmBytes);
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeographyType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeographyType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeographyType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeographyType.crs84(), xyzmBytes);
+
+ // a non-default CRS must not change the binary encoding
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.of("EPSG:3857"), xyBytes);
+ }
+
+ @Test
+ public void testNullByteBufferConversionsForGeometry() {
Review Comment:
Good catch — you're right that null short-circuits at the shared guards before the type switch, so this test doesn't exercise the new GEOMETRY/GEOGRAPHY arms; those are covered by the non-null round-trips in `testByteBufferConversionsForGeometry`. I kept this test to pin the null contract for geo types (so a future change to the guards can't silently alter null handling) and added a comment making that intent explicit.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Single-value binary serialization for geometry and geography [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #16607:
URL: https://github.com/apache/iceberg/pull/16607#discussion_r3392294306
##########
api/src/test/java/org/apache/iceberg/types/TestConversions.java:
##########
@@ -191,6 +194,70 @@ public void testByteBufferConversions() {
.isEqualTo(new byte[] {11});
}
+ @Test
+ public void testByteBufferConversionsForGeometry() {
+ // geometry and geography lower/upper bounds are points encoded as an x:y:z:m concatenation
+ // of 8-byte little-endian IEEE 754 doubles. x and y are mandatory; the encoding is x:y when
+ // both z and m are unset, x:y:z when only m is unset, and x:y:NaN:m when only z is unset.
+
+ // x=10.0 -> 0, 0, 0, 0, 0, 0, 36, 64 (little-endian IEEE 754)
+ // y=13.0 -> 0, 0, 0, 0, 0, 0, 42, 64
+ // z=15.0 -> 0, 0, 0, 0, 0, 0, 46, 64
+ // m=20.0 -> 0, 0, 0, 0, 0, 0, 52, 64
+ // NaN -> 0, 0, 0, 0, 0, 0, -8, 127
+
+ byte[] xyBytes = new byte[] {0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 42, 64};
+ byte[] xyzBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64
+ };
+ byte[] xymBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, -8, 127,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+ byte[] xyzmBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeometryType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeometryType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeometryType.crs84(), xyzmBytes);
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeographyType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeographyType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeographyType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeographyType.crs84(), xyzmBytes);
+
+ // a non-default CRS must not change the binary encoding
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.of("EPSG:3857"), xyBytes);
+ }
+
+ @Test
+ public void testNullByteBufferConversionsForGeometry() {
+ // Null is handled by the shared guards in toByteBuffer / fromByteBuffer before the type
+ // switch, so this does not exercise the GEOMETRY / GEOGRAPHY arms (those are covered by
+ // testByteBufferConversionsForGeometry). It pins the null contract for geo types so a future
+ // change to the guards cannot silently alter null handling.
+ assertThat(Conversions.toByteBuffer(GeometryType.crs84(), null)).isNull();
+ assertThat(Conversions.toByteBuffer(GeographyType.crs84(), null)).isNull();
+ GeospatialBound geometryBound = Conversions.fromByteBuffer(GeometryType.crs84(), null);
+ GeospatialBound geographyBound = Conversions.fromByteBuffer(GeographyType.crs84(), null);
+ assertThat(geometryBound).isNull();
+ assertThat(geographyBound).isNull();
+ }
Review Comment:
```suggestion
@Test
public void testByteBufferConversionsForGeometry() {
// geometry/geography bounds are a single point encoded as 8-byte little-endian IEEE 754
// doubles in X, Y[, Z][, M] order:
// x:y (16B) when z and m are unset
// x:y:z (24B) when only m is unset
// x:y:NaN:m (32B) when only z is unset -- z slot is filled with NaN
// x:y:z:m (32B) when all are set
byte[] x = {0, 0, 0, 0, 0, 0, 36, 64}; // 10.0
byte[] y = {0, 0, 0, 0, 0, 0, 42, 64}; // 13.0
byte[] z = {0, 0, 0, 0, 0, 0, 46, 64}; // 15.0
byte[] m = {0, 0, 0, 0, 0, 0, 52, 64}; // 20.0
byte[] nan = {0, 0, 0, 0, 0, 0, -8, 127};
for (Type type : ImmutableList.of(GeometryType.crs84(), GeographyType.crs84())) {
assertConversion(GeospatialBound.createXY(10.0, 13.0), type, Bytes.concat(x, y));
assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), type, Bytes.concat(x, y, z));
assertConversion(
GeospatialBound.createXYM(10.0, 13.0, 20.0), type, Bytes.concat(x, y, nan, m));
assertConversion(
GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), type, Bytes.concat(x, y, z, m));
}
// a non-default CRS must not change the binary encoding
assertConversion(
GeospatialBound.createXY(10.0, 13.0), GeometryType.of("EPSG:3857"), Bytes.concat(x, y));
}
@Test
public void testNullByteBufferConversionsForGeometry() {
// Null is handled by the shared guards in toByteBuffer / fromByteBuffer before the type switch,
// so this pins the null contract for geo types rather than exercising the GEOMETRY / GEOGRAPHY
// arms (those are covered by testByteBufferConversionsForGeometry).
assertThat(Conversions.toByteBuffer(GeometryType.crs84(), null)).isNull();
assertThat(Conversions.toByteBuffer(GeographyType.crs84(), null)).isNull();
assertThat((Object) Conversions.fromByteBuffer(GeometryType.crs84(), null)).isNull();
assertThat((Object) Conversions.fromByteBuffer(GeographyType.crs84(), null)).isNull();
}
```
##########
api/src/test/java/org/apache/iceberg/types/TestConversions.java:
##########
@@ -26,13 +26,16 @@
import java.nio.charset.StandardCharsets;
import java.util.UUID;
import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.geospatial.GeospatialBound;
Review Comment:
```suggestion
import org.apache.iceberg.geospatial.GeospatialBound;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.primitives.Bytes;
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Single-value binary serialization for geometry and geography [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #16607:
URL: https://github.com/apache/iceberg/pull/16607#issuecomment-4686053664
Merged, thanks @huan233usc , and @wombatu-kun for additional review
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Single-value binary serialization for geometry and geography [iceberg]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #16607:
URL: https://github.com/apache/iceberg/pull/16607#discussion_r3392938503
##########
api/src/test/java/org/apache/iceberg/types/TestConversions.java:
##########
@@ -191,6 +194,70 @@ public void testByteBufferConversions() {
.isEqualTo(new byte[] {11});
}
+ @Test
+ public void testByteBufferConversionsForGeometry() {
+ // geometry and geography lower/upper bounds are points encoded as an x:y:z:m concatenation
+ // of 8-byte little-endian IEEE 754 doubles. x and y are mandatory; the encoding is x:y when
+ // both z and m are unset, x:y:z when only m is unset, and x:y:NaN:m when only z is unset.
+
+ // x=10.0 -> 0, 0, 0, 0, 0, 0, 36, 64 (little-endian IEEE 754)
+ // y=13.0 -> 0, 0, 0, 0, 0, 0, 42, 64
+ // z=15.0 -> 0, 0, 0, 0, 0, 0, 46, 64
+ // m=20.0 -> 0, 0, 0, 0, 0, 0, 52, 64
+ // NaN -> 0, 0, 0, 0, 0, 0, -8, 127
+
+ byte[] xyBytes = new byte[] {0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 42, 64};
+ byte[] xyzBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64
+ };
+ byte[] xymBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, -8, 127,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+ byte[] xyzmBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeometryType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeometryType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeometryType.crs84(), xyzmBytes);
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeographyType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeographyType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeographyType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeographyType.crs84(), xyzmBytes);
+
+ // a non-default CRS must not change the binary encoding
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.of("EPSG:3857"), xyBytes);
+ }
+
+ @Test
+ public void testNullByteBufferConversionsForGeometry() {
+ // Null is handled by the shared guards in toByteBuffer / fromByteBuffer before the type
+ // switch, so this does not exercise the GEOMETRY / GEOGRAPHY arms (those are covered by
+ // testByteBufferConversionsForGeometry). It pins the null contract for geo types so a future
+ // change to the guards cannot silently alter null handling.
+ assertThat(Conversions.toByteBuffer(GeometryType.crs84(), null)).isNull();
+ assertThat(Conversions.toByteBuffer(GeographyType.crs84(), null)).isNull();
+ GeospatialBound geometryBound = Conversions.fromByteBuffer(GeometryType.crs84(), null);
+ GeospatialBound geographyBound = Conversions.fromByteBuffer(GeographyType.crs84(), null);
+ assertThat(geometryBound).isNull();
+ assertThat(geographyBound).isNull();
+ }
Review Comment:
Thanks, applied in 72bab21ab — named the per-axis little-endian constants (x, y, z, m, nan), build each golden layout with `Bytes.concat` so the on-disk bytes stay explicit while the X/Y/Z/M structure is visible, and loop over geometry/geography.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Single-value binary serialization for geometry and geography [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #16607:
URL: https://github.com/apache/iceberg/pull/16607#discussion_r3392294299
##########
api/src/test/java/org/apache/iceberg/types/TestConversions.java:
##########
@@ -26,13 +26,16 @@
import java.nio.charset.StandardCharsets;
import java.util.UUID;
import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.geospatial.GeospatialBound;
Review Comment:
```suggestion
import org.apache.iceberg.geospatial.GeospatialBound;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.primitives.Bytes;
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Single-value binary serialization for geometry and geography [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho merged PR #16607:
URL: https://github.com/apache/iceberg/pull/16607
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Single-value binary serialization for geometry and geography [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16607:
URL: https://github.com/apache/iceberg/pull/16607#discussion_r3371697147
##########
api/src/test/java/org/apache/iceberg/types/TestConversions.java:
##########
@@ -191,6 +194,66 @@ public void testByteBufferConversions() {
.isEqualTo(new byte[] {11});
}
+ @Test
+ public void testByteBufferConversionsForGeometry() {
+ // geometry and geography lower/upper bounds are points encoded as an x:y:z:m concatenation
+ // of 8-byte little-endian IEEE 754 doubles. x and y are mandatory; the encoding is x:y when
+ // both z and m are unset, x:y:z when only m is unset, and x:y:NaN:m when only z is unset.
+
+ // x=10.0 -> 0, 0, 0, 0, 0, 0, 36, 64 (little-endian IEEE 754)
+ // y=13.0 -> 0, 0, 0, 0, 0, 0, 42, 64
+ // z=15.0 -> 0, 0, 0, 0, 0, 0, 46, 64
+ // m=20.0 -> 0, 0, 0, 0, 0, 0, 52, 64
+ // NaN -> 0, 0, 0, 0, 0, 0, -8, 127
+
+ byte[] xyBytes = new byte[] {0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 42, 64};
+ byte[] xyzBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64
+ };
+ byte[] xymBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, -8, 127,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+ byte[] xyzmBytes =
+ new byte[] {
+ 0, 0, 0, 0, 0, 0, 36, 64,
+ 0, 0, 0, 0, 0, 0, 42, 64,
+ 0, 0, 0, 0, 0, 0, 46, 64,
+ 0, 0, 0, 0, 0, 0, 52, 64
+ };
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeometryType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeometryType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeometryType.crs84(), xyzmBytes);
+
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeographyType.crs84(), xyBytes);
+ assertConversion(GeospatialBound.createXYZ(10.0, 13.0, 15.0), GeographyType.crs84(), xyzBytes);
+ assertConversion(GeospatialBound.createXYM(10.0, 13.0, 20.0), GeographyType.crs84(), xymBytes);
+ assertConversion(
+ GeospatialBound.createXYZM(10.0, 13.0, 15.0, 20.0), GeographyType.crs84(), xyzmBytes);
+
+ // a non-default CRS must not change the binary encoding
+ assertConversion(GeospatialBound.createXY(10.0, 13.0), GeometryType.of("EPSG:3857"), xyBytes);
+ }
+
+ @Test
+ public void testNullByteBufferConversionsForGeometry() {
Review Comment:
`toByteBuffer` and `fromByteBuffer` return null on the shared guard (Conversions.java:90-92 and 148-150) before reaching the GEOMETRY/GEOGRAPHY cases, so this test passes even without the new branches. It still pins the geo null contract, but note it does not cover the added code - the non-null round-trips in testByteBufferConversionsForGeometry are what exercise the new arms.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org