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

asifsmohammed · 2026-08-01T02:08:33Z

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?

Add getWriterVersion() to FileMetaData with lazy-init via an immutable WriterVersionResult holder (thread-safe, double-checked locking)
Add shouldIgnoreStatistics(ParsedVersion, PrimitiveTypeName) overload to CorruptStatistics that uses the cached SemanticVersion from ParsedVersion directly
Refactor ParquetMetadataConverter.fromParquetMetadata to construct FileMetaData before the row-group loop and use the cached ParsedVersion in buildColumnChunkMetaData
Falls back to the String-based path when getWriterVersion() throws VersionParseException to preserve exact logging behavior

Are these changes tested?

Yes.

FileMetaDataTest — 6 tests covering valid, null, empty, unparseable version strings, and caching
CorruptStatisticsTest.testParsedVersionOverload — covers all branches of the new ParsedVersion overload including null, non-parquet-mr, empty version, invalid semver, corrupt, and fixed versions
TestParquetMetadataConverter — 70 existing tests pass (validates the refactored fromParquetMetadata path)

Are there any user-facing changes?

No breaking changes. Adds new public methods:

FileMetaData.getWriterVersion() — returns cached ParsedVersion, throws VersionParseException for unparseable strings
CorruptStatistics.shouldIgnoreStatistics(ParsedVersion, PrimitiveTypeName) — for callers that already have a parsed version

Closes #3601

wgtmac

Thanks for fixing this! If this gets checked in, the PR that fixes malformed stats checking can be closed, right?

wgtmac

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.

wgtmac · 2026-08-01T07:57:21Z

+    FileMetaData meta =
+        new FileMetaData(SCHEMA, Collections.emptyMap(), "parquet-mr version 1.12.0 (build abc123)");
+
+    assertThat(meta.getWriterVersion()).isNotNull();


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.

…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

asifsmohammed · 2026-08-02T17:42:17Z

This adds the cached value, but no production call site uses it yet. Please migrate the callers listed in the issue, especially footer stats, page stats, reader init, rewrite, and lazy encrypted metadata paths.

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.

@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.
If needed I can add fixes for 1 caller using the ParsedVersion in this PR or I create a separate PR to fix all callers using ParsedVersion in parallel to this PR. Wdyt? I have no concerns on fixing all of them in this PR itself.

wgtmac

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.

…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.

asifsmohammed · 2026-08-05T19:07:12Z

+    if (!writerVersion.hasSemanticVersion()) {
+      warnOnce("Ignoring statistics because created_by could not be parsed (see PARQUET-251): " + writerVersion);
+      return true;
+    }
+
+    SemanticVersion semver = writerVersion.getSemanticVersion();


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.

asifsmohammed · 2026-08-13T19:38:06Z

Hi @wgtmac, I've addressed your feedback.
Would you have a chance to take another look when you get a moment? Happy to address any further feedback. Thanks!

charlessumo · 2026-08-18T18:11:58Z

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

wgtmac

Sorry for the delay! Thanks @asifsmohammed for improving this! I still have some comments. Will merge it after all those have been addressed.

wgtmac · 2026-08-21T09:32:18Z

      ParsedVersion version = VersionParser.parse(createdBy);
+      return shouldIgnoreStatistics(version, columnType);
+    } catch (RuntimeException | VersionParseException e) {
+      warnParseErrorOnce(createdBy, e);


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.

wgtmac · 2026-08-21T09:34:16Z

        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


Could we preserve these comments?

wgtmac · 2026-08-21T09:54:23Z

+   * @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) {


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?

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.

wgtmac · 2026-08-21T09:54:25Z

+      return true;
+    }
+
+    if (!writerVersion.hasSemanticVersion()) {


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?

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.

wgtmac · 2026-08-21T09:54:26Z

      String createdBy, Statistics formatStats, PrimitiveType type, SortOrder typeSortOrder) {
-    // create stats object based on the column type
+    return fromParquetStatisticsInternal(
+        CorruptStatistics.shouldIgnoreStatistics(createdBy, type.getPrimitiveTypeName()),


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?

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.

- 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.

wgtmac approved these changes Aug 1, 2026

View reviewed changes

Comment thread parquet-hadoop/src/main/java/org/apache/parquet/hadoop/metadata/FileMetaData.java Outdated

wgtmac requested changes Aug 1, 2026

View reviewed changes

asifsmohammed force-pushed the gh-3696-cache-parsed-version branch from d7c4f6d to 2f01073 Compare August 2, 2026 16:59

asifsmohammed force-pushed the gh-3696-cache-parsed-version branch from 2f01073 to bc23cf5 Compare August 2, 2026 17:08

wgtmac reviewed Aug 3, 2026

View reviewed changes

Comment thread parquet-hadoop/src/main/java/org/apache/parquet/hadoop/metadata/FileMetaData.java Outdated

asifsmohammed added 2 commits August 5, 2026 10:31

asifsmohammed changed the title ~~GH-3696: Cache ParsedVersion in FileMetaData to eliminate redundant parsing~~ GH-3696: Cache ParsedVersion in FileMetaData and use it in fromParquetMetadata Aug 5, 2026

asifsmohammed commented Aug 5, 2026

View reviewed changes

asifsmohammed requested a review from wgtmac August 12, 2026 17:52

wgtmac requested changes Aug 21, 2026

View reviewed changes

asifsmohammed requested a review from wgtmac August 21, 2026 19:43

Uh oh!

Conversation

asifsmohammed commented Aug 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asifsmohammed commented Aug 2, 2026

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asifsmohammed commented Aug 13, 2026

Uh oh!

charlessumo commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asifsmohammed commented Aug 1, 2026 •

edited

Loading

charlessumo commented Aug 18, 2026 •

edited

Loading