yadavay-amzn commented on code in PR #3607:
URL: https://github.com/apache/parquet-java/pull/3607#discussion_r3447246480
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -970,6 +970,46 @@ public org.apache.parquet.column.statistics.Statistics
fromParquetStatistics(
return fromParquetStatisticsInternal(createdBy, statistics, type,
expectedOrder);
}
+ // Overload that uses a pre-computed shouldIgnoreCorruptStats flag to avoid
redundant parsing
+ private org.apache.parquet.column.statistics.Statistics
fromParquetStatisticsInternal(
Review Comment:
Done. Removed the duplicated body; the `createdBy` overload now delegates to
the boolean overload via
`CorruptStatistics.fileHasCorruptStatistics(createdBy)`.
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -1829,6 +1880,10 @@ public ParquetMetadata fromParquetMetadata(
MessageType messageType = fromParquetSchema(parquetMetadata.getSchema(),
parquetMetadata.getColumn_orders());
List<BlockMetaData> blocks = new ArrayList<BlockMetaData>();
List<RowGroup> row_groups = parquetMetadata.getRow_groups();
+ // Compute once per file: the result is the same for BINARY and
FIXED_LEN_BYTE_ARRAY
+ // (the only types affected by PARQUET-251), and always false for other
types.
+ boolean shouldIgnoreCorruptStats =
+
CorruptStatistics.shouldIgnoreStatistics(parquetMetadata.getCreated_by(),
PrimitiveTypeName.BINARY);
Review Comment:
Done. Added `isCorruptStatisticsColumnType(columnType)` and
`fileHasCorruptStatistics(createdBy)`, with `shouldIgnoreStatistics` delegating
to both. The file-level flag is computed via `fileHasCorruptStatistics` (no
hardcoded BINARY), and the per-column BINARY/FIXED_LEN_BYTE_ARRAY check is
applied where stats are converted.
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
Review Comment:
Done. The precomputed boolean is threaded instead; `createdBy` is no longer
passed downstream.
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -1794,13 +1834,24 @@ public FileMetaDataAndRowGroupOffsetInfo
visit(RangeMetadataFilter filter) throw
public ColumnChunkMetaData buildColumnChunkMetaData(
ColumnMetaData metaData, ColumnPath columnPath, PrimitiveType type,
String createdBy) {
+ boolean shouldIgnoreCorruptStats =
+ CorruptStatistics.shouldIgnoreStatistics(createdBy,
PrimitiveTypeName.BINARY);
+ return buildColumnChunkMetaData(metaData, columnPath, type, createdBy,
shouldIgnoreCorruptStats);
+ }
+
+ ColumnChunkMetaData buildColumnChunkMetaData(
Review Comment:
Done. `buildColumnChunkMetaData` now delegates to a package-private overload
taking the boolean, with the SortOrder computation moved into it; the public
`(..., String createdBy)` overload is preserved for back-compat.
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -970,6 +970,46 @@ public org.apache.parquet.column.statistics.Statistics
fromParquetStatistics(
return fromParquetStatisticsInternal(createdBy, statistics, type,
expectedOrder);
}
+ // Overload that uses a pre-computed shouldIgnoreCorruptStats flag to avoid
redundant parsing
+ private org.apache.parquet.column.statistics.Statistics
fromParquetStatisticsInternal(
+ String createdBy, Statistics formatStats, PrimitiveType type, boolean
shouldIgnoreCorruptStats) {
+ SortOrder typeSortOrder = overrideSortOrderToSigned(type) ?
SortOrder.SIGNED : sortOrder(type);
+ org.apache.parquet.column.statistics.Statistics.Builder statsBuilder =
+
org.apache.parquet.column.statistics.Statistics.getBuilderForReading(type);
+
+ if (formatStats != null) {
+ if (formatStats.isSetMin_value() && formatStats.isSetMax_value()) {
+ byte[] min = formatStats.min_value.array();
+ byte[] max = formatStats.max_value.array();
+ if (isMinMaxStatsSupported(type) || Arrays.equals(min, max)) {
+ statsBuilder.withMin(min);
+ statsBuilder.withMax(max);
+ }
+ } else {
+ boolean isSet = formatStats.isSetMax() && formatStats.isSetMin();
+ boolean maxEqualsMin = isSet ? Arrays.equals(formatStats.getMin(),
formatStats.getMax()) : false;
+ boolean sortOrdersMatch = SortOrder.SIGNED == typeSortOrder;
+ // The shouldIgnoreCorruptStats flag applies only to BINARY and
FIXED_LEN_BYTE_ARRAY.
+ // For other types, shouldIgnoreStatistics always returns false, so we
only guard those.
+ PrimitiveTypeName primitiveTypeName = type.getPrimitiveTypeName();
+ boolean ignoreForThisColumn = shouldIgnoreCorruptStats
+ && (primitiveTypeName == PrimitiveTypeName.BINARY
+ || primitiveTypeName ==
PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY);
+ if (!ignoreForThisColumn && (sortOrdersMatch || maxEqualsMin)) {
Review Comment:
Done. Removed the now-dead column-type check; the gate is expressed via
`CorruptStatistics.isCorruptStatisticsColumnType` in
`fromParquetStatisticsInternal`.
--
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]
