[GitHub] [sedona] jiayuasu commented on a diff in pull request #764: [WIP] [SEDONA-235] Integrate S2, add ST_S2CellIDs

["jiayuasu (via GitHub)" <gi...@apache.org>]

jiayuasu commented on code in PR #764:
URL: https://github.com/apache/sedona/pull/764#discussion_r1103891926


##########
common/src/test/java/org/apache/sedona/common/FunctionsTest.java:
##########
@@ -238,4 +235,98 @@ public void splitHeterogeneousGeometryCollection() {
 
         assertNull(actualResult);
     }
+
+    @Test

Review Comment:
   Please add more tests for different types (7 types in total)



##########
sql/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala:
##########
@@ -1055,3 +1055,11 @@ case class ST_Split(inputExpressions: Seq[Expression])
     copy(inputExpressions = newChildren)
   }
 }
+

Review Comment:
   Please also add this SQL function to Sedona Flink module



##########
common/src/main/java/org/apache/sedona/common/Functions.java:
##########
@@ -543,4 +542,25 @@ public static Geometry split(Geometry input, Geometry blade) {
         // check input geometry
         return new GeometrySplitter(GEOMETRY_FACTORY).split(input, blade);
     }
+
+
+    /**
+     * get the coordinates of a geometry and transform to Google s2 cell id
+     * @param input Geometry
+     * @param level integer, minimum level of cells covering the geometry
+     * @return List of coordinates
+     */
+    public static Long[] s2CellIDs(Geometry input, int level) {
+        HashSet<S2CellId> cellIds = new HashSet<>();
+        List<Geometry> geoms = GeomUtils.extractGeometryCollection(input);
+        for (Geometry geom : geoms) {
+            try {
+                cellIds.addAll(S2Utils.s2RegionToCellIDs(S2Utils.toS2Region(geom), 1, level, Integer.MAX_VALUE - 1));

Review Comment:
   More case handlers are needed. Currently, Sedona supports 7 most common geometry types
   
   instanceOf Point
   instanceOf MuliPoint
   instanceOf Polygon
   instanceOf MultiPolygon
   instanceOf LineString
   instanceOf MultiLineString
   instanceOf GeometryCollection



##########
docs/api/sql/Function.md:
##########
@@ -1536,3 +1536,20 @@ SELECT ST_ZMin(ST_GeomFromText('LINESTRING(1 3 4, 5 6 7)'))
 ```
 
 Output: `4.0`
+
+## ST_S2CellIDs
+
+Introduction: Cover the geometry with Google S2 Cells, return the corresponding cell IDs with the given level.
+The level indicates the [size of cells](https://s2geometry.io/resources/s2cell_statistics.html). With a bigger level,
+the cells will be smaller, the coverage will be more accurate, but the result size will be exponentially increasing.
+
+Format: `ST_S2CellIDs(geom: geometry, level: Int)`
+
+Since: `v1.3.2`

Review Comment:
   Please use version 1.4.0



-- 
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: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org