wgtmac commented on code in PR #3200:
URL: https://github.com/apache/parquet-java/pull/3200#discussion_r2063877617
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -520,6 +523,35 @@ public Optional<LogicalType>
visit(LogicalTypeAnnotation.IntervalLogicalTypeAnno
public Optional<LogicalType>
visit(LogicalTypeAnnotation.VariantLogicalTypeAnnotation variantLogicalType) {
return of(LogicalTypes.VARIANT(variantLogicalType.getSpecVersion()));
}
+
+ @Override
+ public Optional<LogicalType>
visit(LogicalTypeAnnotation.GeometryLogicalTypeAnnotation geometryLogicalType) {
+ GeometryType geometryType = new GeometryType();
+ if (geometryLogicalType.getCrs() != null) {
Review Comment:
```suggestion
if (geometryLogicalType.getCrs() != null &&
!geometryLogicalType.getCrs().isEmpty()) {
```
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -520,6 +523,35 @@ public Optional<LogicalType>
visit(LogicalTypeAnnotation.IntervalLogicalTypeAnno
public Optional<LogicalType>
visit(LogicalTypeAnnotation.VariantLogicalTypeAnnotation variantLogicalType) {
return of(LogicalTypes.VARIANT(variantLogicalType.getSpecVersion()));
}
+
+ @Override
+ public Optional<LogicalType>
visit(LogicalTypeAnnotation.GeometryLogicalTypeAnnotation geometryLogicalType) {
+ GeometryType geometryType = new GeometryType();
+ if (geometryLogicalType.getCrs() != null) {
+ geometryType.setCrs(geometryLogicalType.getCrs());
+ }
+ return of(LogicalType.GEOMETRY(geometryType));
+ }
+
+ @Override
+ public Optional<LogicalType>
visit(LogicalTypeAnnotation.GeographyLogicalTypeAnnotation
geographyLogicalType) {
+ GeographyType geographyType = new GeographyType();
+ if (geographyLogicalType.getCrs() != null) {
+ geographyType.setCrs(geographyLogicalType.getCrs());
+ }
+ if (geographyLogicalType.getAlgorithm() != null) {
+ try {
+ // Convert from schema.EdgeInterpolationAlgorithm to
format.EdgeInterpolationAlgorithm
+ EdgeInterpolationAlgorithm algorithm =
EdgeInterpolationAlgorithm.valueOf(
+ geographyLogicalType.getAlgorithm().name());
+ geographyType.setAlgorithm(algorithm);
+ } catch (IllegalArgumentException e) {
Review Comment:
Why not simply removing the catch since you throw the same exception here?
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -1183,6 +1215,24 @@ LogicalTypeAnnotation
getLogicalTypeAnnotation(LogicalType type) {
return LogicalTypeAnnotation.uuidType();
case FLOAT16:
return LogicalTypeAnnotation.float16Type();
+ case GEOMETRY:
+ GeometryType geometry = type.getGEOMETRY();
+ return LogicalTypeAnnotation.geometryType(geometry.getCrs());
+ case GEOGRAPHY:
+ GeographyType geography = type.getGEOGRAPHY();
+ // Handle when either algorithm or CRS is null
+ if (geography == null) {
+ return LogicalTypeAnnotation.geographyType(null, null);
+ }
+
+ EdgeInterpolationAlgorithm algorithm = geography.getAlgorithm();
+ org.apache.parquet.column.schema.EdgeInterpolationAlgorithm
parquetAlgorithm = null;
+ if (algorithm != null) {
+ parquetAlgorithm =
+
org.apache.parquet.column.schema.EdgeInterpolationAlgorithm.valueOf(algorithm.name());
Review Comment:
Same as above. We'd better implement a direct conversion.
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -520,6 +523,35 @@ public Optional<LogicalType>
visit(LogicalTypeAnnotation.IntervalLogicalTypeAnno
public Optional<LogicalType>
visit(LogicalTypeAnnotation.VariantLogicalTypeAnnotation variantLogicalType) {
return of(LogicalTypes.VARIANT(variantLogicalType.getSpecVersion()));
}
+
+ @Override
+ public Optional<LogicalType>
visit(LogicalTypeAnnotation.GeometryLogicalTypeAnnotation geometryLogicalType) {
+ GeometryType geometryType = new GeometryType();
+ if (geometryLogicalType.getCrs() != null) {
+ geometryType.setCrs(geometryLogicalType.getCrs());
+ }
+ return of(LogicalType.GEOMETRY(geometryType));
+ }
+
+ @Override
+ public Optional<LogicalType>
visit(LogicalTypeAnnotation.GeographyLogicalTypeAnnotation
geographyLogicalType) {
+ GeographyType geographyType = new GeographyType();
+ if (geographyLogicalType.getCrs() != null) {
+ geographyType.setCrs(geographyLogicalType.getCrs());
+ }
+ if (geographyLogicalType.getAlgorithm() != null) {
+ try {
+ // Convert from schema.EdgeInterpolationAlgorithm to
format.EdgeInterpolationAlgorithm
+ EdgeInterpolationAlgorithm algorithm =
EdgeInterpolationAlgorithm.valueOf(
Review Comment:
Can we add a direct conversion method between thrift and parquet
EdgeInterpolationAlgorithm in this file? String conversion looks inefficient.
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -520,6 +523,35 @@ public Optional<LogicalType>
visit(LogicalTypeAnnotation.IntervalLogicalTypeAnno
public Optional<LogicalType>
visit(LogicalTypeAnnotation.VariantLogicalTypeAnnotation variantLogicalType) {
return of(LogicalTypes.VARIANT(variantLogicalType.getSpecVersion()));
}
+
+ @Override
+ public Optional<LogicalType>
visit(LogicalTypeAnnotation.GeometryLogicalTypeAnnotation geometryLogicalType) {
Review Comment:
Thanks for adding the test!
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -1183,6 +1215,24 @@ LogicalTypeAnnotation
getLogicalTypeAnnotation(LogicalType type) {
return LogicalTypeAnnotation.uuidType();
case FLOAT16:
return LogicalTypeAnnotation.float16Type();
+ case GEOMETRY:
+ GeometryType geometry = type.getGEOMETRY();
+ return LogicalTypeAnnotation.geometryType(geometry.getCrs());
+ case GEOGRAPHY:
+ GeographyType geography = type.getGEOGRAPHY();
+ // Handle when either algorithm or CRS is null
+ if (geography == null) {
+ return LogicalTypeAnnotation.geographyType(null, null);
+ }
Review Comment:
```suggestion
```
They looks strange here.
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -520,6 +523,35 @@ public Optional<LogicalType>
visit(LogicalTypeAnnotation.IntervalLogicalTypeAnno
public Optional<LogicalType>
visit(LogicalTypeAnnotation.VariantLogicalTypeAnnotation variantLogicalType) {
return of(LogicalTypes.VARIANT(variantLogicalType.getSpecVersion()));
}
+
+ @Override
+ public Optional<LogicalType>
visit(LogicalTypeAnnotation.GeometryLogicalTypeAnnotation geometryLogicalType) {
+ GeometryType geometryType = new GeometryType();
+ if (geometryLogicalType.getCrs() != null) {
+ geometryType.setCrs(geometryLogicalType.getCrs());
+ }
+ return of(LogicalType.GEOMETRY(geometryType));
+ }
+
+ @Override
+ public Optional<LogicalType>
visit(LogicalTypeAnnotation.GeographyLogicalTypeAnnotation
geographyLogicalType) {
+ GeographyType geographyType = new GeographyType();
+ if (geographyLogicalType.getCrs() != null) {
Review Comment:
```suggestion
if (geographyLogicalType.getCrs() != null &&
!geographyLogicalType.getCrs().isEmpty()) {
```
--
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]
