csringhofer commented on code in PR #6581:
URL: https://github.com/apache/hive/pull/6581#discussion_r3636014772
##########
ql/src/test/org/apache/hadoop/hive/ql/udf/esri/TestEsriShapeConverter.java:
##########
@@ -143,6 +143,120 @@ public void testEmptyAndNullGeometryWriteNullShape()
throws Exception {
assertNull(EsriShapeConverter.fromEsriShape(ByteBuffer.wrap(EsriShapeConverter.toEsriShape(null))));
}
+ private static byte[] hex(String s) {
+ byte[] out = new byte[s.length() / 2];
+ for (int i = 0; i < out.length; i++) {
+ out[i] = (byte) Integer.parseInt(s.substring(2 * i, 2 * i + 2), 16);
+ }
+ return out;
+ }
+
+ /**
+ * Golden bytes hand-encoded from the ESRI Shapefile spec (little-endian),
so this checks
+ * the reader against the on-disk format itself rather than only against our
own writer.
Review Comment:
Note about spec vs our writer: your current code may not handle some cases
that are ok according to the the spec. An example I know are (multi)polygons -
the spec does not mandate any ordering among external / internal rings, while
your reader relies on hulls being followed by their holes. AFAIK the old lib
will also keep this convention, so it is ok to assume it, but based on the lib
and not the spec.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_GeodesicLengthWGS84.java:
##########
@@ -66,37 +65,46 @@ public DoubleWritable evaluate(BytesWritable geomref) {
return null;
}
- OGCGeometry ogcGeometry = GeometryUtils.geometryFromEsriShape(geomref);
- if (ogcGeometry == null) {
+ Geometry geom = GeometryUtils.geometryFromEsriShape(geomref);
+ if (geom == null) {
LogUtils.Log_ArgumentsNull(LOG);
return null;
}
- Geometry esriGeom = ogcGeometry.getEsriGeometry();
- switch (esriGeom.getType()) {
- case Point:
- case MultiPoint:
- resultDouble.set(0.);
- break;
- default:
- MultiPath lines = (MultiPath) (esriGeom);
- int nPath = lines.getPathCount();
- double length = 0.;
- for (int ix = 0; ix < nPath; ix++) {
- int curPt = lines.getPathStart(ix);
- int pastPt = lines.getPathEnd(ix);
- Point fromPt = lines.getPoint(curPt);
- Point toPt = null;
- for (int vx = curPt + 1; vx < pastPt; vx++) {
- toPt = lines.getPoint(vx);
- length += GeometryEngine.geodesicDistanceOnWGS84(fromPt, toPt);
- fromPt = toPt;
+ switch (geom.getGeometryType()) {
+ case "Point", "MultiPoint" -> resultDouble.set(0.0);
+ case "LineString", "LinearRing" ->
resultDouble.set(lineStringLength((LineString) geom));
+ case "MultiLineString" -> {
+ MultiLineString mls = (MultiLineString) geom;
+ double total = 0.0;
+ for (int i = 0; i < mls.getNumGeometries(); i++) {
+ total += lineStringLength((LineString) mls.getGeometryN(i));
}
+ resultDouble.set(total);
}
- resultDouble.set(length);
- break;
+ default -> resultDouble.set(0.0);
}
return resultDouble;
}
+
+ /**
+ * Computes the geodesic length of a single LineString by summing Haversine
+ * distances between consecutive vertices. Iterates the CoordinateSequence
Review Comment:
I don't think that it is ok to call this ST_GeodesicLengthWGS84 with
Haversine formula as geodesic + WGS84 implies ellipsoid. AFAIK the Esri lib
uses Vincenty underneath, which is not included in JTS.
I would
a: rename this function to ST_LengthSphere (redshift has this:
https://docs.aws.amazon.com/redshift/latest/dg/ST_LengthSphere-function.html)
b: keep the ESRI geometry library to fill this gap in JTS until a
replacement lib is found for ellipsoid computations
Note that the Esri implementation also sums the segment length in more
sophisticated way to reduce precision loss when adding doubles. jts also has
tools for this:
https://github.com/locationtech/jts/blob/master/modules/core/src/main/java/org/locationtech/jts/math/DD.java
Probably this is not that important, but can be another source of small
differences.
A bigger issue IMO is that this function is not tested at all. Impala has a
few tests generated from the original spatail framwork for Hadoop tests:
https://github.com/apache/impala/blob/780e6683a21dae3622744a82f92e155ae06e13f2/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test#L397
##########
ql/src/test/org/apache/hadoop/hive/ql/udf/esri/TestEsriShapeConverter.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.udf.esri;
+
+import org.junit.Test;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.LineString;
+import org.locationtech.jts.geom.MultiLineString;
+import org.locationtech.jts.geom.MultiPoint;
+import org.locationtech.jts.geom.MultiPolygon;
+import org.locationtech.jts.geom.Point;
+import org.locationtech.jts.geom.Polygon;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKTReader;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+public class TestEsriShapeConverter {
+
+ private static final double EPSILON = 1e-9;
+
+ private static final WKTReader WKT_READER = new
WKTReader(GeometryUtils.GEOMETRY_FACTORY);
+
+ private static Geometry wkt(String text) throws ParseException {
+ return WKT_READER.read(text);
+ }
+
+ private static Geometry roundTrip(String text) throws ParseException {
+ Geometry geom = wkt(text);
+ byte[] shape = EsriShapeConverter.toEsriShape(geom);
+ Geometry back = EsriShapeConverter.fromEsriShape(ByteBuffer.wrap(shape));
+ assertNotNull("Round-trip of " + text + " must not be null!", back);
+ return back;
+ }
+
+ private static int shapeType(byte[] shape) {
+ return ByteBuffer.wrap(shape).order(ByteOrder.LITTLE_ENDIAN).getInt();
+ }
+
+ @Test
+ public void testPointRoundTrip() throws Exception {
+ Point point = (Point) roundTrip("point (12.224 51.829)");
+ assertEquals(12.224, point.getX(), EPSILON);
+ assertEquals(51.829, point.getY(), EPSILON);
+ }
+
+ @Test
+ public void testPointZRoundTrip() throws Exception {
+ Point point = (Point) roundTrip("point z (1 2 3)");
+ assertEquals(1, point.getX(), EPSILON);
+ assertEquals(2, point.getY(), EPSILON);
+ assertEquals(3, point.getCoordinate().getZ(), EPSILON);
+ assertEquals(11, shapeType(EsriShapeConverter.toEsriShape(wkt("point z (1
2 3)"))));
+ }
+
+ @Test
+ public void testMultiPointRoundTrip() throws Exception {
+ Geometry back = roundTrip("multipoint ((1 2), (3 4), (5 6))");
+ assertTrue("Expected a MultiPoint!", back instanceof MultiPoint);
+ assertTrue(back.equalsTopo(wkt("multipoint ((1 2), (3 4), (5 6))")));
+ }
+
+ @Test
+ public void testMultiPointZRoundTrip() throws Exception {
+ MultiPoint back = (MultiPoint) roundTrip("multipoint z ((1 2 3), (4 5
6))");
+ assertEquals(3, back.getGeometryN(0).getCoordinate().getZ(), EPSILON);
+ assertEquals(6, back.getGeometryN(1).getCoordinate().getZ(), EPSILON);
+ }
+
+ @Test
+ public void testLineStringRoundTrip() throws Exception {
+ Geometry back = roundTrip("linestring (0 0, 1 1, 2 0)");
+ assertTrue("Expected a LineString!", back instanceof LineString);
+ assertTrue(back.equalsTopo(wkt("linestring (0 0, 1 1, 2 0)")));
+ }
+
+ @Test
+ public void testMultiLineStringRoundTrip() throws Exception {
+ String text = "multilinestring ((0 0, 1 1), (2 2, 3 3))";
+ Geometry back = roundTrip(text);
+ assertTrue("Expected a MultiLineString!", back instanceof MultiLineString);
+ assertTrue(back.equalsTopo(wkt(text)));
+ }
+
+ @Test
+ public void testLineStringZRoundTrip() throws Exception {
+ LineString back = (LineString) roundTrip("linestring z (0 0 1, 1 1 2)");
+ assertEquals(1, back.getCoordinateN(0).getZ(), EPSILON);
+ assertEquals(2, back.getCoordinateN(1).getZ(), EPSILON);
+ }
+
+ @Test
+ public void testPolygonRoundTrip() throws Exception {
+ String text = "polygon ((0 0, 4 0, 4 4, 0 4, 0 0))";
+ Geometry back = roundTrip(text);
+ assertTrue("Expected a Polygon!", back instanceof Polygon);
+ assertTrue(back.equalsTopo(wkt(text)));
+ }
+
+ @Test
+ public void testPolygonWithHoleRoundTrip() throws Exception {
+ String text = "polygon ((0 0, 10 0, 10 10, 0 10, 0 0), (2 2, 2 4, 4 4, 4
2, 2 2))";
+ Polygon back = (Polygon) roundTrip(text);
+ assertEquals(1, back.getNumInteriorRing());
+ assertTrue(back.equalsTopo(wkt(text)));
+ }
+
+ @Test
+ public void testMultiPolygonRoundTrip() throws Exception {
Review Comment:
It would be nice to add multipolygon with holes, as it the most complex case
to (de)serialize due to ring orientation rules.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_Buffer.java:
##########
@@ -37,14 +37,13 @@ public BytesWritable evaluate(BytesWritable geometryref1,
DoubleWritable distanc
return null;
}
- OGCGeometry ogcGeometry =
GeometryUtils.geometryFromEsriShape(geometryref1);
- if (ogcGeometry == null) {
+ Geometry geom = GeometryUtils.geometryFromEsriShape(geometryref1);
+ if (geom == null) {
LogUtils.Log_ArgumentsNull(LOG);
return null;
}
- OGCGeometry bufferedGeometry = ogcGeometry.buffer(distance.get());
- // TODO persist type information (polygon vs multipolygon)
+ Geometry bufferedGeometry = geom.buffer(distance.get(), 24);
Review Comment:
24 quadrantSegments is chosen to match Esri lib behavior? A comment could
mention this.
(this change is familiar to me from Trino PR description:
https://github.com/trinodb/trino/pull/27881)
##########
ql/src/test/queries/clientpositive/geospatial_udfs.q:
##########
@@ -1,3 +1,6 @@
+-- Truncate floating-point numbers to 12 decimal places for cross-platform
reproducibility
+-- (JTS buffer uses Math.cos/sin which can differ by 1 ULP across JDK
implementations)
+--! qt:replace:/(\d+\.\d{12})\d+/$1/
-- create a table with two columns one for each point.
Review Comment:
I recently created HIVE-29726 about a bug in st_intersects() on
multilinestrings. The bug comes from the ESRI lib, so this patch should fix it.
Do you think that it makes sense to create a PR for that before this change is
merged, or lets fix the issue with that?
Anyway, it would be nice to have a regression test for it, so that the
following query returns false:
select st_intersects(st_geomfromtext('LINESTRING(1 0, 1 1)'),
st_geomfromtext('MULTILINESTRING((0 0, 0 1), (2 0, 2 1))'));
##########
ql/src/test/org/apache/hadoop/hive/ql/udf/esri/TestEsriShapeConverter.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.udf.esri;
+
+import org.junit.Test;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.LineString;
+import org.locationtech.jts.geom.MultiLineString;
+import org.locationtech.jts.geom.MultiPoint;
+import org.locationtech.jts.geom.MultiPolygon;
+import org.locationtech.jts.geom.Point;
+import org.locationtech.jts.geom.Polygon;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKTReader;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+public class TestEsriShapeConverter {
+
+ private static final double EPSILON = 1e-9;
+
+ private static final WKTReader WKT_READER = new
WKTReader(GeometryUtils.GEOMETRY_FACTORY);
+
+ private static Geometry wkt(String text) throws ParseException {
+ return WKT_READER.read(text);
+ }
+
+ private static Geometry roundTrip(String text) throws ParseException {
+ Geometry geom = wkt(text);
+ byte[] shape = EsriShapeConverter.toEsriShape(geom);
+ Geometry back = EsriShapeConverter.fromEsriShape(ByteBuffer.wrap(shape));
+ assertNotNull("Round-trip of " + text + " must not be null!", back);
+ return back;
+ }
+
+ private static int shapeType(byte[] shape) {
+ return ByteBuffer.wrap(shape).order(ByteOrder.LITTLE_ENDIAN).getInt();
+ }
+
+ @Test
+ public void testPointRoundTrip() throws Exception {
+ Point point = (Point) roundTrip("point (12.224 51.829)");
+ assertEquals(12.224, point.getX(), EPSILON);
+ assertEquals(51.829, point.getY(), EPSILON);
+ }
+
+ @Test
+ public void testPointZRoundTrip() throws Exception {
+ Point point = (Point) roundTrip("point z (1 2 3)");
+ assertEquals(1, point.getX(), EPSILON);
+ assertEquals(2, point.getY(), EPSILON);
+ assertEquals(3, point.getCoordinate().getZ(), EPSILON);
+ assertEquals(11, shapeType(EsriShapeConverter.toEsriShape(wkt("point z (1
2 3)"))));
+ }
+
+ @Test
+ public void testMultiPointRoundTrip() throws Exception {
+ Geometry back = roundTrip("multipoint ((1 2), (3 4), (5 6))");
+ assertTrue("Expected a MultiPoint!", back instanceof MultiPoint);
+ assertTrue(back.equalsTopo(wkt("multipoint ((1 2), (3 4), (5 6))")));
+ }
+
+ @Test
+ public void testMultiPointZRoundTrip() throws Exception {
+ MultiPoint back = (MultiPoint) roundTrip("multipoint z ((1 2 3), (4 5
6))");
+ assertEquals(3, back.getGeometryN(0).getCoordinate().getZ(), EPSILON);
+ assertEquals(6, back.getGeometryN(1).getCoordinate().getZ(), EPSILON);
+ }
+
+ @Test
+ public void testLineStringRoundTrip() throws Exception {
+ Geometry back = roundTrip("linestring (0 0, 1 1, 2 0)");
+ assertTrue("Expected a LineString!", back instanceof LineString);
+ assertTrue(back.equalsTopo(wkt("linestring (0 0, 1 1, 2 0)")));
+ }
+
+ @Test
+ public void testMultiLineStringRoundTrip() throws Exception {
+ String text = "multilinestring ((0 0, 1 1), (2 2, 3 3))";
+ Geometry back = roundTrip(text);
+ assertTrue("Expected a MultiLineString!", back instanceof MultiLineString);
+ assertTrue(back.equalsTopo(wkt(text)));
+ }
+
+ @Test
+ public void testLineStringZRoundTrip() throws Exception {
+ LineString back = (LineString) roundTrip("linestring z (0 0 1, 1 1 2)");
+ assertEquals(1, back.getCoordinateN(0).getZ(), EPSILON);
+ assertEquals(2, back.getCoordinateN(1).getZ(), EPSILON);
+ }
+
+ @Test
+ public void testPolygonRoundTrip() throws Exception {
+ String text = "polygon ((0 0, 4 0, 4 4, 0 4, 0 0))";
+ Geometry back = roundTrip(text);
+ assertTrue("Expected a Polygon!", back instanceof Polygon);
+ assertTrue(back.equalsTopo(wkt(text)));
+ }
+
+ @Test
+ public void testPolygonWithHoleRoundTrip() throws Exception {
+ String text = "polygon ((0 0, 10 0, 10 10, 0 10, 0 0), (2 2, 2 4, 4 4, 4
2, 2 2))";
+ Polygon back = (Polygon) roundTrip(text);
+ assertEquals(1, back.getNumInteriorRing());
+ assertTrue(back.equalsTopo(wkt(text)));
+ }
+
+ @Test
+ public void testMultiPolygonRoundTrip() throws Exception {
+ String text = "multipolygon (((0 0, 4 0, 4 4, 0 4, 0 0)), ((10 10, 14 10,
14 14, 10 14, 10 10)))";
+ Geometry back = roundTrip(text);
+ assertTrue("Expected a MultiPolygon!", back instanceof MultiPolygon);
+ assertEquals(2, back.getNumGeometries());
+ assertTrue(back.equalsTopo(wkt(text)));
Review Comment:
Is there a reason for using equalsTopo? It can hide subtle differences like
reordering rings. Not sure if it enforces ring orientation.
IMO equalsExact should be also true here and could catch more bugs. It has a
version with tolerance to accept small rounding of coordinates:
https://locationtech.github.io/jts/javadoc/org/locationtech/jts/geom/Geometry.html#equalsExact-org.locationtech.jts.geom.Geometry-double-
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_GeodesicLengthWGS84.java:
##########
@@ -17,14 +17,13 @@
*/
package org.apache.hadoop.hive.ql.udf.esri;
Review Comment:
Do you plan to rename the package? At this point there is not much Esri
about these functions anymore. Probably best done in another commit to reduce
noise.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/EsriShapeConverter.java:
##########
@@ -0,0 +1,651 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.udf.esri;
+
+import org.locationtech.jts.algorithm.Orientation;
+import org.locationtech.jts.geom.Coordinate;
+import org.locationtech.jts.geom.CoordinateXYM;
+import org.locationtech.jts.geom.CoordinateXYZM;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryCollection;
+import org.locationtech.jts.geom.LinearRing;
+import org.locationtech.jts.geom.LineString;
+import org.locationtech.jts.geom.MultiLineString;
+import org.locationtech.jts.geom.MultiPoint;
+import org.locationtech.jts.geom.MultiPolygon;
+import org.locationtech.jts.geom.Point;
+import org.locationtech.jts.geom.Polygon;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Reads and writes raw ESRI shape binary format directly from/to JTS
geometries,
+ * without any dependency on the ESRI Geometry library.
+ *
+ * <p>All binary values are little-endian as per the ESRI shape format
specification
+ * (ESRI Shapefile Technical Description, July 1998,
+ * <a
href="https://www.esri.com/content/dam/esrisites/sitecore-archive/Files/Pdfs/library/whitepapers/pdfs/shapefile.pdf";>
+ * shapefile.pdf</a>).
+ *
+ * <p>Supported shape types on read:
+ * <ul>
+ * <li>0 - Null/Unknown</li>
+ * <li>1 - Point</li>
+ * <li>3 - Polyline (paths)</li>
+ * <li>5 - Polygon (rings)</li>
+ * <li>8 - MultiPoint</li>
+ * <li>11 - PointZ (optionally with M)</li>
+ * <li>13 - PolylineZ (optionally with M)</li>
+ * <li>15 - PolygonZ (optionally with M)</li>
+ * <li>18 - MultiPointZ (optionally with M)</li>
+ * <li>21 - PointM</li>
+ * <li>23 - PolylineM</li>
+ * <li>25 - PolygonM</li>
+ * <li>28 - MultiPointM</li>
+ * </ul>
+ *
+ * <p>The M-bearing types are read (to stay compatible with data serialized by
the
+ * previous ESRI-library implementation) but never written; {@link
#toEsriShape} emits
+ * only the XY and XYZ types.
+ */
+public final class EsriShapeConverter {
+
+ private static final int TYPE_NULL = 0;
+ private static final int TYPE_POINT = 1;
+ private static final int TYPE_POLYLINE = 3;
+ private static final int TYPE_POLYGON = 5;
+ private static final int TYPE_MULTIPOINT = 8;
+ private static final int TYPE_POINT_Z = 11;
+ private static final int TYPE_POLYLINE_Z = 13;
+ private static final int TYPE_POLYGON_Z = 15;
+ private static final int TYPE_MULTIPOINT_Z = 18;
+ private static final int TYPE_POINT_M = 21;
+ private static final int TYPE_POLYLINE_M = 23;
+ private static final int TYPE_POLYGON_M = 25;
+ private static final int TYPE_MULTIPOINT_M = 28;
+
+ // ESRI marks an absent measure with any value less than this sentinel.
+ private static final double NO_DATA_M = -1e38;
+
+ private EsriShapeConverter() {
+ }
+
+ /**
+ * Reads the ESRI shape binary from {@code shapeBuffer} (already positioned
at start,
+ * little-endian byte order will be set internally) and returns a JTS {@link
Geometry}.
+ *
+ * @param shapeBuffer buffer containing raw ESRI shape bytes starting at its
current position
+ * @return a JTS Geometry, or {@code null} for the Null/Unknown shape type
+ * @throws IllegalArgumentException if the buffer contains an unsupported or
malformed shape
+ */
+ public static Geometry fromEsriShape(ByteBuffer shapeBuffer) {
Review Comment:
It is a bit inconsistent that this function expects the shape file format
part of a shape, while geometryFromEsriShape expects the header. Could be
renamed, e.g. to fromEsriShapeBody.
##########
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:
I don't have a better solution, but I am worried about interoperability. If
there are actually tables with the old format, and users insert to those after
this change, then we'll have mixed tables. Reading those will be really painful
with any engine other than Hive.
Is there are function that just reads a BINARY and writes it out in the the
new format? Would be useful to rewrite tables to get rid of the old format.
ST_GeomFromShape does only read old format, right?
In Impala I made the choice of Esri shape vs new format (WKB) a startup flag
in https://gerrit.cloudera.org/#/c/24549/ . This is less convenient than
accepting both, but I see it as a temporary solution before adding GEOMETRY
type and I don't want to commit to a custom format for that time.
What is the plan in Hive about adding GEOMETRY type? That would be an
unavoidable breaking change, as functions that return BINARY shapes would need
to return geometry (e.g. st_point), which would break queries that relied on
the type being BINARY. Adding this new format now would mean that you'll need
to be able to read it later, e.g by having ST_GeomFromFromNewHiveShape() (or
accepting both formats in ST_GeomFromShape() ).
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_GeomFromShape.java:
##########
@@ -40,29 +41,14 @@ public BytesWritable evaluate(BytesWritable shape) throws
UDFArgumentException {
public BytesWritable evaluate(BytesWritable shape, int wkid) throws
UDFArgumentException {
try {
- Geometry geometry =
GeometryEngine.geometryFromEsriShape(shape.getBytes(), Geometry.Type.Unknown);
- switch (geometry.getType()) {
- case Point:
- return GeometryUtils.geometryToEsriShapeBytesWritable(geometry, wkid,
OGCType.ST_POINT);
-
- case MultiPoint:
- return GeometryUtils.geometryToEsriShapeBytesWritable(geometry, wkid,
OGCType.ST_MULTIPOINT);
-
- case Line:
- return GeometryUtils.geometryToEsriShapeBytesWritable(geometry, wkid,
OGCType.ST_LINESTRING);
-
- case Polyline:
- return GeometryUtils.geometryToEsriShapeBytesWritable(geometry, wkid,
OGCType.ST_MULTILINESTRING);
-
- case Envelope:
- return GeometryUtils.geometryToEsriShapeBytesWritable(geometry, wkid,
OGCType.ST_POLYGON);
-
- case Polygon:
- return GeometryUtils.geometryToEsriShapeBytesWritable(geometry, wkid,
OGCType.ST_MULTIPOLYGON);
-
- default:
- return GeometryUtils.geometryToEsriShapeBytesWritable(geometry, wkid,
OGCType.UNKNOWN);
+ ByteBuffer shapeBuffer = ByteBuffer.wrap(shape.getBytes(), 0,
shape.getLength())
+ .order(ByteOrder.LITTLE_ENDIAN);
+ Geometry jtsGeom = EsriShapeConverter.fromEsriShape(shapeBuffer);
Review Comment:
This function reads only the old format, right? Can you add a comment about
that?
--
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]
