szehon-ho commented on code in PR #16850:
URL: https://github.com/apache/iceberg/pull/16850#discussion_r3431828640
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetrics.java:
##########
@@ -264,13 +266,19 @@ private FieldMetrics<ByteBuffer> metricsFromFooter(
int truncateLength) {
if (primitive.getPrimitiveTypeName() ==
PrimitiveType.PrimitiveTypeName.INT96) {
return null;
- } else if (truncateLength <= 0) {
+ } else if (truncateLength <= 0 || isGeospatial(icebergType)) {
+ // Parquet lexicographic min/max is not meaningful for spatial WKB.
return counts(fieldId);
} else {
return bounds(fieldId, icebergType, primitive, truncateLength);
}
}
+ private static boolean
isGeospatial(org.apache.iceberg.types.Type.PrimitiveType icebergType) {
+ TypeID typeId = Preconditions.checkNotNull(icebergType, "Invalid type:
null").typeId();
Review Comment:
`isGeospatial` throws via `checkNotNull` when `icebergType` is null, and it
runs before `bounds(...)`. But `bounds()` has an existing null guard (`if
(icebergType == null) return null;`) that degrades gracefully to empty metrics,
implying null was a tolerated case in the footer path. With this change, a null
type at `truncateLength > 0` now throws instead of returning null.
Is null actually reachable here? If yes, consider `icebergType != null &&
(typeId == GEOMETRY || typeId == GEOGRAPHY)` to preserve the graceful fallback.
If not, the null guard in `bounds()` is now dead code and could be removed for
clarity.
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetrics.java:
##########
@@ -264,13 +266,19 @@ private FieldMetrics<ByteBuffer> metricsFromFooter(
int truncateLength) {
if (primitive.getPrimitiveTypeName() ==
PrimitiveType.PrimitiveTypeName.INT96) {
return null;
- } else if (truncateLength <= 0) {
+ } else if (truncateLength <= 0 || isGeospatial(icebergType)) {
+ // Parquet lexicographic min/max is not meaningful for spatial WKB.
return counts(fieldId);
} else {
return bounds(fieldId, icebergType, primitive, truncateLength);
}
}
+ private static boolean
isGeospatial(org.apache.iceberg.types.Type.PrimitiveType icebergType) {
Review Comment:
Nit (optional, no change needed): the fully-qualified
`org.apache.iceberg.types.Type.PrimitiveType` is unavoidable here since both
`Type` and `PrimitiveType` simple names are already taken by the Parquet
imports, and it's consistent with the other methods in this file. If you did
want to drop the qualification, this helper only needs the `TypeID` (already
imported as a simple name), so it could take a `TypeID` param and be called as
`isGeospatial(icebergType.typeId())`.
--
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]
