Xikui Wang has posted comments on this change. Change subject: [ASTERIXDB-1371][FUN][AQL][SQL] Add standard geometry data type and functions ......................................................................
Patch Set 6: (36 comments) It's a very nice patch, and there are so many exciting new features! I left some comments on the general code style and format. Haven't got the time to look into the implementation details yet. :( Key points are: fix import *; use error code properly. One general question, do we have enough test cases to cover all the new functions? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/geojson/datatype/primitive.4.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/geojson/datatype/primitive.4.query.sqlpp: Line 21: SELECT VALUE {"Type": geometry_type(geo.myGeometry), "Area": st_area(geo.myGeometry), "Coordinate dimension": st_coord_dim(geo.myGeometry), "Dimension":st_dimension(geo.myGeometry), "NPoints":st_n_points(geo.myGeometry), "XMax":st_x_max(geo.myGeometry),"XMin":st_x_min(geo.myGeometry), "YMax":st_y_max(geo.myGeometry), "YMin":st_y_min(geo.myGeometry), "Binary": st_as_binary(geo.myGeometry), "GeoJSON":st_as_geojson(geo.myGeometry),"WKT":st_as_text(geo.myGeometry)} FROM Geometries geo; Let's trim the long lines into several lines for easier reading/editing. https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-app/src/test/resources/runtimets/results/geojson/datatype/datatype.3.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/geojson/datatype/datatype.3.adm: Line 2: { "Geometries": { "id": 135, "myGeometry": {"type":"LineString","coordinates":[[1,2],[4,5],[7,8]],"crs":{"type":"name","properties":{"name":"EPSG:4326"}}} } } Maybe order the result by type or id? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-app/src/test/resources/runtimets/results/geojson/datatype/result.12.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/geojson/datatype/result.12.adm: Line 1: { "PointN": {"type":"Point","coordinates":[-69.199136,-12.599842],"crs":{"type":"name","properties":{"name":"EPSG:4326"}}}, "StartPoint": {"type":"Point","coordinates":[-69.1991349,-12.6006222],"crs":{"type":"name","properties":{"name":"EPSG:4326"}}}, "Envelope": {"type":"Polygon","coordinates":[[[-69.199136,-12.6010968],[-69.1972972,-12.6010968],[-69.1972972,-12.5998133],[-69.199136,-12.5998133],[-69.199136,-12.6010968]]],"crs":{"type":"name","properties":{"name":"EPSG:4326"}}} } It would be nice to split this big test case into several small test cases, one for each test point. That might be easier for debugging. Also, it would be better to match the names for test queries files and results, as part of the coding style. https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java: Line 724: //Geo space https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/STUnionAggregateFunction.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/STUnionAggregateFunction.java: Line 104: throw new NullPointerException("st_union: Geometry type encountered was null"); Maybe use this UnsupportedItemTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractSTDoubleGeometryDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractSTDoubleGeometryDescriptor.java: Line 40: import java.io.*; replace * from list of import Line 63: @Override @SuppressWarnings("unchecked") code format https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractSTGeometryNDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractSTGeometryNDescriptor.java: Line 38: import java.io.*; same https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractSTSingleGeometryDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractSTSingleGeometryDescriptor.java: Line 40: import java.io.*; same https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STEndPointDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STEndPointDescriptor.java: Line 45: throw new UnsupportedOperationException( let's use error code for this maybe? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STExteriorRingDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STExteriorRingDescriptor.java: Line 46: "Only Polygon is supported by this function"); errorcode? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STGeomFromTextDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STGeomFromTextDescriptor.java: Line 44: import java.io.*; same https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STGeomFromTextSRIDDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STGeomFromTextSRIDDescriptor.java: Line 45: import java.io.*; same :) Line 112: structure = wktImporter.executeOGC(WktImportFlags.wktImportNonTrusted, geometry, null); maybe one comment for why not rethrowing the exception? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STGeometryNDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STGeometryNDescriptor.java: Line 51: throw new UnsupportedOperationException( try UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STInteriorRingNDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STInteriorRingNDescriptor.java: Line 51: throw new UnsupportedOperationException( try UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STIsClosedDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STIsClosedDescriptor.java: Line 21: import com.esri.core.geometry.ogc.*; fix import * Line 63: throw new UnsupportedOperationException("The " + geometry.geometryType() + " type is not supported"); UnsupportedTypeException https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STIsRingDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STIsRingDescriptor.java: Line 45: throw new UnsupportedOperationException( UnsupportedTypeException https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STLengthDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STLengthDescriptor.java: Line 57: throw new HyracksDataException("This function expects a LineString or a MultilineString geometry type"); errorcode? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STLineFromMultiPointDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STLineFromMultiPointDescriptor.java: Line 56: throw new HyracksDataException("This function expects a MultiPoint geometry type"); UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STMDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STMDescriptor.java: Line 45: throw new HyracksDataException("This function expects a point geometry type"); UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STNPointsDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STNPointsDescriptor.java: Line 66: throw new HyracksDataException("The number of vertices cannot be calculated"); errorcode? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STNRingsDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STNRingsDescriptor.java: Line 56: throw new HyracksDataException("This function expects a Polygon or a MultiPolygon geometry type"); errorcode https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STNumInteriorRingsDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STNumInteriorRingsDescriptor.java: Line 45: throw new HyracksDataException("This function expects a point geometry type"); errorcode https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STPointNDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STPointNDescriptor.java: Line 51: throw new UnsupportedOperationException( UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STPolygonizeDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STPolygonizeDescriptor.java: Line 51: import java.io.*; fix import * https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STRelateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STRelateDescriptor.java: Line 45: import java.io.*; fix import * https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STStartPointDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STStartPointDescriptor.java: Line 45: throw new UnsupportedOperationException( UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STXDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STXDescriptor.java: Line 45: throw new HyracksDataException("This function expects a point geometry type"); UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STXMaxDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STXMaxDescriptor.java: Line 50: "This method is not spported for the geometry type: " + geometry.geometryType()); UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STXMinDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STXMinDescriptor.java: Line 50: "This method is not spported for the geometry type: " + geometry.geometryType()); UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STYDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STYDescriptor.java: Line 45: throw new HyracksDataException("This function expects a point geometry type"); UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STYMaxDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STYMaxDescriptor.java: Line 50: "This method is not spported for the geometry type: " + geometry.geometryType()); UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STYMinDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STYMinDescriptor.java: Line 50: "This method is not spported for the geometry type: " + geometry.geometryType()); UnsupportedTypeException? https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STZDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/geo/STZDescriptor.java: Line 45: throw new HyracksDataException("This function expects a point geometry type"); use mismatch or unsupported? -- To view, visit https://asterix-gerrit.ics.uci.edu/2056 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9cddeffea42e85469b6fc38f361bd98e64025289 Gerrit-PatchSet: 6 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ahmed Eldawy <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]> Gerrit-HasComments: Yes
