GH-3601: Avoid repeated created_by parsing in footer metadata conversion

[yadavay-amzn]

Summary

Footer conversion previously evaluated CorruptStatistics.shouldIgnoreStatistics(createdBy, columnType) per (row-group x column), re-parsing the created_by version string on every call.

This PR factors CorruptStatistics into two orthogonal checks:

• fileHasCorruptStatistics(createdBy) - file-level, parsed once
• isCorruptStatisticsColumnType(columnType) - per-column type gate

The file-level corrupt-stats flag is precomputed once in fromParquetMetadata, gated on the schema containing at least one affected column type (BINARY or FIXED_LEN_BYTE_ARRAY), so binary-free files skip the created_by parsing entirely and avoid spurious PARQUET-251 warnings. The boolean is threaded through buildColumnChunkMetaData/fromParquetStatisticsInternal instead of re-parsing created_by per column.

Note: an earlier process-wide cache approach was dropped per review feedback.

Tests

• CorruptStatisticsTest: unit tests for the factored methods (fileHasCorruptStatistics, isCorruptStatisticsColumnType)
• TestParquetMetadataConverter#testCorruptStatsPerColumnGate: verifies per-column gate behavior with corrupt/non-corrupt files
• TestParquetMetadataConverter#testSchemaGateSkipsCorruptStatsCheckForNonBinarySchema: verifies that an INT-only schema with a known-corrupt created_by preserves statistics (schema gate prevents unnecessary parsing)

Closes #3601

[asifsmohammed]

I generally like the approach @yadavay-amzn, here's why I would not use cache alone as I mentioned in the issue. It's better than the current approach of checking it RGxCC times

• In high workload scenarios we are going to call shouldIgnoreStatistics RGxCC times i.e. we are going to call .size and .computeIfAbsent(hash + equals) that many times which is still not ideal.

The cleaner fix would be at the caller side in ParquetMetadataConverter.fromParquetMetadata(): compute shouldIgnoreStatistics once before the row group loop using the file-level createdBy, then pass the pre-computed boolean through buildColumnChunkMetaData → fromParquetStatisticsInternal. This eliminates all per-column overhead — no hash, no lookup, just a boolean flowing through the call chain.

This eliminates the need to check shouldIgnoreStatistics for every RGxCC. Both approaches could coexist (cache helps other callers), but the caller-side fix is where we see real improvements

[yadavay-amzn]

@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

[Copilot]

Pull request overview

Optimizes footer metadata conversion by avoiding repeated CorruptStatistics.shouldIgnoreStatistics(created_by, ...) evaluation during row-group/column iteration, by precomputing a boolean once and threading it through column-chunk metadata/statistics conversion.

Changes:

• Added a new internal statistics conversion overload that accepts a precomputed shouldIgnoreCorruptStats boolean.
• Added an overloaded buildColumnChunkMetaData method that accepts the precomputed flag and routes footer-reading through it.
• Precomputes shouldIgnoreCorruptStats once in fromParquetMetadata(...) and passes it into the column loop.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[asif-moh]

This is calling shouldIgnoreStatistics with PrimitiveTypeName.BINARY hardcoded which is incorrect.

Instead we can refactor shouldIgnoreStatistics by adding public methods.

public static boolean shouldIgnoreStatistics(String createdBy, PrimitiveTypeName columnType) {
    if (!isCorruptStatisticsColumnType(columnType)) {
      // the bug only applies to binary columns
      return false;
    }
    return fileHasCorruptStatistics(createdBy);
}

public static boolean isCorruptStatisticsColumnType(PrimitiveTypeName columnType) {
    return columnType == PrimitiveTypeName.BINARY || columnType == PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY;
}

public static boolean fileHasCorruptStatistics(String createdBy) {
    // rest of the logic from shouldIgnoreStatistics
}

[yadavay-amzn]

Done. Added isCorruptStatisticsColumnType(columnType) and fileHasCorruptStatistics(createdBy), with shouldIgnoreStatistics delegating to both. The file-level flag is computed via fileHasCorruptStatistics (no hardcoded BINARY), and the per-column BINARY/FIXED_LEN_BYTE_ARRAY check is applied where stats are converted.

[asif-moh]

Instead of duplicating the entire fromParquetStatisticsInternal body, the existing method can simply delegate to a new overload and this eliminates duplicate code

static org.apache.parquet.column.statistics.Statistics fromParquetStatisticsInternal(
      String createdBy, Statistics formatStats, PrimitiveType type, SortOrder typeSortOrder) {
    return fromParquetStatisticsInternal(
        formatStats, 
        type, 
        typeSortOrder, 
        CorruptStatistics.fileHasCorruptStatistics(createdBy) // This is a new method in CorruptStatistics
    );
}


// overloaded method
static org.apache.parquet.column.statistics.Statistics fromParquetStatisticsInternal(
      Statistics formatStats, PrimitiveType type, SortOrder typeSortOrder, boolean fileHasCorruptStats) {

[yadavay-amzn]

Done. Removed the duplicated body; the createdBy overload now delegates to the boolean overload via CorruptStatistics.fileHasCorruptStatistics(createdBy).

[asif-moh]

This is calling shouldIgnoreStatistics with PrimitiveTypeName.BINARY hardcoded which is incorrect.

Instead we can refactor shouldIgnoreStatistics by adding public methods.

public static boolean shouldIgnoreStatistics(String createdBy, PrimitiveTypeName columnType) {
    if (!isCorruptStatisticsColumnType(columnType)) {
      // the bug only applies to binary columns
      return false;
    }
    return fileHasCorruptStatistics(createdBy);
}

public static boolean isCorruptStatisticsColumnType(PrimitiveTypeName columnType) {
    return columnType == PrimitiveTypeName.BINARY || columnType == PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY;
}

public static boolean fileHasCorruptStatistics(String createdBy) {
    // rest of the logic from shouldIgnoreStatistics
}

[asif-moh]

We no longer need to pass createdBy downstream

[yadavay-amzn]

Done. The precomputed boolean is threaded instead; createdBy is no longer passed downstream.

[asif-moh]

buildColumnChunkMetaData can delegate to a package-private overload that takes the boolean similar to what you have done but with few changes,

public ColumnChunkMetaData buildColumnChunkMetaData(
      ColumnMetaData metaData, ColumnPath columnPath, PrimitiveType type, String createdBy) {
    return buildColumnChunkMetaData(
        metaData, columnPath, type, CorruptStatistics.fileHasCorruptStatistics(createdBy));
}

ColumnChunkMetaData buildColumnChunkMetaData(
      ColumnMetaData metaData, ColumnPath columnPath, PrimitiveType type, boolean fileHasCorruptStats) {
    SortOrder expectedOrder = overrideSortOrderToSigned(type) ? SortOrder.SIGNED : sortOrder(type);
    return ColumnChunkMetaData.get(...,
        fromParquetStatisticsInternal(metaData.statistics, type, expectedOrder, fileHasCorruptStats), ...);
}

No need to pass createdBy downstream, the boolean is all the internal overload needs. SortOrder computation moves here since we bypass fromParquetStatistics to avoid re-parsing createdBy as you have already done by replacing fromParquetStatisticsInternal with fromParquetStatistics.

Also notice how the new public methods we extracted in CorruptStatistics are being used in each delegate method here

[yadavay-amzn]

Done. buildColumnChunkMetaData now delegates to a package-private overload taking the boolean, with the SortOrder computation moved into it; the public (..., String createdBy) overload is preserved for back-compat.

[asif-moh]

This check in shouldIgnoreStatistics is dead code with current changes as we always pass BINARY

if (columnType != PrimitiveTypeName.BINARY && columnType !=PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY)

We can utilize the new methods here. Please refer to below comments for context.

if (!(fileHasCorruptStats && CorruptStatistics.isCorruptStatisticsColumnType(type.getPrimitiveTypeName()))

Instead of calling or moving the PrimitiveTypeName checks to ParquetMetadataConverter and leave it as the responsibility of CorruptStatistics

if (!CorruptStatistics.shouldIgnoreStatistics(createdBy, type.getPrimitiveTypeName())

[yadavay-amzn]

Done. Removed the now-dead column-type check; the gate is expressed via CorruptStatistics.isCorruptStatisticsColumnType in fromParquetStatisticsInternal.

[yadavay-amzn]

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

[asif-moh]

LGTM! Thanks for making the changes

[wgtmac]

IMHO, we need to be cautious when adding a new public method because we have to maintain it forever. Can we remove it from here and just let downstream where calls it to use a private method instead?

[asif-moh]

We need fileHasCorruptStatistics (or mayHaveCorruptStatistics after rename) to remain public, otherwise we'd have to duplicate the version-parsing logic in ParquetMetadataConverter. We can make isCorruptStatisticsColumnType private and inline the column-type check downstream.
Or we can overload shouldIgnoreStatistics(String createdBy) but either way we will have 1 new public method.

@yadavay-amzn @wgtmac wdyt?

[yadavay-amzn]

Went with your suggestion - kept mayHaveCorruptStatistics public (ParquetMetadataConverter needs it) and made isCorruptStatisticsColumnType private, inlining the column-type check (BINARY || FIXED_LEN_BYTE_ARRAY) at its call sites. Net new public API is just mayHaveCorruptStatistics(String). Behavior unchanged; parquet-column + parquet-hadoop tests pass.

[yadavay-amzn]

Minimized the new public surface: isCorruptStatisticsColumnType is now private with the column-type check inlined at its call sites, so mayHaveCorruptStatistics(String) is the only new public method. It has to stay public because ParquetMetadataConverter relies on it (otherwise the version-parsing logic would be duplicated).

[wgtmac]

Rename it to mayHaveCorruptStatistics ?

[yadavay-amzn]

Renamed to mayHaveCorruptStatistics.

[yadavay-amzn]

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

[wgtmac]

Could you resolve the conflict?

[yadavay-amzn]

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

[asifsmohammed]

@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 updates! I took another look and I still do not think this is the right shape yet.

I have to admit that my earlier suggestion may have pushed this in the wrong direction. I suggested factoring out a file-level corrupt-stats check, but I do not think we should merge a partial solution with side effect. My concerns with the current implementation are:

1. CorruptStatistics.mayHaveCorruptStatistics adds a new public API for an internal optimization. It is tied to PARQUET-251 semantics, so we would need to maintain that specific meaning as public API.

2. mayHaveCorruptStatistics is not a pure predicate. It can log warnings and mutate alreadyLogged. Precomputing it can therefore have user-visible side effects.

3. The fix is still partial. The main footer path avoids the row-group x column parse, but other production paths still parse created_by on each call. Examples include fromParquetStatisticsInternal(String createdBy, ...), buildColumnChunkMetaData(..., String createdBy), page-level stats in ParquetFileReader, and lazy encrypted metadata in ColumnChunkMetaData.

4. The current fileHasCorruptStats boolean leaks too much PARQUET-251-specific logic into ParquetMetadataConverter. The converter now knows about the schema gate, BINARY / FIXED_LEN_BYTE_ARRAY, and the file-level corrupt-stats flag. This pattern will not scale well if we later need another writer-version-based statistics workaround.

5. The schema-gate test does not prove the schema gate. testSchemaGateSkipsCorruptStatsCheckForNonBinarySchema only proves INT stats are preserved. That would still pass because of the later per-column gate. It does not prove that parsing or the corrupt-stats check was skipped.

I have not had enough time to fully design the final API, so please treat the following as a possible direction, not a complete design. A cleaner approach may be to cache the parsed created_by result, not a PARQUET-251-specific boolean.

Java already has VersionParser.ParsedVersion in parquet-common. parquet-hadoop already depends on parquet-common. There is also precedent in CorruptDeltaByteArrays.requiresSequentialReads(ParsedVersion, Encoding).

The converter could parse created_by once per file into a ParsedVersion, pass that ParsedVersion through the converter, and keep the PARQUET-251 decision inside CorruptStatistics.

For example:

    fromParquetStatisticsInternal(writerVersion, metaData.statistics, type, typeSortOrder)

And CorruptStatistics could have:

    shouldIgnoreStatistics(ParsedVersion writerVersion, PrimitiveTypeName columnType)

The existing String-based API can stay as a compatibility wrapper. This keeps the optimization in ParquetMetadataConverter, keeps the version-specific logic in CorruptStatistics, and avoids exposing mayHaveCorruptStatistics or threading a PARQUET-251 boolean through generic metadata conversion code.