GH-3696: Cache ParsedVersion in FileMetaData to eliminate redundant parsing - #3700
GH-3696: Cache ParsedVersion in FileMetaData to eliminate redundant parsing#3700asifsmohammed wants to merge 1 commit into
Conversation
…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).
wgtmac
left a comment
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?
| this.keyValueMetaData = | ||
| unmodifiableMap(Objects.requireNonNull(keyValueMetaData, "keyValueMetaData cannot be null")); | ||
| this.createdBy = createdBy; | ||
| this.writerVersion = parseVersion(createdBy); |
There was a problem hiding this comment.
Seems reasonable. Do we want to initialize this field lazily?
wgtmac
left a comment
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.
| @@ -42,6 +44,7 @@ public enum EncryptionType { | |||
| private final MessageType schema; | |||
| private final Map<String, String> keyValueMetaData; | |||
There was a problem hiding this comment.
This cached field is transient final. After Java deserialization it will stay null even when createdBy is valid. Recompute lazily in getWriterVersion(), or add serialization handling and tests.
| * @return the parsed writer version, or {@code null} if {@code createdBy} is null, empty, or unparseable | ||
| */ | ||
| @JsonIgnore | ||
| public ParsedVersion getWriterVersion() { |
There was a problem hiding this comment.
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.
| return VersionParser.parse(createdBy); | ||
| } catch (RuntimeException | VersionParser.VersionParseException e) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Returning null loses the difference between missing createdBy and parse failure. CorruptStatistics currently logs different warnings for these cases. Preserve the parse failure state, or keep enough context for migrated callers to preserve existing behavior.
| 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.
2 participants
Rationale for this change
VersionParser.parse(createdBy)is called from 7 production sites, all parsing the same constant string fromFileMetaData.getCreatedBy(). SinceFileMetaDatais constructed once per file and already stores thecreatedBystring, it is the natural place to parse once and cache the result.This allows callers that already hold a
FileMetaDatareference to usegetWriterVersion()directly instead of re-parsing thecreatedBystring on every access.What changes are included in this PR?
transient ParsedVersion writerVersionfield toFileMetaDatacreatedByonce in the canonical constructor and cache the resultgetWriterVersion()getter (annotated@JsonIgnoreto preserve JSON serialization)FileMetaDataTestwith coverage for valid, null, empty, and unparseable version stringsAre these changes tested?
Yes. New
FileMetaDataTestwith 5 test cases.Are there any user-facing changes?
No breaking changes. Adds a new public method
FileMetaData.getWriterVersion()that downstream call sites can use to avoid redundant parsing.Closes #3696