Copilot commented on code in PR #2260:
URL: https://github.com/apache/sedona/pull/2260#discussion_r2267963618
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/geography/Constructors.scala:
##########
@@ -79,3 +79,18 @@ private[apache] case class ST_GeogFromWKB(inputExpressions:
Seq[Expression])
copy(inputExpressions = newChildren)
}
}
+
+/**
+ * Return a Geography from a EWKB string
+ *
+ * @param inputExpressions
+ * This function takes a geometry string and a srid. The string format must
be WKB binary array
+ * / string.
+ */
+private[apache] case class ST_GeogFromEWKB(inputExpressions: Seq[Expression])
+ extends InferredExpression(Constructors.geogFromWKB(_: Array[Byte], _:
Int)) {
Review Comment:
The ST_GeogFromEWKB constructor is incorrectly using the same underlying
implementation as ST_GeogFromWKB. EWKB format contains embedded SRID
information that should be extracted from the binary data, not passed as a
separate parameter. This should use a different method that can parse the SRID
from the EWKB data itself.
```suggestion
extends InferredExpression(Constructors.geogFromEWKB(_: Array[Byte])) {
```
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/st_constructors.scala:
##########
@@ -108,6 +108,9 @@ object st_constructors {
wrapExpression[ST_GeogFromWKB](wkb, srid)
def ST_GeogFromWKB(wkb: String, srid: Int): Column =
wrapExpression[ST_GeogFromWKB](wkb, srid)
+ def ST_GeogFromEWKB(wkb: Column): Column =
wrapExpression[ST_GeogFromEWKB](wkb, 0)
+ def ST_GeogFromEWKB(wkb: String): Column =
wrapExpression[ST_GeogFromEWKB](wkb, 0)
Review Comment:
Hardcoding SRID as 0 is incorrect for EWKB format. EWKB (Extended Well-Known
Binary) contains the SRID embedded in the binary data itself, so passing 0 as a
default SRID parameter defeats the purpose of using EWKB. The expression should
only take the WKB data and extract the SRID from it.
```suggestion
def ST_GeogFromEWKB(wkb: Column): Column =
wrapExpression[ST_GeogFromEWKB](wkb)
def ST_GeogFromEWKB(wkb: String): Column =
wrapExpression[ST_GeogFromEWKB](wkb)
```
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/st_constructors.scala:
##########
@@ -108,6 +108,9 @@ object st_constructors {
wrapExpression[ST_GeogFromWKB](wkb, srid)
def ST_GeogFromWKB(wkb: String, srid: Int): Column =
wrapExpression[ST_GeogFromWKB](wkb, srid)
+ def ST_GeogFromEWKB(wkb: Column): Column =
wrapExpression[ST_GeogFromEWKB](wkb, 0)
+ def ST_GeogFromEWKB(wkb: String): Column =
wrapExpression[ST_GeogFromEWKB](wkb, 0)
Review Comment:
Same issue as the previous line - hardcoding SRID as 0 is incorrect for EWKB
format. The SRID should be extracted from the EWKB data itself, not passed as a
parameter.
```suggestion
def ST_GeogFromEWKB(wkb: Column): Column =
wrapExpression[ST_GeogFromEWKB](wkb)
def ST_GeogFromEWKB(wkb: String): Column =
wrapExpression[ST_GeogFromEWKB](wkb)
```
--
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]
