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 2 commits into
Conversation
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.
d7c4f6d to
2f01073
Compare
…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).
2f01073 to
bc23cf5
Compare
- 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. |
wgtmac
left a comment
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.
| private final Map<String, String> keyValueMetaData; | ||
| private final String createdBy; | ||
| private transient volatile ParsedVersion writerVersion; | ||
| private transient volatile boolean writerVersionParsed; |
There was a problem hiding this comment.
Please replace writerVersion and writerVersionParsed with one private immutable WriterVersionResult field. Use a null field only for “not initialized”. The result should represent valid, missing, or invalid and retain the original parse exception. Initialize it once in a synchronized block. Keep only getWriterVersion() public: return the parsed version, return null for missing createdBy, and rethrow the cached exception for invalid createdBy so callers can preserve the existing fallback without reparsing.
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