Copilot commented on code in PR #2223:
URL: https://github.com/apache/sedona/pull/2223#discussion_r2258250119
##########
common/src/main/java/org/apache/sedona/common/geometryObjects/Geography.java:
##########
@@ -18,20 +18,60 @@
*/
package org.apache.sedona.common.geometryObjects;
-import org.locationtech.jts.geom.Geometry;
+import com.esotericsoftware.kryo.io.UnsafeOutput;
+import com.google.common.geometry.S2Region;
+import com.google.common.geometry.S2Shape;
+import java.io.IOException;
+import org.apache.sedona.common.S2Geography.EncodeOptions;
+import org.apache.sedona.common.S2Geography.S2Geography;
-public class Geography {
- private final Geometry geometry;
+public class Geography extends S2Geography {
+ // Hold the underlying S2Geography directly, not another Geography
+ private final S2Geography delegate;
- public Geography(Geometry geometry) {
- this.geometry = geometry;
+ public Geography(S2Geography delegate) {
+ // Initialize super with the correct kind
+ super(GeographyKind.fromKind(delegate.getKind()));
+ this.delegate = delegate;
}
- public Geometry getGeometry() {
- return this.geometry;
+ /** Return the wrapped S2Geography. */
+ public Geography getGeography() {
+ return (Geography) delegate;
Review Comment:
This method creates an infinite recursion. The method `getGeography()`
returns `(Geography) delegate`, but `delegate` is of type `S2Geography`, not
`Geography`. This cast will either fail at runtime or create incorrect
behavior. The method should likely return `this` or the delegate directly
without casting.
```suggestion
public S2Geography getGeography() {
return delegate;
```
##########
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.getGeography());
Review Comment:
This creates infinite recursion since `geography.getGeography()` returns a
`Geography` object, and this method expects a `Geography` parameter, leading to
an endless loop. The method should likely call a different method or access the
underlying geometry data differently.
```suggestion
return asEWKT(geography.getGeometry());
```
##########
spark/common/src/test/scala/org/apache/sedona/sql/constructorTestScala.scala:
##########
@@ -264,13 +264,13 @@ class constructorTestScala extends TestBaseScala {
assert(thrown.getMessage.contains("Unknown geometry type"))
}
- it("Passed ST_GeogFromWKT") {
- val wkt = "LINESTRING (1 2, 3 4, 5 6)"
- val row = sparkSession.sql(s"SELECT ST_GeogFromWKT('$wkt') AS
geog").first()
- val geog = row.get(0)
- assert(geog.isInstanceOf[Geography])
- assert(geog.asInstanceOf[Geography].getGeometry.toText == wkt)
- }
+// it("Passed ST_GeogFromWKT") {
+// val wkt = "LINESTRING (1 2, 3 4, 5 6)"
+// val row = sparkSession.sql(s"SELECT ST_GeogFromWKT('$wkt') AS
geog").first()
+// val geog = row.get(0)
+// assert(geog.isInstanceOf[Geography])
+// assert(geog.asInstanceOf[Geography].getGeographhy.toText == wkt)
Review Comment:
There is a spelling error in the method name 'getGeographhy' - it should be
'getGeography'.
```suggestion
// assert(geog.asInstanceOf[Geography].getGeography.toText == wkt)
```
##########
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.getGeography());
Review Comment:
This creates infinite recursion since `geography.getGeography()` returns a
`Geography` object, and this method expects a `Geography` parameter, leading to
an endless loop. The method should likely call a different method or access the
underlying geometry data differently.
```suggestion
return asEWKB(geography.getGeometry());
```
--
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]
