asif-moh commented on code in PR #3607:
URL: https://github.com/apache/parquet-java/pull/3607#discussion_r3400521468
##########
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:
This check in `shouldIgnoreStatistics` is dead code with current changes as
we always pass `BINARY`
```
if (columnType != PrimitiveTypeName.BINARY && columnType
!=PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY)
```
We can utilize the new methods here. Please refer to below comments for
context.
```
if (!(fileHasCorruptStats &&
CorruptStatistics.isCorruptStatisticsColumnType(type.getPrimitiveTypeName()))
```
Instead of calling or moving the `PrimitiveTypeName` checks to
`ParquetMetadataConverter` and leave it as the responsibility of
`CorruptStatistics`
```
if (!CorruptStatistics.shouldIgnoreStatistics(createdBy,
type.getPrimitiveTypeName())
```
##########
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:
This is calling `shouldIgnoreStatistics` with `PrimitiveTypeName.BINARY`
hardcoded which is incorrect.
Instead we can refactor `shouldIgnoreStatistics` by adding public methods.
```
public static boolean shouldIgnoreStatistics(String createdBy,
PrimitiveTypeName columnType) {
if (!isCorruptStatisticsColumnType(columnType)) {
// the bug only applies to binary columns
return false;
}
return fileHasCorruptStatistics(createdBy);
}
public static boolean isCorruptStatisticsColumnType(PrimitiveTypeName
columnType) {
return columnType == PrimitiveTypeName.BINARY || columnType ==
PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY;
}
public static boolean fileHasCorruptStatistics(String createdBy) {
// rest of the logic from shouldIgnoreStatistics
}
```
##########
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:
Instead of duplicating the entire `fromParquetStatisticsInternal` body, the
existing method can simply delegate to a new overload and this eliminates
duplicate code
```
static org.apache.parquet.column.statistics.Statistics
fromParquetStatisticsInternal(
String createdBy, Statistics formatStats, PrimitiveType type,
SortOrder typeSortOrder) {
return fromParquetStatisticsInternal(
formatStats,
type,
typeSortOrder,
CorruptStatistics.fileHasCorruptStatistics(createdBy) // This is a
new method in CorruptStatistics
);
}
// overloaded method
static org.apache.parquet.column.statistics.Statistics
fromParquetStatisticsInternal(
Statistics formatStats, PrimitiveType type, SortOrder typeSortOrder,
boolean fileHasCorruptStats) {
```
##########
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:
`buildColumnChunkMetaData` can delegate to a package-private overload that
takes the boolean similar to what you have done but with few changes,
```
public ColumnChunkMetaData buildColumnChunkMetaData(
ColumnMetaData metaData, ColumnPath columnPath, PrimitiveType type,
String createdBy) {
return buildColumnChunkMetaData(
metaData, columnPath, type,
CorruptStatistics.fileHasCorruptStatistics(createdBy));
}
ColumnChunkMetaData buildColumnChunkMetaData(
ColumnMetaData metaData, ColumnPath columnPath, PrimitiveType type,
boolean fileHasCorruptStats) {
SortOrder expectedOrder = overrideSortOrderToSigned(type) ?
SortOrder.SIGNED : sortOrder(type);
return ColumnChunkMetaData.get(...,
fromParquetStatisticsInternal(metaData.statistics, type,
expectedOrder, fileHasCorruptStats), ...);
}
```
No need to pass createdBy downstream, the boolean is all the internal
overload needs. SortOrder computation moves here since we bypass
`fromParquetStatistics` to avoid re-parsing createdBy as you have already done
by replacing `fromParquetStatisticsInternal` with `fromParquetStatistics`.
Also notice how the new public methods we extracted in `CorruptStatistics`
are being used in each delegate method here
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
Review Comment:
We no longer need to pass `createdBy` downstream
--
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]
