Copilot commented on code in PR #2223:
URL: https://github.com/apache/sedona/pull/2223#discussion_r2259748256
##########
common/src/main/java/org/apache/sedona/common/Functions.java:
##########
@@ -785,7 +785,7 @@ public static String asEWKT(Geometry geometry) {
}
public static String asEWKT(Geography geography) {
- return asEWKT(geography.getGeometry());
+ return asEWKT(geography);
Review Comment:
This creates infinite recursion. The method calls itself instead of
converting the Geography to a string representation. It should call a WKT
writer method or convert to geometry first.
```suggestion
return asEWKT(geography.toGeometry());
```
##########
common/src/main/java/org/apache/sedona/common/Functions.java:
##########
@@ -797,7 +797,7 @@ public static byte[] asEWKB(Geometry geometry) {
}
public static byte[] asEWKB(Geography geography) {
- return asEWKB(geography.getGeometry());
+ return asEWKB(geography);
Review Comment:
This creates infinite recursion. The method calls itself instead of
converting the Geography to bytes. It should call a WKB writer method or
convert to geometry first.
```suggestion
return asEWKB(geography.toGeometry());
```
##########
common/src/main/java/org/apache/sedona/common/S2Geography/PointGeography.java:
##########
@@ -139,6 +139,7 @@ public void encodeTagged(OutputStream os, EncodeOptions
opts) throws IOException
tag.setCoveringSize((byte) 1);
tag.encode(out);
out.writeLong(cid.id());
+ out.writeInt(getSRID()); // write the SRID
Review Comment:
This appears to be redundant SRID writing. The parent class
Geography.encodeTagged() already writes the SRID, so this additional writeInt()
call could cause deserialization problems.
```suggestion
```
##########
common/src/main/java/org/apache/sedona/common/S2Geography/PointGeography.java:
##########
@@ -148,7 +149,7 @@ public void encodeTagged(OutputStream os, EncodeOptions
opts) throws IOException
}
@Override
- protected void encode(UnsafeOutput out, EncodeOptions opts) throws
IOException {
+ public void encode(UnsafeOutput out, EncodeOptions opts) throws IOException {
Review Comment:
The visibility of the encode method changed from protected to public. This
breaks encapsulation as encode should be an internal implementation detail, not
part of the public API.
```suggestion
protected void encode(UnsafeOutput out, EncodeOptions opts) throws
IOException {
```
##########
common/src/main/java/org/apache/sedona/common/S2Geography/Geography.java:
##########
@@ -204,37 +217,46 @@ public void encodeTagged(OutputStream os, EncodeOptions
opts) throws IOException
// 3) Write the geography
this.encode(out, opts);
+ out.writeInt(getSRID()); // write the SRID
Review Comment:
SRID is written twice in the encodeTagged method - once at line 194 for
empty geometries and again at line 220. This could cause deserialization issues
as the decode method only reads SRID once.
##########
common/src/test/java/org/apache/sedona/common/S2Geography/PointGeographyTest.java:
##########
@@ -42,7 +42,7 @@ public void testEncodeTag() throws IOException {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
geog.encodeTagged(baos, new EncodeOptions());
byte[] data = baos.toByteArray();
- assertEquals(4, data.length);
+ assertEquals(8, data.length);
Review Comment:
The expected length changed from 4 to 8 bytes, but the comment above
indicates this should be 4 bytes. This change should be verified and the
comment updated if the new expectation is correct.
--
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]
