zhangfengcdt commented on code in PR #1811:
URL: https://github.com/apache/sedona/pull/1811#discussion_r1964149111
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/InferredExpression.scala:
##########
@@ -23,7 +23,7 @@ import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.expressions.{Expression,
ImplicitCastInputTypes}
import org.apache.spark.sql.catalyst.expressions.codegen.CodegenFallback
import org.apache.spark.sql.catalyst.util.ArrayData
-import org.apache.spark.sql.sedona_sql.UDT.GeometryUDT
+import org.apache.spark.sql.sedona_sql.UDT.{GeometryUDT, GeographyUDT}
Review Comment:
I think creating a base class for GeographyUDT and GeometryUDT can promote
code reuse and simplify the implementation of common operations. By doing this,
we can ensure a clear hierarchy and facilitate maintainability, especially when
dealing with operations applicable to both geography and geometry types.
Also, for the Geography and Geometry, we can define a common base GeoObject
as well, and have a Geometry implementation in Sedona that extends this base
class but use JTS Geometry internally to implement algorithms.
##########
common/src/main/java/org/apache/sedona/common/Functions.java:
##########
@@ -784,6 +785,10 @@ public static byte[] asEWKB(Geometry geometry) {
return GeomUtils.getEWKB(geometry);
}
+ public static byte[] geogAsEWKB(Geography geography) {
Review Comment:
Having Geometry and Geography from different packages / repositories looks a
bit strange to me, though I understand the tradeoff here. Is it a good
opportunity to formally introduce the sedona geo types for this in this effort?
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/InferredExpression.scala:
##########
@@ -23,7 +23,7 @@ import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.expressions.{Expression,
ImplicitCastInputTypes}
import org.apache.spark.sql.catalyst.expressions.codegen.CodegenFallback
import org.apache.spark.sql.catalyst.util.ArrayData
-import org.apache.spark.sql.sedona_sql.UDT.GeometryUDT
+import org.apache.spark.sql.sedona_sql.UDT.{GeometryUDT, GeographyUDT}
Review Comment:
Also our RasterUDT can also potentially rebased on this as well.
--
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-unsubscr...@sedona.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
