Copilot commented on code in PR #2275:
URL: https://github.com/apache/sedona/pull/2275#discussion_r2278061818
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/st_constructors.scala:
##########
@@ -23,6 +23,8 @@ import org.apache.spark.sql.sedona_sql.DataFrameShims._
import
org.apache.spark.sql.sedona_sql.expressions.geography.{ST_GeogCollFromText,
ST_GeogFromEWKB, ST_GeogFromText, ST_GeogFromWKB, ST_GeogFromWKT}
import
org.apache.spark.sql.sedona_sql.expressions.geography.{ST_GeogCollFromText,
ST_GeogFromEWKT, ST_GeogFromText, ST_GeogFromWKB, ST_GeogFromWKT}
import
org.apache.spark.sql.sedona_sql.expressions.geography.{ST_GeogCollFromText,
ST_GeogFromGeoHash, ST_GeogFromText, ST_GeogFromWKB, ST_GeogFromWKT}
+import org.apache.spark.sql.sedona_sql.DataFrameShims.{wrapExpression, _}
+import
org.apache.spark.sql.sedona_sql.expressions.geography.{ST_GeogCollFromText,
ST_GeogFromEWKB, ST_GeogFromEWKT, ST_GeogFromText, ST_GeogFromWKB,
ST_GeogFromWKT, ST_GeogToGeometry}
Review Comment:
This import line is duplicated. The same imports (except ST_GeogToGeometry)
are already imported on lines 23-25, making most of this line redundant. Only
ST_GeogToGeometry should be added to an existing import.
```suggestion
import
org.apache.spark.sql.sedona_sql.expressions.geography.{ST_GeogCollFromText,
ST_GeogFromGeoHash, ST_GeogFromText, ST_GeogFromWKB, ST_GeogFromWKT,
ST_GeogToGeometry}
import org.apache.spark.sql.sedona_sql.DataFrameShims.{wrapExpression, _}
```
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/st_constructors.scala:
##########
@@ -23,6 +23,8 @@ import org.apache.spark.sql.sedona_sql.DataFrameShims._
import
org.apache.spark.sql.sedona_sql.expressions.geography.{ST_GeogCollFromText,
ST_GeogFromEWKB, ST_GeogFromText, ST_GeogFromWKB, ST_GeogFromWKT}
import
org.apache.spark.sql.sedona_sql.expressions.geography.{ST_GeogCollFromText,
ST_GeogFromEWKT, ST_GeogFromText, ST_GeogFromWKB, ST_GeogFromWKT}
import
org.apache.spark.sql.sedona_sql.expressions.geography.{ST_GeogCollFromText,
ST_GeogFromGeoHash, ST_GeogFromText, ST_GeogFromWKB, ST_GeogFromWKT}
+import org.apache.spark.sql.sedona_sql.DataFrameShims.{wrapExpression, _}
Review Comment:
This import line is duplicated. The same imports are already added on lines
23-25, making this line redundant.
```suggestion
```
##########
common/src/main/java/org/apache/sedona/common/geography/Constructors.java:
##########
@@ -79,4 +83,185 @@ public static Geography geogFromGeoHash(String geoHash,
Integer precision) {
throw new RuntimeException(e);
}
}
+
+ public static Geometry geogToGeometry(Geography geography, int srid) {
+ int SRID = srid == geography.getSRID() ? srid : geography.getSRID();
Review Comment:
The SRID logic is incorrect. When `srid == geography.getSRID()`, it returns
`srid`, but when they don't match, it returns `geography.getSRID()`,
effectively ignoring the passed `srid` parameter. This should likely prioritize
the passed `srid` parameter or have clearer logic for which SRID to use.
```suggestion
int SRID = srid;
```
##########
spark/common/src/test/scala/org/apache/sedona/sql/geography/ConstructorsDataFrameAPITest.scala:
##########
@@ -96,4 +97,34 @@ class ConstructorsDataFrameAPITest extends TestBaseScala {
assertEquals(expectedWkt, actualResult)
}
+ it("passed st_geogtogeometry pomultipolygonlygon") {
Review Comment:
Test name contains a typo: 'pomultipolygonlygon' should be 'multipolygon'.
```suggestion
it("passed st_geogtogeometry multipolygon") {
```
--
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]
