Copilot commented on code in PR #2303:
URL: https://github.com/apache/sedona/pull/2303#discussion_r2287109310
##########
common/src/main/java/org/apache/sedona/common/S2Geography/MultiPolygonGeography.java:
##########
@@ -44,4 +51,50 @@ public int dimension() {
// every child PolygonGeography
return 2;
}
+
+ @Override
+ public void encode(UnsafeOutput out, EncodeOptions opts) throws IOException {
+ out.writeInt(features.size());
+ for (Geography feature : features) {
+ ((PolygonGeography) feature).polygon.encode(out);
+ }
+ out.flush();
+ }
+
+ /** This is what decodeTagged() actually calls */
+ public static MultiPolygonGeography decode(Input in, EncodeTag tag) throws
IOException {
+ // cast to UnsafeInput—will work if you always pass a Kryo-backed stream
+ if (!(in instanceof UnsafeInput)) {
+ throw new IllegalArgumentException("Expected UnsafeInput");
+ }
+ return decode((UnsafeInput) in, tag);
Review Comment:
[nitpick] The type checking and casting pattern here duplicates logic found
in other Geography decode methods. Consider extracting this pattern to a
utility method or using a more polymorphic approach to reduce code duplication
across the codebase.
```suggestion
return decode(requireUnsafeInput(in), tag);
}
/**
* Utility method to ensure the input is an UnsafeInput and cast it.
*/
private static UnsafeInput requireUnsafeInput(Input in) {
if (!(in instanceof UnsafeInput)) {
throw new IllegalArgumentException("Expected UnsafeInput");
}
return (UnsafeInput) in;
```
##########
common/src/test/java/org/apache/sedona/common/S2Geography/PolygonGeographyTest.java:
##########
@@ -42,4 +46,42 @@ public void testEncodedPolygon() throws IOException {
TestHelper.assertRoundTrip(geo, new EncodeOptions());
}
+
+ @Test
+ public void testEncodeMultiPolygon() throws IOException {
+ S2Point pt = S2LatLng.fromDegrees(45, -64).toPoint();
+ S2Point pt_mid = S2LatLng.fromDegrees(45, 0).toPoint();
+ S2Point pt_end = S2LatLng.fromDegrees(0, 0).toPoint();
+ // Build a single polygon and wrap in geography
+ List<S2Point> points = new ArrayList<>();
+ points.add(pt);
+ points.add(pt_mid);
+ points.add(pt_end);
+ points.add(pt);
+ S2Loop polyline = new S2Loop(points);
+ S2Polygon poly = new S2Polygon(polyline);
+
+ S2Point pt2 = S2LatLng.fromDegrees(45, -64).toPoint();
+ S2Point pt_mid2 = S2LatLng.fromDegrees(45, 0).toPoint();
+ S2Point pt_end2 = S2LatLng.fromDegrees(0, 0).toPoint();
+ // Build a single polygon and wrap in geography
Review Comment:
The second polygon uses identical coordinates to the first polygon (lines
52-62). This creates duplicate test data that doesn't effectively test
MultiPolygon with distinct geometries. Consider using different coordinates for
the second polygon to create a more meaningful test case.
```suggestion
S2Point pt2 = S2LatLng.fromDegrees(30, 10).toPoint();
S2Point pt_mid2 = S2LatLng.fromDegrees(35, 20).toPoint();
S2Point pt_end2 = S2LatLng.fromDegrees(32, 15).toPoint();
// Build a second, distinct polygon and wrap in geography
```
##########
common/src/main/java/org/apache/sedona/common/S2Geography/MultiPolygonGeography.java:
##########
@@ -44,4 +51,50 @@ public int dimension() {
// every child PolygonGeography
return 2;
}
+
+ @Override
+ public void encode(UnsafeOutput out, EncodeOptions opts) throws IOException {
+ out.writeInt(features.size());
+ for (Geography feature : features) {
+ ((PolygonGeography) feature).polygon.encode(out);
+ }
+ out.flush();
+ }
+
+ /** This is what decodeTagged() actually calls */
+ public static MultiPolygonGeography decode(Input in, EncodeTag tag) throws
IOException {
+ // cast to UnsafeInput—will work if you always pass a Kryo-backed stream
+ if (!(in instanceof UnsafeInput)) {
+ throw new IllegalArgumentException("Expected UnsafeInput");
+ }
+ return decode((UnsafeInput) in, tag);
+ }
+
+ /** Decodes a GeographyCollection from a tagged input stream. */
+ public static MultiPolygonGeography decode(UnsafeInput in, EncodeTag tag)
throws IOException {
+ List<S2Polygon> polygons = new ArrayList<>();
+
+ // Handle EMPTY flag
+ if ((tag.getFlags() & EncodeTag.FLAG_EMPTY) != 0) {
+ logger.fine("Decoded empty Multipolygon.");
+ return new MultiPolygonGeography();
+ }
+
+ // Skip any covering data
+ tag.skipCovering(in);
+
+ // 3) Ensure we have at least 4 bytes for the count
+ if (in.available() < Integer.BYTES) {
+ throw new IOException("MultiPolygon.decodeTagged error: insufficient
header bytes");
+ }
+
+ // Read feature count
Review Comment:
The `available()` method on Input streams does not guarantee the actual
number of bytes available for reading and may return 0 even when data is
available. This check could cause false positives. Consider removing this check
or using a different approach to validate sufficient data.
```suggestion
// 3) Read feature count (will throw if insufficient 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
