Ahmed Eldawy 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)

I handled the comments raised in that pull request. I pushed the updated code 
but it got assigned a new ID. I don't know why this happened as I'm not very 
familiar with gerrit yet. You can find the updated code here 
https://asterix-gerrit.ics.uci.edu/#/c/2347/

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.
Done


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?
Done! Sorted by ID


https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/AGeometrySerializerDeserializer.java
File 
asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/AGeometrySerializerDeserializer.java:

PS6, Line 49:             OGCGeometry geometry = OGCGeometry
            :                     
.createFromOGCStructure(OperatorImportFromWkb.local().executeOGC(0, buffer, 
null),
            :                             SpatialReference.create(4326))
> What are the 0, null, and 4326 here for? Can we get them defined as static 
The parameter 0 uses the default import flags in JTS.
null is a progress tracker which allows canceling or tracking the running time 
of a lengthy operation. Currently, it is not used but it can be added in the 
future if we need to terminate or kill a job.
4326 is the ID of the WGS84 coordinate reference system, a.k.a. latitude and 
longitude. http://spatialreference.org/ref/epsg/wgs-84/


https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/AGeometry.java
File 
asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/AGeometry.java:

Line 73:             throw new RuntimeException(e);
> +1, should throw something else preferably...
We're bound here to the signature of JSONSerializable#toJSON which does not 
declare any exceptions to be thrown. We cannot throw any of the Hyracks 
exceptions because they all extend HyracksException which is a type of 
IOException. We can only throw one of the RuntimeExceptions which do not need 
to be declared explicitly.
Is it OK to declare my own exception and make it a subtype of RuntimeException?
As an example, the class 
org.apache.hyracks.algebricks.common.exceptions.NotImplementedException was 
declared in this way.


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
Should I add a space? I found no space in the heading of other sections.


https://asterix-gerrit.ics.uci.edu/#/c/2056/6/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/utils/NonTaggedFormatUtil.java
File 
asterixdb/asterix-om/src/main/java/org/apache/asterix/om/utils/NonTaggedFormatUtil.java:

PS6, Line 200: AInt32SerializerDeserializer
> Wait, what? Why AInt32?
For efficiency, we stored the size of the geometry attribute in the first 
32-bits. I added a comment to explain this.


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:

PS6, Line 66: geometry = new OGCPoint(new Point(), 
SpatialReference.create(4326));
            :     }
> Again what is the 4326 about?
4326 is the ID of the WGS84 CRS which is the default one used by most 
applications (think: latitude and longitude).


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
Done


Line 63:                     @Override @SuppressWarnings("unchecked")
> code format
What is the code format that I should change?


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
Done!


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
Done!


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?
I could not find a constructor to UnsupportedOperationException that takes an 
error code. Do you suggest using another exception or embedding the error code 
in the message?


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
Done!


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 :)
Done!


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?
I think UnsupportedOperationException is more relevant. I changed the error 
message to be compatible.


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?
I changed the error message to be consistent with the exception type.


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 *
I fixed it. However, I want to mention that the code style file in the 
development section does not explicitly specify the maximum number of imports 
before using *. Do you suggest adding it to the style file?


Line 63:             throw new UnsupportedOperationException("The " + 
geometry.geometryType() + " type is not supported");
> UnsupportedTypeException
Changed the error message to be consistent with the exception type.


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
Changed the error message to be consistent with the error type.


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?
Changed to UnsupportedOperationException and updated the error message


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?
Used UnsupportedOperationException and updated the error message to be 
consistent


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?
Used UnsupportedOperationException and updated the error message to be 
consistent


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?
Used UnsupportedOperationException and updated the error message to be 
consistent


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
Used UnsupportedOperationException and updated the error message to be 
consistent


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
Used UnsupportedOperationException and updated the error message to be 
consistent


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?
Used UnsupportedOperationException and updated the error message to be 
consistent


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 *
Fixed


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 *
Done!


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?
Used UnsupportedOperationException and updated the error message to be 
consistent with the error type


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?
Used UnsupportedOperationException and updated the error message to be 
consistent with the error type


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?
Used UnsupportedOperationException and updated the error message to be 
consistent with the error type


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?
Used UnsupportedOperationException and updated the error message to be 
consistent with the error type


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?
Used UnsupportedOperationException and updated the error message to be 
consistent with the error type


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?
Used UnsupportedOperationException and updated the error message to be 
consistent with the error type


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?
Used UnsupportedOperationException and updated the error message to be 
consistent with the error type


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?
Used UnsupportedOperationException and updated the error message to be 
consistent with the error type


-- 
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 <eld...@cs.umn.edu>
Gerrit-Reviewer: Ahmed Eldawy <aseld...@gmail.com>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Ian Maxon <ima...@apache.org>
Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Xikui Wang <xkk...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to