@asifsmohammed Thanks for reviewing! I am an agreement with your suggestions.

Removed the global static cache entirely. Now computes shouldIgnoreStatistics once per file in fromParquetMetadata before the row-group loop and passes the pre-computed flag through buildColumnChunkMetaData to fromParquetStatisticsInternal.

cc @wgtmac

@asif-moh @wgtmac Addressed the feedback: factored CorruptStatistics into fileHasCorruptStatistics + isCorruptStatisticsColumnType (with shouldIgnoreStatistics delegating), precompute the file-level flag once in fromParquetMetadata (schema-gated so binary-free files do no created_by parsing), thread a boolean through buildColumnChunkMetaData/fromParquetStatisticsInternal, drop the duplicated body and dead code, and fix the per-column gate so non-binary columns are not ignored. Updated the title/description to match. PTAL.

@wgtmac addressed the API-minimization and rename: fileHasCorruptStatistics is renamed to mayHaveCorruptStatistics and kept public (ParquetMetadataConverter relies on it, otherwise the version-parsing logic would be duplicated). isCorruptStatisticsColumnType is now private with the column-type check inlined at its call sites, so the only new public method this PR adds is mayHaveCorruptStatistics(String). Behavior is unchanged; tests pass. PTAL.

Could you resolve the conflict?

@wgtmac rebased onto latest master and resolved the conflict (it was only in TestParquetMetadataConverter - kept master's new NaN/IEEE754 tests plus the two GH-3601 tests). The GH-3601 change is intact; parquet-column + parquet-hadoop tests pass and spotless is clean.

@wgtmac, @yadavay-amzn has addressed your feedback and rebased onto master. Is there anything else
pending on this PR, or is it ready to merge? We'd love to get this included in the next release if possible. TY!

@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():

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.