FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-3696: Cache ParsedVersion in FileMetaData and use it in fromParquetMetadata by asifsmohammed · Pull Request #3700 · apache/parquet-java · GitHub

/ parquet-java Public

GH-3696: Cache ParsedVersion in FileMetaData and use it in fromParquetMetadata #3700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asifsmohammed wants to merge 7 commits into
base: master
Choose a base branch
Could not load branches
Branch not found: {{ refName }}

Are you sure you want to change the base?

Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
from
Conversation 27 Commits 7 Checks 0 Files changed
Open

GH-3696: Cache ParsedVersion in FileMetaData and use it in fromParquetMetadata #3700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bc23cf5
GH-3696: Cache ParsedVersion in FileMetaData to eliminate redundant p…
asifsmohammed Aug 1, 2026
51ca7b7
Address review: lazy init for writerVersion
asifsmohammed Aug 2, 2026
b6a87de
Address review: use immutable WriterVersionResult with lazy synchroni…
asifsmohammed Aug 5, 2026
6973b84
Use cached ParsedVersion in fromParquetMetadata
asifsmohammed Aug 5, 2026
8ac10c9
Address review: fix null ambiguity, log parity, and lazy eval
asifsmohammed Aug 21, 2026
17a2870
Address review: retain parse failure, eliminate duplication
asifsmohammed Aug 29, 2026
27ad053
Skip extra delegation hop in fromParquetStatistics(String)
asifsmohammed Aug 29, 2026
File filter

Filter by extension

Filter by extension .java  (7) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
81 changes: 53 additions & 28 deletions parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.apache.parquet;

import java.util.concurrent.atomic.AtomicBoolean;
import org.apache.parquet.SemanticVersion.SemanticVersionParseException;
import org.apache.parquet.VersionParser.ParsedVersion;
import org.apache.parquet.VersionParser.VersionParseException;
import org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName;
Expand Down Expand Up @@ -70,37 +69,63 @@ public static boolean shouldIgnoreStatistics(String createdBy, PrimitiveTypeName

try {
ParsedVersion version = VersionParser.parse(createdBy);
return shouldIgnoreStatistics(version, createdBy, columnType);
} catch (RuntimeException | VersionParseException e) {
// couldn't parse the created_by field, log what went wrong, don't trust the
// stats, but don't make this fatal.
warnParseErrorOnce(createdBy, e);

wgtmac Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
warnParseErrorOnce(createdBy, e);
// couldn't parse the created_by field, log what went wrong, don't trust the
// stats, but don't make this fatal.
warnParseErrorOnce(createdBy, e);

Let's keep the original comment.
return true;
}
}

/**
* Decides if the statistics from a file should be ignored because they are potentially corrupt.
* Use this when the writer version has already been parsed to avoid redundant parsing.
*
* @param writerVersion the pre-parsed writer version, or {@code null} if unknown/unparseable
* @param createdBy the original created-by string from the file footer (used for logging)
* @param columnType the type of the column that this is checking
* @return true if the statistics may be invalid and should be ignored, false otherwise
*/
public static boolean shouldIgnoreStatistics(
ParsedVersion writerVersion, String createdBy, PrimitiveTypeName columnType) {

if (!"parquet-mr".equals(version.application)) {
// assume other applications don't have this bug
return false;
}

if (Strings.isNullOrEmpty(version.version)) {
warnOnce("Ignoring statistics because created_by did not contain a semver (see PARQUET-251): "
+ createdBy);
return true;
}

SemanticVersion semver = SemanticVersion.parse(version.version);

if (semver.compareTo(PARQUET_251_FIXED_VERSION) < 0
&& !(semver.compareTo(CDH_5_PARQUET_251_FIXED_START) >= 0
&& semver.compareTo(CDH_5_PARQUET_251_FIXED_END) < 0)) {
warnOnce("Ignoring statistics because this file was created prior to "
+ PARQUET_251_FIXED_VERSION
+ ", see PARQUET-251");
return true;
}

// this file was created after the fix
if (columnType != PrimitiveTypeName.BINARY && columnType != PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY) {
return false;
} catch (RuntimeException | SemanticVersionParseException | VersionParseException e) {
// couldn't parse the created_by field, log what went wrong, don't trust the stats,
// but don't make this fatal.
warnParseErrorOnce(createdBy, e);
}

if (writerVersion == null) {
warnOnce("Ignoring statistics because created_by is null or empty! See PARQUET-251 and PARQUET-297");
return true;
}

if (!"parquet-mr".equals(writerVersion.application)) {
return false;
}

if (Strings.isNullOrEmpty(writerVersion.version)) {
warnOnce("Ignoring statistics because created_by did not contain a semver (see PARQUET-251): " + createdBy);
return true;
}

if (!writerVersion.hasSemanticVersion()) {

wgtmac Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ParsedVersion has already swallowed SemanticVersionParseException here, so this no longer preserves the old warnParseErrorOnce(createdBy, e) behavior. Could we keep the original string and parse exception for this path?

asifsmohammed Aug 21, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

createdBy string is now passed as a parameter, and the !hasSemanticVersion() branch re-parses writerVersion.version to recreate the SemanticVersionParseException for warnParseErrorOnce(createdBy, e). This gives exact log parity (original string + stack trace). The re-parse only fires when the ParsedVersion fails to parse it, so zero performance impact on the hot path.
warnParseErrorOnce(createdBy, writerVersion.getSemanticVersionParseFailure());
return true;
}

SemanticVersion semver = writerVersion.getSemanticVersion();
Comment on lines +111 to +116

asifsmohammed Aug 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ParsedVersion eagerly parses and caches the SemanticVersion in its constructor, so getSemanticVersion() avoids the redundant SemanticVersion.parse(version.version) that the String-based overload previously performed on every call. The left and right spikes in flame graph are for parsing SemanticVersion twice.

if (semver.compareTo(PARQUET_251_FIXED_VERSION) < 0
&& !(semver.compareTo(CDH_5_PARQUET_251_FIXED_START) >= 0
&& semver.compareTo(CDH_5_PARQUET_251_FIXED_END) < 0)) {
warnOnce("Ignoring statistics because this file was created prior to "
+ PARQUET_251_FIXED_VERSION
+ ", see PARQUET-251");
return true;
}

// this file was created after the fix
return false;
}

private static void warnParseErrorOnce(String createdBy, Throwable e) {
Expand Down
46 changes: 46 additions & 0 deletions parquet-column/src/test/java/org/apache/parquet/CorruptStatisticsTest.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import static org.assertj.core.api.Assertions.assertThat;

import org.apache.parquet.VersionParser.ParsedVersion;
import org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -129,6 +130,51 @@ public void testCorruptStatistics() {
.isFalse();
}

@Test
public void testShouldIgnoreStatisticsWithParsedVersion() throws Exception {
String createdBy = "parquet-mr version 1.6.0 (build abc)";

assertThat(CorruptStatistics.shouldIgnoreStatistics(null, null, PrimitiveTypeName.BINARY))
.isTrue();

assertThat(CorruptStatistics.shouldIgnoreStatistics(null, null, PrimitiveTypeName.INT32))
.isFalse();

ParsedVersion impala = VersionParser.parse("impala version 1.2.0 (build abc)");
assertThat(CorruptStatistics.shouldIgnoreStatistics(
impala, "impala version 1.2.0 (build abc)", PrimitiveTypeName.BINARY))
.isFalse();

ParsedVersion corrupt = VersionParser.parse(createdBy);
assertThat(CorruptStatistics.shouldIgnoreStatistics(corrupt, createdBy, PrimitiveTypeName.BINARY))
.isTrue();

ParsedVersion fixed = VersionParser.parse("parquet-mr version 1.8.0 (build abc)");
assertThat(CorruptStatistics.shouldIgnoreStatistics(
fixed, "parquet-mr version 1.8.0 (build abc)", PrimitiveTypeName.BINARY))
.isFalse();

ParsedVersion newer = VersionParser.parse("parquet-mr version 1.12.0 (build abc)");
assertThat(CorruptStatistics.shouldIgnoreStatistics(
newer, "parquet-mr version 1.12.0 (build abc)", PrimitiveTypeName.BINARY))
.isFalse();

// version field present but not a valid semantic version
ParsedVersion invalidSemver = new ParsedVersion("parquet-mr", "not-a-semver", "abc");
assertThat(invalidSemver.hasSemanticVersion()).isFalse();
assertThat(invalidSemver.getSemanticVersionParseFailure())
.isInstanceOf(SemanticVersion.SemanticVersionParseException.class);
assertThat(CorruptStatistics.shouldIgnoreStatistics(
invalidSemver, "parquet-mr version not-a-semver (build abc)", PrimitiveTypeName.BINARY))
.isTrue();

// empty version field
ParsedVersion emptyVersion = new ParsedVersion("parquet-mr", "", "abc");
assertThat(CorruptStatistics.shouldIgnoreStatistics(
emptyVersion, "parquet-mr version (build abc)", PrimitiveTypeName.BINARY))
.isTrue();
}

@Test
public void testDistributionCorruptStatistics() {
assertThat(CorruptStatistics.shouldIgnoreStatistics(
Expand Down
28 changes: 20 additions & 8 deletions parquet-common/src/main/java/org/apache/parquet/VersionParser.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -41,24 +41,28 @@ public static class ParsedVersion {

private final boolean hasSemver;
private final SemanticVersion semver;
private final Exception semanticVersionParseFailure;

public ParsedVersion(String application, String version, String appBuildHash) {
checkArgument(!Strings.isNullOrEmpty(application), "application cannot be null or empty");
this.application = application;
this.version = Strings.isNullOrEmpty(version) ? null : version;
this.appBuildHash = Strings.isNullOrEmpty(appBuildHash) ? null : appBuildHash;

SemanticVersion sv;
boolean hasSemver;
try {
sv = SemanticVersion.parse(version);
hasSemver = true;
} catch (RuntimeException | SemanticVersionParseException e) {
sv = null;
hasSemver = false;
SemanticVersion sv = null;
boolean hasSemver = false;
Exception parseFailure = null;
if (this.version != null) {
try {
sv = SemanticVersion.parse(this.version);
hasSemver = true;
} catch (RuntimeException | SemanticVersionParseException e) {
parseFailure = e;
}
}
this.semver = sv;
this.hasSemver = hasSemver;
this.semanticVersionParseFailure = parseFailure;
}

public boolean hasSemanticVersion() {
Expand All @@ -69,6 +73,14 @@ public SemanticVersion getSemanticVersion() {
return semver;
}

/**
* Returns the exception captured when parsing the semantic version failed, or {@code null} if
* parsing succeeded.
*/
Exception getSemanticVersionParseFailure() {
return semanticVersionParseFailure;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
Expand Down
73 changes: 58 additions & 15 deletions ...et-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@
import org.apache.parquet.CorruptStatistics;
import org.apache.parquet.ParquetReadOptions;
import org.apache.parquet.Preconditions;
import org.apache.parquet.VersionParser.ParsedVersion;
import org.apache.parquet.VersionParser.VersionParseException;
import org.apache.parquet.column.ColumnDescriptor;
import org.apache.parquet.column.EncodingStats;
import org.apache.parquet.column.ParquetProperties;
Expand Down Expand Up @@ -945,7 +947,16 @@ public static org.apache.parquet.column.statistics.Statistics fromParquetStatist
// Visible for testing
static org.apache.parquet.column.statistics.Statistics fromParquetStatisticsInternal(
String createdBy, Statistics formatStats, PrimitiveType type, SortOrder typeSortOrder) {
// create stats object based on the column type
return fromParquetStatisticsInternal(null, createdBy, formatStats, type, typeSortOrder);
}

// Visible for testing
static org.apache.parquet.column.statistics.Statistics fromParquetStatisticsInternal(
ParsedVersion writerVersion,
String createdBy,
Statistics formatStats,
PrimitiveType type,
SortOrder typeSortOrder) {
org.apache.parquet.column.statistics.Statistics.Builder statsBuilder =
org.apache.parquet.column.statistics.Statistics.getBuilderForReading(type);

Expand All @@ -967,8 +978,11 @@ static org.apache.parquet.column.statistics.Statistics fromParquetStatisticsInte
// valid with the type's sort order. In previous releases, all stats were
// aggregated using a signed byte-wise ordering, which isn't valid for all the
// types (e.g. strings, decimals etc.).
if (!CorruptStatistics.shouldIgnoreStatistics(createdBy, type.getPrimitiveTypeName())
&& (sortOrdersMatch || maxEqualsMin)) {
boolean shouldIgnoreStatistics = writerVersion == null
? CorruptStatistics.shouldIgnoreStatistics(createdBy, type.getPrimitiveTypeName())
: CorruptStatistics.shouldIgnoreStatistics(
writerVersion, createdBy, type.getPrimitiveTypeName());
if (!shouldIgnoreStatistics && (sortOrdersMatch || maxEqualsMin)) {
if (isSet) {
statsBuilder.withMin(formatStats.min.array());
statsBuilder.withMax(formatStats.max.array());
Expand All @@ -989,7 +1003,13 @@ static org.apache.parquet.column.statistics.Statistics fromParquetStatisticsInte
public org.apache.parquet.column.statistics.Statistics fromParquetStatistics(
String createdBy, Statistics statistics, PrimitiveType type) {
SortOrder expectedOrder = overrideSortOrderToSigned(type) ? SortOrder.SIGNED : sortOrder(type);
return fromParquetStatisticsInternal(createdBy, statistics, type, expectedOrder);
return fromParquetStatisticsInternal(null, createdBy, statistics, type, expectedOrder);
}

public org.apache.parquet.column.statistics.Statistics fromParquetStatistics(
ParsedVersion writerVersion, String createdBy, Statistics statistics, PrimitiveType type) {
SortOrder expectedOrder = overrideSortOrderToSigned(type) ? SortOrder.SIGNED : sortOrder(type);
return fromParquetStatisticsInternal(writerVersion, createdBy, statistics, type, expectedOrder);
}

GeospatialStatistics toParquetGeospatialStatistics(
Expand Down Expand Up @@ -1821,13 +1841,22 @@ public FileMetaDataAndRowGroupOffsetInfo visit(RangeMetadataFilter filter) throw

public ColumnChunkMetaData buildColumnChunkMetaData(
ColumnMetaData metaData, ColumnPath columnPath, PrimitiveType type, String createdBy) {
return buildColumnChunkMetaData(metaData, columnPath, type, null, createdBy);
}

public ColumnChunkMetaData buildColumnChunkMetaData(
ColumnMetaData metaData,
ColumnPath columnPath,
PrimitiveType type,
ParsedVersion writerVersion,
String createdBy) {
return ColumnChunkMetaData.get(
columnPath,
type,
fromFormatCodec(metaData.codec),
convertEncodingStats(metaData.getEncoding_stats()),
fromFormatEncodings(metaData.encodings),
fromParquetStatistics(createdBy, metaData.statistics, type),
fromParquetStatistics(writerVersion, createdBy, metaData.statistics, type),
metaData.data_page_offset,
metaData.dictionary_page_offset,
metaData.num_values,
Expand All @@ -1854,6 +1883,15 @@ public ParquetMetadata fromParquetMetadata(
Map<RowGroup, Long> rowGroupToRowIndexOffsetMap)
throws IOException {
MessageType messageType = fromParquetSchema(parquetMetadata.getSchema(), parquetMetadata.getColumn_orders());
org.apache.parquet.hadoop.metadata.FileMetaData fileMetaData =
buildFileMetaData(parquetMetadata, messageType, encryptedFooter, fileDecryptor);
String createdBy = fileMetaData.getCreatedBy();
ParsedVersion writerVersion = null;
try {
writerVersion = fileMetaData.getWriterVersion();
} catch (VersionParseException e) {
// Fall back to String-based path which logs the parse error with full context
}
List<BlockMetaData> blocks = new ArrayList<BlockMetaData>();
List<RowGroup> row_groups = parquetMetadata.getRow_groups();

Expand Down Expand Up @@ -1930,13 +1968,11 @@ public ParquetMetadata fromParquetMetadata(
}
}

String createdBy = parquetMetadata.getCreated_by();
if (!lazyMetadataDecryption) { // full column metadata (with stats) is available
column = buildColumnChunkMetaData(
metaData,
columnPath,
messageType.getType(columnPath.toArray()).asPrimitiveType(),
createdBy);
PrimitiveType primitiveType =
messageType.getType(columnPath.toArray()).asPrimitiveType();
column =
buildColumnChunkMetaData(metaData, columnPath, primitiveType, writerVersion, createdBy);
column.setRowGroupOrdinal(rowGroup.getOrdinal());
if (metaData.isSetBloom_filter_offset()) {
column.setBloomFilterOffset(metaData.getBloom_filter_offset());
Expand Down Expand Up @@ -1975,6 +2011,15 @@ public ParquetMetadata fromParquetMetadata(
blocks.add(blockMetaData);
}
}
return new ParquetMetadata(fileMetaData, blocks);
}

private static org.apache.parquet.hadoop.metadata.FileMetaData buildFileMetaData(
FileMetaData parquetMetadata,
MessageType messageType,
boolean encryptedFooter,
InternalFileDecryptor fileDecryptor) {
String createdBy = parquetMetadata.getCreated_by();
Map<String, String> keyValueMetaData = new HashMap<String, String>();
List<KeyValue> key_value_metadata = parquetMetadata.getKey_value_metadata();
if (key_value_metadata != null) {
Expand All @@ -1990,10 +2035,8 @@ public ParquetMetadata fromParquetMetadata(
} else {
encryptionType = EncryptionType.UNENCRYPTED;
}
return new ParquetMetadata(
new org.apache.parquet.hadoop.metadata.FileMetaData(
messageType, keyValueMetaData, parquetMetadata.getCreated_by(), encryptionType, fileDecryptor),
blocks);
return new org.apache.parquet.hadoop.metadata.FileMetaData(
messageType, keyValueMetaData, createdBy, encryptionType, fileDecryptor);
}

private static IndexReference toColumnIndexReference(ColumnChunk columnChunk) {
Expand Down
Loading
Back | FazBrowse Home | New Git URL