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: [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]

Reply via email to