Copilot commented on code in PR #3607:
URL: https://github.com/apache/parquet-java/pull/3607#discussion_r3378131980
##########
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:
`shouldIgnoreCorruptStats` is parsed unconditionally once per file using
`PrimitiveTypeName.BINARY`. For files with no BINARY/FIXED_LEN_BYTE_ARRAY
columns, this adds avoidable work and can log PARQUET-251 warnings even though
the result is never used. Consider only parsing `created_by` if the schema
contains at least one affected column type.
##########
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);
Review Comment:
`buildColumnChunkMetaData(..., PrimitiveType type, String createdBy)` now
always calls `CorruptStatistics.shouldIgnoreStatistics(createdBy, BINARY)` even
for non-binary columns. This forces expensive `created_by` parsing (and may
emit PARQUET-251 warnings) for types that were previously a fast no-op. Compute
the flag only when the column is BINARY/FIXED_LEN_BYTE_ARRAY; otherwise keep it
`false`.
##########
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.
Review Comment:
The PR description/title mention adding a bounded process-wide cache and new
cache-focused tests, but this change set implements a per-file precomputation
(and no new cache/test additions are present in the diff). Please align the PR
description/testing notes with the actual implementation (or include the
missing cache/test changes if they are intended).
--
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]
