GH-3696: Cache ParsedVersion in FileMetaData and use it in fromParquetMetadata - #3700
GH-3696: Cache ParsedVersion in FileMetaData and use it in fromParquetMetadata#3700asifsmohammed wants to merge 7 commits into
Conversation
Sorry, something went wrong.
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for fixing this! If this gets checked in, the PR that fixes malformed stats checking can be closed, right?
Sorry, something went wrong.
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for pushing this cleaner direction. I think caching the parsed writer version in FileMetaData is the right foundation, but this PR is not a complete fix yet. It currently adds the cache, but does not migrate the production call sites that still parse created_by repeatedly. Please update the hot/footer/page/reader/rewrite paths to consume the cached ParsedVersion, and cover that behavior in tests.
Sorry, something went wrong.
| FileMetaData meta = | ||
| new FileMetaData(SCHEMA, Collections.emptyMap(), "parquet-mr version 1.12.0 (build abc123)"); | ||
|
|
||
| assertThat(meta.getWriterVersion()).isNotNull(); |
There was a problem hiding this comment.
These tests only cover the getter. They do not prove the repeated parsing problem is fixed. Please add coverage for at least one migrated production path using the cached ParsedVersion instead of reparsing createdBy.
Sorry, something went wrong.
…dant parsing Parse the createdBy version string once during FileMetaData construction and cache the result as a transient field. This avoids redundant VersionParser.parse() calls at every downstream call site (R×C times during footer decode alone).
- Change writerVersion to lazy computation on first getWriterVersion() call - Fixes deserialization correctness (transient fields recompute from createdBy) - Add writerVersionParsed flag to avoid retrying on parse failure - Document contract for distinguishing missing vs. unparseable in javadoc
@wgtmac I'd prefer to keep this PR focused on the caching foundation and migrate callers in a follow-up. The migration touches multiple files which is a larger change that's easier to review separately. The follow-up will include tests proving the production path uses the cached version. |
Sorry, something went wrong.
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for the quick response!
I agree with keeping this PR small and moving the full migration to follow-ups. However, I do think it is worth migrating at least one production caller so this PR provides a concrete fix, not only an unused cache.
Please also narrow the title and description and remove “Closes #3696”, since the remaining paths still parse created_by repeatedly.
Sorry, something went wrong.
…zed init - Replace writerVersion + writerVersionParsed with single WriterVersionResult - Null field means not-yet-initialized, MISSING for null/empty createdBy - Double-checked locking with synchronized for thread-safe one-time init - Rethrow cached VersionParseException so callers preserve existing fallback - Use Strings.isNullOrEmpty for consistency with CorruptStatistics
Add shouldIgnoreStatistics(ParsedVersion, PrimitiveTypeName) overload to CorruptStatistics that uses the pre-parsed and cached SemanticVersion from ParsedVersion, eliminating redundant VersionParser.parse and SemanticVersion.parse calls in the R×C hot path. Refactor ParquetMetadataConverter.fromParquetMetadata to construct the hadoop FileMetaData before the row-group loop and extract the cached ParsedVersion once via getWriterVersion(). The loop now uses the ParsedVersion-based buildColumnChunkMetaData overload, avoiding per-column re-parsing. Falls back to the String-based path when getWriterVersion() throws VersionParseException to preserve exact logging parity.
| if (!writerVersion.hasSemanticVersion()) { | ||
| warnOnce("Ignoring statistics because created_by could not be parsed (see PARQUET-251): " + writerVersion); | ||
| return true; | ||
| } | ||
|
|
||
| SemanticVersion semver = writerVersion.getSemanticVersion(); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
Hi @wgtmac, I've addressed your feedback. |
Sorry, something went wrong.
|
Hi @wgtmac is it possible to speed up process to get this and subsequent PRs merged? It will bring significantly cost savings to our company |
Sorry, something went wrong.
wgtmac
left a comment
There was a problem hiding this comment.
Sorry for the delay! Thanks @asifsmohammed for improving this! I still have some comments. Will merge it after all those have been addressed.
Sorry, something went wrong.
| ParsedVersion version = VersionParser.parse(createdBy); | ||
| return shouldIgnoreStatistics(version, columnType); | ||
| } catch (RuntimeException | VersionParseException e) { | ||
| warnParseErrorOnce(createdBy, e); |
There was a problem hiding this comment.
| 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.
Sorry, something went wrong.
| boolean isSet = formatStats.isSetMax() && formatStats.isSetMin(); | ||
| boolean maxEqualsMin = isSet ? Arrays.equals(formatStats.getMin(), formatStats.getMax()) : false; | ||
| boolean sortOrdersMatch = SortOrder.SIGNED == typeSortOrder; | ||
| // NOTE: See docs in CorruptStatistics for explanation of why this check is needed |
There was a problem hiding this comment.
Could we preserve these comments?
Sorry, something went wrong.
| * @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, PrimitiveTypeName columnType) { |
There was a problem hiding this comment.
This overload makes calls like shouldIgnoreStatistics(null, type) ambiguous; the cast in the new test demonstrates the source incompatibility. Could we use a distinct method name for the ParsedVersion path, including the new converter overloads?
Sorry, something went wrong.
There was a problem hiding this comment.
Added String createdBy as a parameter to the ParsedVersion overload, so this signature (ParsedVersion, String, PrimitiveTypeName) is no longer ambiguous with (String, PrimitiveTypeName). The createdBy parameter also solves the logging parity issue mentioned below comment. Converter overloads follow the same pattern.
Sorry, something went wrong.
| return true; | ||
| } | ||
|
|
||
| if (!writerVersion.hasSemanticVersion()) { |
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| String createdBy, Statistics formatStats, PrimitiveType type, SortOrder typeSortOrder) { | ||
| // create stats object based on the column type | ||
| return fromParquetStatisticsInternal( | ||
| CorruptStatistics.shouldIgnoreStatistics(createdBy, type.getPrimitiveTypeName()), |
There was a problem hiding this comment.
This evaluates shouldIgnoreStatistics even when stats are null or V2 min/max is used, so it may log “Ignoring statistics” and consume the one-shot warning when nothing is ignored. Could we keep this check inside the legacy min/max branch and add a regression test?
Sorry, something went wrong.
There was a problem hiding this comment.
Moved shouldIgnoreStatistics evaluation inside the V1 legacy min/max branch, V2 stats now bypass it entirely. Added testV2StatsDoNotTriggerCorruptStatisticsCheck regression test that verifies a corrupt writer version with V2 stats still gets valid min/max without consuming the one-shot warning.
Sorry, something went wrong.
- Add createdBy parameter to shouldIgnoreStatistics(ParsedVersion, ...) to resolve null ambiguity (distinct 3-param signature) and restore exact log parity by using the raw string in warnings. - Re-parse SemanticVersion in the !hasSemanticVersion() branch to recreate the exception for warnParseErrorOnce with stack trace. - Move shouldIgnoreStatistics evaluation inside the V1 legacy min/max branch so V2 stats never trigger the one-shot warning. - Restore original comments in both CorruptStatistics and ParquetMetadataConverter. - Add regression test for V2 stats with corrupt writer version.
|
Sorry for the delay. I still think the current approach is not clean. To avoid more roundtrip discussion, I've put up a draft as the diff below. The basic idea is to make ParsedVersion retain the parse failure, so callers can reuse the original exception instead of parsing the same version again just to reconstruct the warning. diff --git a/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java b/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
index 546b9bdf..85493efc 100644
--- a/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
+++ b/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
@@ -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;
@@ -110,11 +109,7 @@ public class CorruptStatistics {
}
if (!writerVersion.hasSemanticVersion()) {
- try {
- SemanticVersion.parse(writerVersion.version);
- } catch (SemanticVersionParseException e) {
- warnParseErrorOnce(createdBy, e);
- }
+ warnParseErrorOnce(createdBy, writerVersion.getSemanticVersionParseFailure());
return true;
}
diff --git a/parquet-column/src/test/java/org/apache/parquet/CorruptStatisticsTest.java b/parquet-column/src/test/java/org/apache/parquet/CorruptStatisticsTest.java
index ff950a25..ba4668ad 100644
--- a/parquet-column/src/test/java/org/apache/parquet/CorruptStatisticsTest.java
+++ b/parquet-column/src/test/java/org/apache/parquet/CorruptStatisticsTest.java
@@ -162,6 +162,8 @@ public class CorruptStatisticsTest {
// 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();
diff --git a/parquet-common/src/main/java/org/apache/parquet/VersionParser.java b/parquet-common/src/main/java/org/apache/parquet/VersionParser.java
index 07feb0d3..82e45eb8 100644
--- a/parquet-common/src/main/java/org/apache/parquet/VersionParser.java
+++ b/parquet-common/src/main/java/org/apache/parquet/VersionParser.java
@@ -41,6 +41,7 @@ public class VersionParser {
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");
@@ -48,17 +49,20 @@ public class VersionParser {
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() {
@@ -69,6 +73,14 @@ public class VersionParser {
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;
diff --git a/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java b/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
index 1f2fd6e4..8f2dd852 100644
--- a/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
+++ b/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
@@ -947,44 +947,7 @@ public class ParquetMetadataConverter {
// Visible for testing
static org.apache.parquet.column.statistics.Statistics fromParquetStatisticsInternal(
String createdBy, Statistics formatStats, PrimitiveType type, SortOrder typeSortOrder) {
- org.apache.parquet.column.statistics.Statistics.Builder statsBuilder =
- org.apache.parquet.column.statistics.Statistics.getBuilderForReading(type);
-
- if (formatStats != null) {
- // Use the new V2 min-max statistics over the former one if it is filled
- 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;
- // NOTE: See docs in CorruptStatistics for explanation of why this check is needed
- // The sort order is checked to avoid returning min/max stats that are not
- // 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)) {
- if (isSet) {
- statsBuilder.withMin(formatStats.min.array());
- statsBuilder.withMax(formatStats.max.array());
- }
- }
- }
-
- if (formatStats.isSetNull_count()) {
- statsBuilder.withNumNulls(formatStats.null_count);
- }
- if (formatStats.isSetNan_count()) {
- statsBuilder.withNanCount(formatStats.getNan_count());
- }
- }
- return statsBuilder.build();
+ return fromParquetStatisticsInternal(null, createdBy, formatStats, type, typeSortOrder);
}
// Visible for testing
@@ -1015,7 +978,10 @@ public class ParquetMetadataConverter {
// 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(writerVersion, createdBy, type.getPrimitiveTypeName())
+ 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());
@@ -1875,20 +1841,7 @@ public class ParquetMetadataConverter {
public ColumnChunkMetaData buildColumnChunkMetaData(
ColumnMetaData metaData, ColumnPath columnPath, PrimitiveType type, String createdBy) {
- return ColumnChunkMetaData.get(
- columnPath,
- type,
- fromFormatCodec(metaData.codec),
- convertEncodingStats(metaData.getEncoding_stats()),
- fromFormatEncodings(metaData.encodings),
- fromParquetStatistics(createdBy, metaData.statistics, type),
- metaData.data_page_offset,
- metaData.dictionary_page_offset,
- metaData.num_values,
- metaData.total_compressed_size,
- metaData.total_uncompressed_size,
- fromParquetSizeStatistics(metaData.size_statistics, type),
- fromParquetStatistics(metaData.geospatial_statistics, type));
+ return buildColumnChunkMetaData(metaData, columnPath, type, null, createdBy);
}
public ColumnChunkMetaData buildColumnChunkMetaData(
@@ -1934,10 +1887,8 @@ public class ParquetMetadataConverter {
buildFileMetaData(parquetMetadata, messageType, encryptedFooter, fileDecryptor);
String createdBy = fileMetaData.getCreatedBy();
ParsedVersion writerVersion = null;
- boolean useWriterVersion = false;
try {
writerVersion = fileMetaData.getWriterVersion();
- useWriterVersion = true;
} catch (VersionParseException e) {
// Fall back to String-based path which logs the parse error with full context
}
@@ -2020,10 +1971,8 @@ public class ParquetMetadataConverter {
if (!lazyMetadataDecryption) { // full column metadata (with stats) is available
PrimitiveType primitiveType =
messageType.getType(columnPath.toArray()).asPrimitiveType();
- column = useWriterVersion
- ? buildColumnChunkMetaData(
- metaData, columnPath, primitiveType, writerVersion, createdBy)
- : buildColumnChunkMetaData(metaData, columnPath, primitiveType, createdBy);
+ column = buildColumnChunkMetaData(
+ metaData, columnPath, primitiveType, writerVersion, createdBy);
column.setRowGroupOrdinal(rowGroup.getOrdinal());
if (metaData.isSetBloom_filter_offset()) {
column.setBloomFilterOffset(metaData.getBloom_filter_offset());
|
Sorry, something went wrong.
- Add getSemanticVersionParseFailure() to ParsedVersion to retain the original exception instead of re-parsing to reconstruct the warning. - Collapse fromParquetStatisticsInternal and buildColumnChunkMetaData into single implementations where the String overload delegates with null writerVersion. - Remove useWriterVersion flag; the shared implementation selects the corrupt-statistics check lazily based on whether writerVersion is null.
|
@wgtmac thanks for the feedback. Please take a look again and let me know if you have further concerns or questions.
Thats a great suggestion, added getSemanticVersionParseFailure
Removed duplicate implementation in fromParquetStatisticsInternal, buildColumnChunkMetaData and also removed useWriterVersion flag. Now we just call shouldIgnoreStatistics based on parsed version value. |
Sorry, something went wrong.
Route fromParquetStatistics(String, Statistics, PrimitiveType) directly to the shared 5-param fromParquetStatisticsInternal with null writerVersion, avoiding an unnecessary intermediate method call.
Rationale for this change
VersionParser.parse(createdBy) is called from 7 production sites, all parsing the same constant string from FileMetaData.getCreatedBy(). In fromParquetMetadata, this happens R×C times (once per column per row group) during footer metadata conversion. Since FileMetaData is
constructed once per file and already stores the createdBy string, it is the natural place to parse once and cache the result.
This PR caches the parsed version and migrates the first (and hottest) call site — ParquetMetadataConverter.fromParquetMetadata — to use the cache, eliminating redundant VersionParser.parse and SemanticVersion.parse calls from the R×C inner loop.
Fixes #1 in this issue #3696
What changes are included in this PR?
Are these changes tested?
Yes.
Are there any user-facing changes?
No breaking changes. Adds new public methods:
Closes #3601