asif-moh commented on PR #3607:
URL: https://github.com/apache/parquet-java/pull/3607#issuecomment-5006760055
@wgtmac Thanks for the review. I agree with the overall direction toward
`ParsedVersion`. Let me share findings from my investigation here,
1. mayHaveCorruptStatistics as public API
Agreed, exposing a PARQUET-251-specific predicate as public API is the
wrong abstraction. It ties us to maintaining that exact semantic contract. The
ParsedVersion-based approach keeps this internal.
2. mayHaveCorruptStatistics is not a pure predicate
I looked at this more closely and I believe the logging behavior is
actually unchanged between master and this PR. The schema gate
(`schemaHasCorruptStatisticsColumnType`) ensures `mayHaveCorruptStatistics` is
only called when the schema has `BINARY`/`FIXED_LEN_BYTE_ARRAY` columns, the
same precondition that would trigger the warning on master. In all 4 cases
(corrupt/non-corrupt × has-binary/no-binary), the warning fires or doesn't fire
in identical scenarios. The only difference is microsecond-level timing within
the same `fromParquetMetadata` call.
That said, I agree the `ParsedVersion` approach is cleaner here regardless
parsing is side-effect-free, and the warning stays inside
`shouldIgnoreStatistics` where it's always been.
3. The fix is partial
I investigated this further and the problem is broader than I initially
realized. `VersionParser.parse(createdBy)` is called from 6 distinct production
sites, all parsing the same constant string from `FileMetaData.getCreatedBy()`:
| # | Call site | Frequency per file | When |
|---|-----------|-------------------|------|
| 1 | `CorruptStatistics.shouldIgnoreStatistics` via
`buildColumnChunkMetaData` | R × C | Footer decode |
| 2 | `CorruptStatistics.shouldIgnoreStatistics` via
`ParquetFileReader.readAllPages` | Pages per column chunk | Page read |
| 3 | `CorruptStatistics.shouldIgnoreStatistics` via
`ParquetRewriter.convertStatistics` | Pages per rewritten chunk | Rewrite |
| 4 | `CorruptStatistics.shouldIgnoreStatistics` via
`EncryptedColumnChunkMetaData.decryptIfNeeded` | 1 per encrypted column | Lazy
decrypt |
| 5 | `CorruptDeltaByteArrays.requiresSequentialReads(String, Encoding)`
in `ParquetRecordReader` | 1 per reader init (parse only if DELTA_BYTE_ARRAY
present) | Reader init |
| 6 | `ColumnReadStoreImpl` constructor via
`MessageColumnIO.getRecordReader` | R (once per row group read) | Row group
materialization |
| 7 | `ColumnReadStoreImpl` constructor via
`ParquetRewriter.nullifyColumn` | 1 per nullified column | Rewrite with
nullification |
Notably, `ColumnReadStoreImpl` already parses `createdBy` into a
`ParsedVersion` and stores it as a field but it does so R times (once per row
group) because nobody upstream caches the parsed result. The
`CorruptDeltaByteArrays.requiresSequentialReads(ParsedVersion, Encoding)`
overload already exists and is used from `ColumnReaderBase` this is exactly the
pattern you suggested for CorruptStatistics.
4. PARQUET-251 logic leaking into the converter
Agreed. Threading a boolean specific to one bug doesn't scale. The
ParsedVersion approach keeps the converter generic and it just says "here's the
writer version" and lets `CorruptStatistics` decide.
Proposed approach:
I'd like to propose addressing the root cause separately: cache a
`ParsedVersion` in `FileMetaData`. Since `FileMetaData` is constructed once per
file and already stores the `createdBy` string, it's the natural place to parse
once and cache:
```
// In FileMetaData:
private final transient ParsedVersion writerVersion;
public FileMetaData(
.... {
this.createdBy = createdBy;
this.writerVersion = parseVersion(createdBy); // ← just add this
...
}
public ParsedVersion getWriterVersion() {
return writerVersion;
}
private static ParsedVersion parseVersion(String createdBy) {
if (Strings.isNullOrEmpty(createdBy)) return null;
try {
return VersionParser.parse(createdBy);
} catch (RuntimeException | VersionParseException e) {
return null;
}
```
This is purely additive (new field + getter), doesn't break serialization
(transient), and enables all 7 call sites to stop re-parsing.
`ColumnReadStoreImpl` could accept `ParsedVersion` directly instead of
re-parsing in its constructor, and `CorruptStatistics` would gain a
`shouldIgnoreStatistics(ParsedVersion, PrimitiveTypeName)` overload following
the established `CorruptDeltaByteArrays.requiresSequentialReads(ParsedVersion,
Encoding)` pattern.
Would you be open to a separate issue/PR for adding `ParsedVersion` caching
to `FileMetaData`? That would be a small, zero-behavior-change change just
parsing `createdBy` in the existing constructor body and exposing a getter.
The ParsedVersion-based overloads for `CorruptStatistics`,
`ColumnReadStoreImpl`, etc. can be added incrementally after that, and this PR
could then rebase onto the foundation cleanly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
