csringhofer commented on code in PR #6581:
URL: https://github.com/apache/hive/pull/6581#discussion_r3652838558
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_GeodesicLengthWGS84.java:
##########
@@ -66,37 +70,100 @@ 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 the
ellipsoidal
+ * (WGS84) distances between consecutive vertices. Iterates the
CoordinateSequence
+ * directly to avoid per-vertex Point object allocation.
+ */
+ private static double lineStringLength(LineString line) {
+ CoordinateSequence seq = line.getCoordinateSequence();
+ int nPts = seq.size();
+ if (nPts < 2) {
+ return 0.0;
+ }
+ double length = 0.0;
+ for (int i = 1; i < nPts; i++) {
+ length += vincentyDistanceMeters(
+ seq.getX(i - 1), seq.getY(i - 1),
+ seq.getX(i), seq.getY(i));
+ }
+ return length;
+ }
+
+ /**
+ * Ellipsoidal geodesic distance between two lon/lat points on the WGS84
spheroid,
+ * using Vincenty's inverse formula (the same ellipsoidal model the ESRI
library used,
+ * so results match the previous implementation). Coordinates are in
degrees, result
Review Comment:
Wouldn't it be simpler to keep to ESRI lib for this functionality?
https://github.com/Esri/geometry-api-java/blob/master/src/main/java/com/esri/core/geometry/GeoDist.java
has its own Vincenty, or the same function could be used an in the original
UDF.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/EsriShapeConverter.java:
##########
@@ -0,0 +1,742 @@
+/*
+ * 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.
Review Comment:
The code uses <=
Which one is correct? Based on googling it seems that ==-1e38 is also
considered no data.
##########
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:
ack
##########
ql/src/test/org/apache/hadoop/hive/ql/udf/esri/TestEsriShapeConverter.java:
##########
@@ -0,0 +1,366 @@
+/*
+ * 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.apache.hadoop.io.BytesWritable;
+import org.junit.Test;
+import org.locationtech.jts.algorithm.Orientation;
+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.assertFalse;
+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.fromEsriShapeBody(ByteBuffer.wrap(shape));
+ assertNotNull("Round-trip of " + text + " must not be null!", back);
+ return back;
+ }
+
+ /**
+ * Asserts coordinate-level equality after canonicalizing both geometries
with
+ * {@link Geometry#normalize()}. Unlike {@code equalsTopo}, this catches
ring reordering,
+ * ring/hole misassignment and orientation bugs (normalize fixes orientation
and vertex
+ * order, so anything left is a real coordinate difference).
+ */
+ private static void assertEqualsNormalized(Geometry expected, Geometry
actual) {
+ Geometry e = expected.copy();
+ Geometry a = actual.copy();
+ e.normalize();
Review Comment:
Is normalization really needed? The round trips should not change the order
of coordinates. Also, fixing orientation can actually hide bugs.
I still think that equalsExact with tolerance is the right way to check
round trips. The order of coordinates can matter, e.g. when representing a
route with a linestring reversing it change semantics.
--
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]
