Copilot commented on code in PR #6581:
URL: https://github.com/apache/hive/pull/6581#discussion_r3542670549
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_PointFromWKB.java:
##########
@@ -55,17 +52,11 @@ public BytesWritable evaluate(BytesWritable wkb) throws
UDFArgumentException {
public BytesWritable evaluate(BytesWritable wkb, int wkid) throws
UDFArgumentException {
try {
- SpatialReference spatialReference = null;
- if (wkid != GeometryUtils.WKID_UNKNOWN) {
- spatialReference = SpatialReference.create(wkid);
- }
- byte[] byteArr = wkb.getBytes();
- ByteBuffer byteBuf = ByteBuffer.allocate(byteArr.length);
- byteBuf.put(byteArr);
- OGCGeometry ogcObj = OGCGeometry.fromBinary(byteBuf);
- ogcObj.setSpatialReference(spatialReference);
- if (ogcObj.geometryType().equals("Point")) {
- return GeometryUtils.geometryToEsriShapeBytesWritable(ogcObj);
+ byte[] bytes = wkb.getBytes();
+ Geometry geom = GeometryUtils.wkbReader().read(bytes);
Review Comment:
BytesWritable#getBytes() returns the backing array which may be larger than
the valid payload; passing it directly to WKBReader can include trailing
garbage bytes and cause parse failures. Use getLength() to bound/copy the byte
array before parsing.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_LineFromWKB.java:
##########
@@ -55,17 +52,11 @@ public BytesWritable evaluate(BytesWritable wkb) throws
UDFArgumentException {
public BytesWritable evaluate(BytesWritable wkb, int wkid) throws
UDFArgumentException {
try {
- SpatialReference spatialReference = null;
- if (wkid != GeometryUtils.WKID_UNKNOWN) {
- spatialReference = SpatialReference.create(wkid);
- }
- byte[] byteArr = wkb.getBytes();
- ByteBuffer byteBuf = ByteBuffer.allocate(byteArr.length);
- byteBuf.put(byteArr);
- OGCGeometry ogcObj = OGCGeometry.fromBinary(byteBuf);
- ogcObj.setSpatialReference(spatialReference);
- if (ogcObj.geometryType().equals("LineString")) {
- return GeometryUtils.geometryToEsriShapeBytesWritable(ogcObj);
+ byte[] bytes = wkb.getBytes();
+ Geometry geom = GeometryUtils.wkbReader().read(bytes);
Review Comment:
BytesWritable#getBytes() returns the backing array which may include extra
unused bytes; passing it directly to WKBReader can corrupt parsing. Bound the
input using getLength() (or wrap with offset/length) before reading.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_GeomFromWKB.java:
##########
@@ -76,16 +73,12 @@ public BytesWritable evaluate(BytesWritable wkb) throws
UDFArgumentException {
public BytesWritable evaluate(BytesWritable wkb, int wkid) throws
UDFArgumentException {
try {
- SpatialReference spatialReference = null;
+ byte[] byteArr = wkb.getBytes();
+ Geometry geom = GeometryUtils.wkbReader().read(byteArr);
Review Comment:
BytesWritable#getBytes() returns the backing array; using it directly can
include bytes beyond wkb.getLength() and break WKB parsing. Copy/slice the
array to the correct length first.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_Is3D.java:
##########
@@ -61,13 +62,14 @@ public BooleanWritable 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;
}
- resultBoolean.set(ogcGeometry.is3D());
+ Coordinate coord = geom.getCoordinate();
+ resultBoolean.set(coord != null && !Double.isNaN(coord.getZ()));
Review Comment:
ST_Is3D currently checks only geom.getCoordinate().getZ(), which can be
misleading for GeometryCollections and also doesn't mirror the repo's dimension
detection logic that guards against NaN Z. Prefer using
GeometryUtils.getOrdinates(geom) to detect whether Z is actually present.
##########
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);
+ byte[] bytes = shape.getBytes();
+ ByteBuffer shapeBuffer =
ByteBuffer.wrap(bytes).order(ByteOrder.LITTLE_ENDIAN);
Review Comment:
BytesWritable#getBytes() returns the backing array; wrapping the whole array
can include bytes beyond shape.getLength() and confuse Esri-shape parsing. Wrap
only the valid slice using offset/length.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_IsMeasured.java:
##########
@@ -61,13 +62,14 @@ public BooleanWritable 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;
}
- resultBoolean.set(ogcGeometry.isMeasured());
+ Coordinate coord = geom.getCoordinate();
+ resultBoolean.set(coord != null && !Double.isNaN(coord.getM()));
Review Comment:
ST_IsMeasured currently checks only geom.getCoordinate().getM(), which can
be incorrect for multi-geometries and doesn't match the codebase's ordinate
detection helper. Use GeometryUtils.getOrdinates(geom) so measured-ness is
detected consistently.
##########
serde/src/java/org/apache/hadoop/hive/serde2/esriJson/serializer/GeometryJsonSerializer.java:
##########
@@ -17,19 +17,32 @@
*/
package org.apache.hadoop.hive.serde2.esriJson.serializer;
-import com.esri.core.geometry.Geometry;
import com.esri.core.geometry.GeometryEngine;
+import com.esri.core.geometry.ogc.OGCGeometry;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.databind.JsonSerializer;
import com.fasterxml.jackson.databind.SerializerProvider;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.io.WKBWriter;
import java.io.IOException;
+/**
+ * Serializes a JTS Geometry to Esri JSON format.
+ * Converts JTS -> ESRI via WKB round-trip, then uses
GeometryEngine.geometryToJson().
+ */
public class GeometryJsonSerializer extends JsonSerializer<Geometry> {
@Override
public void serialize(Geometry geometry, JsonGenerator jsonGenerator,
SerializerProvider arg2) throws IOException {
-
- jsonGenerator.writeRawValue(GeometryEngine.geometryToJson(null, geometry));
+ try {
+ byte[] wkb = new WKBWriter().write(geometry);
+ OGCGeometry ogcGeom =
OGCGeometry.fromBinary(java.nio.ByteBuffer.wrap(wkb));
+ com.esri.core.geometry.Geometry esriGeom = ogcGeom.getEsriGeometry();
+ int wkid = geometry.getSRID();
+ jsonGenerator.writeRawValue(GeometryEngine.geometryToJson(wkid,
esriGeom));
+ } catch (Exception e) {
+ jsonGenerator.writeNull();
+ }
Review Comment:
This serializer swallows all exceptions and writes JSON null, which can
silently drop geometries during serialization and make failures hard to
diagnose. Prefer failing fast by surfacing the error as an IOException (or at
least logging it) so callers/test output show the root cause.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_MLineFromWKB.java:
##########
@@ -55,18 +52,12 @@ public BytesWritable evaluate(BytesWritable wkb) throws
UDFArgumentException {
public BytesWritable evaluate(BytesWritable wkb, int wkid) throws
UDFArgumentException {
try {
- SpatialReference spatialReference = null;
- if (wkid != GeometryUtils.WKID_UNKNOWN) {
- spatialReference = SpatialReference.create(wkid);
- }
- byte[] byteArr = wkb.getBytes();
- ByteBuffer byteBuf = ByteBuffer.allocate(byteArr.length);
- byteBuf.put(byteArr);
- OGCGeometry ogcObj = OGCGeometry.fromBinary(byteBuf);
- ogcObj.setSpatialReference(spatialReference);
- String gType = ogcObj.geometryType();
+ byte[] bytes = wkb.getBytes();
+ Geometry geom = GeometryUtils.wkbReader().read(bytes);
Review Comment:
BytesWritable#getBytes() may include unused trailing bytes; pass only the
valid slice (getLength()) to WKBReader to avoid parse failures.
##########
ql/src/java/org/apache/hadoop/hive/ql/io/esriJson/EsriFeatureClass.java:
##########
@@ -39,14 +43,19 @@ public class EsriFeatureClass {
public Map<String, Object> fieldAliases;
/**
- * Esri geometry type (Polygon, Point, ...)
+ * Esri geometry type string (e.g. "esriGeometryPoint",
"esriGeometryPolygon").
*/
- public Geometry.Type geometryType;
+ @JsonDeserialize(using = GeometryTypeJsonDeserializer.class)
+ @JsonSerialize(using = GeometryTypeJsonSerializer.class)
+ public String geometryType;
/**
- * Spatial reference for the feature class (null, if undefined)
+ * Spatial reference WKID for the feature class (0 if undefined).
+ * The JSON form is {"wkid": N}; deserialized to a plain int.
*/
- public SpatialReference spatialReference;
+ @JsonDeserialize(using = SpatialReferenceJsonDeserializer.class)
+ @JsonSerialize(using = SpatialReferenceJsonSerializer.class)
+ public int spatialReference;
Review Comment:
With spatialReference as a primitive int defaulting to 0, Jackson will
always serialize it, producing a spatialReference object with wkid=0. That
differs from EsriJsonConverter's convention (wkid==0 means omit
spatialReference) and can produce non-canonical Esri JSON. Consider excluding
default 0 from serialization.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_MPointFromWKB.java:
##########
@@ -55,20 +52,14 @@ public BytesWritable evaluate(BytesWritable wkb) throws
UDFArgumentException {
public BytesWritable evaluate(BytesWritable wkb, int wkid) throws
UDFArgumentException {
try {
- SpatialReference spatialReference = null;
- if (wkid != GeometryUtils.WKID_UNKNOWN) {
- spatialReference = SpatialReference.create(wkid);
- }
- byte[] byteArr = wkb.getBytes();
- ByteBuffer byteBuf = ByteBuffer.allocate(byteArr.length);
- byteBuf.put(byteArr);
- OGCGeometry ogcObj = OGCGeometry.fromBinary(byteBuf);
- ogcObj.setSpatialReference(spatialReference);
- String gType = ogcObj.geometryType();
+ byte[] bytes = wkb.getBytes();
+ Geometry geom = GeometryUtils.wkbReader().read(bytes);
Review Comment:
BytesWritable#getBytes() returns the backing array, not necessarily sized to
the payload; using it directly can feed extra bytes into WKBReader. Copy/slice
using wkb.getLength() first.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_MPolyFromWKB.java:
##########
@@ -55,18 +52,12 @@ public BytesWritable evaluate(BytesWritable wkb) throws
UDFArgumentException {
public BytesWritable evaluate(BytesWritable wkb, int wkid) throws
UDFArgumentException {
try {
- SpatialReference spatialReference = null;
- if (wkid != GeometryUtils.WKID_UNKNOWN) {
- spatialReference = SpatialReference.create(wkid);
- }
- byte[] byteArr = wkb.getBytes();
- ByteBuffer byteBuf = ByteBuffer.allocate(byteArr.length);
- byteBuf.put(byteArr);
- OGCGeometry ogcObj = OGCGeometry.fromBinary(byteBuf);
- ogcObj.setSpatialReference(spatialReference);
- String gType = ogcObj.geometryType();
+ byte[] bytes = wkb.getBytes();
+ Geometry geom = GeometryUtils.wkbReader().read(bytes);
Review Comment:
BytesWritable#getBytes() may include bytes beyond the valid payload length;
pass only the first wkb.getLength() bytes to WKBReader to avoid corrupted
parses.
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/esri/ST_PolyFromWKB.java:
##########
@@ -55,17 +52,11 @@ public BytesWritable evaluate(BytesWritable wkb) throws
UDFArgumentException {
public BytesWritable evaluate(BytesWritable wkb, int wkid) throws
UDFArgumentException {
try {
- SpatialReference spatialReference = null;
- if (wkid != GeometryUtils.WKID_UNKNOWN) {
- spatialReference = SpatialReference.create(wkid);
- }
- byte[] byteArr = wkb.getBytes();
- ByteBuffer byteBuf = ByteBuffer.allocate(byteArr.length);
- byteBuf.put(byteArr);
- OGCGeometry ogcObj = OGCGeometry.fromBinary(byteBuf);
- ogcObj.setSpatialReference(spatialReference);
- if (ogcObj.geometryType().equals("Polygon")) {
- return GeometryUtils.geometryToEsriShapeBytesWritable(ogcObj);
+ byte[] bytes = wkb.getBytes();
+ Geometry geom = GeometryUtils.wkbReader().read(bytes);
Review Comment:
BytesWritable#getBytes() returns the backing buffer which can be larger than
wkb.getLength(); WKBReader should only see the valid portion to avoid parse
errors.
--
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]
