You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "ZhuochengShang (via GitHub)" <gi...@apache.org> on 2025/08/04 21:27:41 UTC
[PR] [GH-] Register S2Geography UDT as Geography UDT into Apache Sedona [sedona]
ZhuochengShang opened a new pull request, #2223:
URL: https://github.com/apache/sedona/pull/2223
## What changes were proposed in this PR?
Register S2Geography UDT as Geography UDT into Apache Sedona
Create Geography format utils
Create Geography serializers
--
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@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2224] Register S2Geography UDT as Geography UDT into Apache Sedona [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2223:
URL: https://github.com/apache/sedona/pull/2223#discussion_r2258257465
##########
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:
I think this comment from AI is correct. We should fix this.
--
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@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2224] Register S2Geography UDT as Geography UDT into Apache Sedona [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu merged PR #2223:
URL: https://github.com/apache/sedona/pull/2223
--
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@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2224] Register S2Geography UDT as Geography UDT into Apache Sedona [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2224] Register S2Geography UDT as Geography UDT into Apache Sedona [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #2223:
URL: https://github.com/apache/sedona/pull/2223#issuecomment-3163360878
I made the following changes:
* Removed the additional `Geography` class which introduces unnecessary code complexity
* It was there because the old Geography class was just a wrapper of JTS Geometry
* Add the SRID support in Geography serializer. SRID is always appended at the end of the bytestream
* Fixed the inferred expression issues with ST_GeogFromWKT
* Removed unnecessary complex Geog constructor logic
Note that the Scala test is now having issues with PrecisionModel. Looks like the WKT reader / writer does not implement PrecisionModel. We need to fix this first otherwise we won't be able to use it to run tests.
--
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@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2224] Register S2Geography UDT as Geography UDT into Apache Sedona [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2223:
URL: https://github.com/apache/sedona/pull/2223#discussion_r2258261623
##########
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:
Please fix this.
--
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@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2224] Register S2Geography UDT as Geography UDT into Apache Sedona [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2223:
URL: https://github.com/apache/sedona/pull/2223#discussion_r2258261204
##########
common/src/main/java/org/apache/sedona/common/utils/GeographyFormatUtils.java:
##########
@@ -0,0 +1,230 @@
+/*
Review Comment:
This file is not needed. This counterpart of this file in Geometry is mainly used for the old RDD APIs. Since we don't plan to support Geography in RDD API, I think we can simply call the WKT and WKB function of S2geography in ST_GeogFromWKT and ST_GeogFromWKB.
--
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@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2224] Register S2Geography UDT as Geography UDT into Apache Sedona [sedona]
Posted by "ZhuochengShang (via GitHub)" <gi...@apache.org>.
ZhuochengShang commented on PR #2223:
URL: https://github.com/apache/sedona/pull/2223#issuecomment-3165052195
> I made the following changes:
>
> * Removed the additional `Geography` class which introduces unnecessary code complexity
>
> * It was there because the old Geography class was just a wrapper of JTS Geometry
> * Add the SRID support in Geography serializer. SRID is always appended at the end of the bytestream
> * Fixed the inferred expression issues with ST_GeogFromWKT
> * Removed unnecessary complex Geog constructor logic
>
> Note that the Scala test is now having issues with PrecisionModel. Looks like the WKT reader / writer does not implement PrecisionModel. We need to fix this first otherwise we won't be able to use it to run tests.
I write a function toString() can return WKT string with PrecisionModel.FIXED as default
--
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@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2224] Register S2Geography UDT as Geography UDT into Apache Sedona [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org