You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sedona.apache.org by GitBox <gi...@apache.org> on 2022/09/11 11:20:58 UTC
[GitHub] [incubator-sedona] Kimahriman commented on a diff in pull request #686: [SEDONA-125]Allows customized CRS in ST_Transform
Kimahriman commented on code in PR #686:
URL: https://github.com/apache/incubator-sedona/pull/686#discussion_r967808456
##########
common/src/main/java/org/apache/sedona/common/Functions.java:
##########
@@ -142,12 +143,53 @@ public static Geometry transform(Geometry geometry, String sourceCRS, String tar
public static Geometry transform(Geometry geometry, String sourceCRS, String targetCRS, boolean lenient)
throws FactoryException, TransformException {
- CoordinateReferenceSystem sourceCRScode = CRS.decode(sourceCRS);
- CoordinateReferenceSystem targetCRScode = CRS.decode(targetCRS);
+
+ CoordinateReferenceSystem sourceCRScode = parseCRSString(sourceCRS);
+ CoordinateReferenceSystem targetCRScode = parseCRSString(targetCRS);
MathTransform transform = CRS.findMathTransform(sourceCRScode, targetCRScode, lenient);
return JTS.transform(geometry, transform);
}
+ private static CoordinateReferenceSystem parseCRSString(String CRSString) throws FactoryException{
+ if (checkCRSCodeFormat(CRSString) ){
+ return CRS.decode(CRSString);
+ }
+ else if (checkCRSWKTFormat(CRSString)){
+ return CRS.parseWKT(CRSString);
+ }
Review Comment:
Since these check methods just try to do the decoding, should this just be something like
```suggestion
try {
return CRS.decode(CRSString);
} catch (FactoryException ex) {
try {
return CRS.parseWKT(CRSString);
} catch (FactoryException ex2) {
// ...raise some exception maybe from one or the other or some custom thing explaining can't be parsed
}
```
##########
flink/src/main/java/org/apache/sedona/flink/expressions/Functions.java:
##########
@@ -115,6 +115,14 @@ public Geometry eval(@DataTypeHint(value = "RAW", bridgedTo = org.locationtech.j
Geometry geom = (Geometry) o;
return org.apache.sedona.common.Functions.transform(geom, sourceCRS, targetCRS);
}
+
+ @DataTypeHint(value = "RAW", bridgedTo = org.locationtech.jts.geom.Geometry.class)
+ public Geometry eval(@DataTypeHint(value = "RAW", bridgedTo = org.locationtech.jts.geom.Geometry.class) Object o, @DataTypeHint("String") String sourceCRS, @DataTypeHint("String") String targetCRS,@DataTypeHint("Boolean") Boolean lenient)
+ throws FactoryException, TransformException {
+ Geometry geom = (Geometry) o;
+ return org.apache.sedona.common.Functions.transform(geom, sourceCRS, targetCRS,lenient);
+ }
Review Comment:
Nit: spacing
```suggestion
public Geometry eval(@DataTypeHint(value = "RAW", bridgedTo = org.locationtech.jts.geom.Geometry.class) Object o, @DataTypeHint("String") String sourceCRS, @DataTypeHint("String") String targetCRS, @DataTypeHint("Boolean") Boolean lenient)
throws FactoryException, TransformException {
Geometry geom = (Geometry) o;
return org.apache.sedona.common.Functions.transform(geom, sourceCRS, targetCRS, lenient);
}
```
##########
common/src/main/java/org/apache/sedona/common/Functions.java:
##########
@@ -142,12 +143,53 @@ public static Geometry transform(Geometry geometry, String sourceCRS, String tar
public static Geometry transform(Geometry geometry, String sourceCRS, String targetCRS, boolean lenient)
throws FactoryException, TransformException {
- CoordinateReferenceSystem sourceCRScode = CRS.decode(sourceCRS);
- CoordinateReferenceSystem targetCRScode = CRS.decode(targetCRS);
+
+ CoordinateReferenceSystem sourceCRScode = parseCRSString(sourceCRS);
+ CoordinateReferenceSystem targetCRScode = parseCRSString(targetCRS);
MathTransform transform = CRS.findMathTransform(sourceCRScode, targetCRScode, lenient);
return JTS.transform(geometry, transform);
}
+ private static CoordinateReferenceSystem parseCRSString(String CRSString) throws FactoryException{
+ if (checkCRSCodeFormat(CRSString) ){
+ return CRS.decode(CRSString);
+ }
+ else if (checkCRSWKTFormat(CRSString)){
+ return CRS.parseWKT(CRSString);
+ }
+ else {
+ return null;
+ }
Review Comment:
Is this going to cause NPEs instead of throwing an exception explaining the string can't be parsed as a CRS?
##########
sql/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala:
##########
@@ -202,19 +206,17 @@ case class ST_Transform(inputExpressions: Seq[Expression])
override def eval(input: InternalRow): Any = {
val geometry = inputExpressions(0).toGeometry(input)
- val sourceCRS = inputExpressions(1).asString(input)
- val targetCRS = inputExpressions(2).asString(input)
+ val sourceCRSString = inputExpressions(1).asString(input)
+ val targetCRSString = inputExpressions(2).asString(input)
- (geometry, sourceCRS, targetCRS) match {
- case (geometry: Geometry, sourceCRS: String, targetCRS: String) =>
- val lenient = if (inputExpressions.length == 4) {
- inputExpressions(3).eval(input).asInstanceOf[Boolean]
- } else {
- false
- }
- Functions.transform(geometry, sourceCRS, targetCRS, lenient).toGenericArrayData
- case (_, _, _) => null
+ val lenient = if (inputExpressions.length == 4) {
+ inputExpressions(3).eval(input).asInstanceOf[Boolean]
+ } else {
+ false
}
+
+ (Functions transform(geometry, sourceCRSString, targetCRSString, lenient)).toGenericArrayData
Review Comment:
Nit: just use dot notation like everywhere else
```suggestion
Functions.transform(geometry, sourceCRSString, targetCRSString, lenient).toGenericArrayData
```
##########
common/src/main/java/org/apache/sedona/common/Functions.java:
##########
@@ -142,12 +143,53 @@ public static Geometry transform(Geometry geometry, String sourceCRS, String tar
public static Geometry transform(Geometry geometry, String sourceCRS, String targetCRS, boolean lenient)
throws FactoryException, TransformException {
- CoordinateReferenceSystem sourceCRScode = CRS.decode(sourceCRS);
- CoordinateReferenceSystem targetCRScode = CRS.decode(targetCRS);
+
+ CoordinateReferenceSystem sourceCRScode = parseCRSString(sourceCRS);
+ CoordinateReferenceSystem targetCRScode = parseCRSString(targetCRS);
MathTransform transform = CRS.findMathTransform(sourceCRScode, targetCRScode, lenient);
return JTS.transform(geometry, transform);
}
+ private static CoordinateReferenceSystem parseCRSString(String CRSString) throws FactoryException{
+ if (checkCRSCodeFormat(CRSString) ){
+ return CRS.decode(CRSString);
+ }
+ else if (checkCRSWKTFormat(CRSString)){
+ return CRS.parseWKT(CRSString);
+ }
+ else {
+ return null;
+ }
+ }
+
+ private static boolean checkCRSCodeFormat(String CRSCode) {
+ try{
+ CRS.decode(CRSCode);
+ }
+ catch (FactoryException ex){
+ return false;
+ }
+
+ return true;
+ }
+
+ private static boolean checkCRSWKTFormat(String CRSWKT) {
+ try{
+ CRS.parseWKT(CRSWKT);
+ }
+ catch (FactoryException ex){
+ return false;
+ }
+
+ return true;
+ }
+
+ public static Geometry transform(Geometry geometry, CoordinateReferenceSystem sourceCRS, CoordinateReferenceSystem targetCRS, boolean lenient)
+ throws FactoryException,TransformException {
+ MathTransform transform = CRS.findMathTransform(sourceCRS,targetCRS,lenient);
+ return JTS.transform(geometry, transform);
+ }
Review Comment:
Is this used anywhere?
--
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: dev-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org