csringhofer commented on code in PR #6581:
URL: https://github.com/apache/hive/pull/6581#discussion_r3638435461


##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/GeometryUtils.java:
##########
@@ -17,29 +17,236 @@
  */
 package org.apache.hadoop.hive.ql.udf.esri;
 
-import com.esri.core.geometry.Geometry;
-import com.esri.core.geometry.GeometryEngine;
-import com.esri.core.geometry.MapGeometry;
-import com.esri.core.geometry.OperatorImportFromESRIShape;
-import com.esri.core.geometry.Polygon;
-import com.esri.core.geometry.SpatialReference;
-import com.esri.core.geometry.ogc.OGCGeometry;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import 
org.apache.hadoop.hive.serde2.objectinspector.primitive.PrimitiveObjectInspectorFactory;
 import 
org.apache.hadoop.hive.serde2.objectinspector.primitive.WritableBinaryObjectInspector;
 import org.apache.hadoop.io.BytesWritable;
+import org.locationtech.jts.algorithm.Orientation;
+import org.locationtech.jts.geom.CoordinateSequence;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryCollection;
+import org.locationtech.jts.geom.GeometryFactory;
+import org.locationtech.jts.geom.LineString;
+import org.locationtech.jts.geom.LinearRing;
+import org.locationtech.jts.geom.MultiPolygon;
+import org.locationtech.jts.geom.Point;
+import org.locationtech.jts.geom.Polygon;
+import org.locationtech.jts.io.Ordinate;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+import org.locationtech.jts.io.WKBWriter;
+import org.locationtech.jts.io.WKTReader;
+import org.locationtech.jts.io.WKTWriter;
+import org.locationtech.jts.io.geojson.GeoJsonReader;
+import org.locationtech.jts.io.geojson.GeoJsonWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
+import java.util.Arrays;
+import java.util.Set;
 
 public class GeometryUtils {
 
+  private static final Logger LOG = 
LoggerFactory.getLogger(GeometryUtils.class);
+
   private static final int SIZE_WKID = 4;
   private static final int SIZE_TYPE = 1;
 
   public static final int WKID_UNKNOWN = 0;
 
+  // Magic byte at offset 4 to distinguish new WKB format from old ESRI format.

Review Comment:
   >and since the old format was already Hive-specific
   
   Impala and Trino can read it. 
https://github.com/trinodb/trino/blob/34bd248c7430256f69d9969f06bd03fa8bba852c/plugin/trino-geospatial/src/main/java/io/trino/plugin/geospatial/GeoFunctions.java#L605
   Probably others too - anyone who can run the Hive UDFs from geospatial 
framework for Hadoop, e.g. Spark. There may be more UDFs that use it.
   
   Adding GEOMETRY support may make sense even without Iceberg integration - 
you could read existing BINARY data with st_geomFromShape(), and treat it as 
GEOMETRY from that point. It could be a huge perf improvement if jts Geometry 
objects could be passed with GEOMETRY instead of always serializing + 
deserializing them between functions (I have no clue how hard is this to 
Implement in Hive - I haven't implemented it in Impala yet).
   
   For the current change: would it be hard to split it in two and keep the 
existing serialization format while switching to JTS? The only missing piece I 
see is writing M values to Esri format. It also doesn't seem inherently slower 
to parse Esri shape for JTS geometries compared to WKB. Without the 
serialization format change the behavior differences would be minor, similarly 
to the Trino PR, so 90% of this PR could be merged without much thinking.



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to