Repository navigation
perf: read native-written Parquet footers without parsing Hadoop's configuration - #6923
Conversation
…nfiguration After a native Iceberg write, every data file's footer is re-read so that its metrics come from iceberg-java. ParquetFileReader.open(InputFile) builds its read options from a new Hadoop Configuration on parquet-mr 1.14 and later, which parses Hadoop's default XML resources once for every data file. Read the footer with parquet-mr's default options built from a PlainParquetConfiguration instead, and keep open(InputFile) on older parquet-mr, whose defaults are built without Hadoop.
sunchao
left a comment
There was a problem hiding this comment.
No introduced P1/P2 issues found within this review.
Reviewed full SHA f3313b1ebb7ac9b0c1cb57dc58492758a4dec0dd against base 42db91b2e892a28965248c8ba84c941eeb6769e8. The complete PR diff contains two files and one commit, with no stacked prerequisites. Ancestry checks distinguish three newer main-only commits from the PR changes. The PR remains non-draft. The snapshot and live discussion contain no reviews or comments.
Routed skills: review-comet-pr and review-comet-iceberg-write-pr. Read AGENTS.md and the relevant contributor documentation.
Summary
- Prior state and problem: Each native-written Iceberg data file was reopened through
ParquetFileReader.open(InputFile). On newer parquet-mr versions, building the read options repeatedly parses Hadoop configuration resources during metrics reconstruction. - Design approach: Supply default options backed by
PlainParquetConfiguration, retaining the existing overload for parquet-mr versions without that API. - Correctness: Checked footer decoding, statistics handling, reader closure, and iceberg-java’s metrics calculation against upstream sources.
ParquetUtil.footerMetrics, float/double field metrics, and cleanup remain intact. Local differential validation found identical footers and metrics. No Spark expression or operator semantics change. - Compatibility analysis: Checked the Iceberg 1.5.2, 1.8.1, 1.10.0, and 1.11.0 dependency versions and relevant Parquet APIs. Deriving class names from the resolved reader preserves shaded and unshaded compatibility. Spark 3.4 CI explicitly passed the new test against the legacy fallback.
- Key design decisions: The existing driver-side reflection probe also resolves the new members. Resolution stays tied to the current class loader, and each file receives fresh read options.
- Implementation sketch:
FooterReadOptionsholds the reflected constructor and builder/open methods.resolveParquetFooterReflectionresolves this optional capability once per metrics-rebuild call, andreadParquetFooterselects the appropriate overload. - Performance: The source confirms that the new path avoids Hadoop-backed option lookups. Per-file footer I/O and option construction remain. The author’s fanout benchmark supports the intended benefit, but I did not independently reproduce its timing. No evidence-backed performance regression was identified.
- Design: The change is localized and preserves iceberg-java as the authority for committed metrics. It addresses configuration overhead without introducing a new native-to-JVM footer transport.
- Abstraction & complexity: The small reflection holder fits the existing compatibility layer. Its separation of member resolution from option creation is justified by version and relocation differences. No unnecessary abstraction meeting the reporting bar was found.
- Behavioral changes worth calling out: Footer read options stop inheriting executor-side
parquet.*defaults. File access still uses the existingFileIO, and write eligibility is unchanged. Compared withbranch-1.1atdc4295ffe4492de42e1b863a3bb5fe96c8551990, this is an intended change to the existing footer-reading path. - Suggested improvements: No additional code change meeting the P1/P2 reporting bar was identified.
Exact-head CI: 130 checks succeeded, 61 were skipped, and three failed. The main PR run passed, including all Spark-profile Comet groups and Iceberg 1.8–1.11 matrices. The separate Iceberg label run failed while resolving Gradle dependencies with Network is unreachable. Its missing shard reports caused the coverage and aggregate failures. These failures do not substantiate a product defect.
Validation: Compiled the exact-head reflection implementation and ran all 27 IcebergReflectionSuite tests in an isolated JVM with cached Spark 4.1.3/Iceberg 1.11.0 dependencies. All passed. A disposable differential harness compared the old and new paths across 19 repository Parquet fixtures, with identical footer metadata and all seven metrics fields. Full Maven/native builds, native-write integration tests, object-store writes, and performance benchmarks were not rerun locally. Integration coverage relies on the exact-head CI results. Project files remain unchanged.
Which issue does this PR close?
Closes #6912.
Rationale for this change
After the native Iceberg writer finishes a task,
IcebergReflection.rebuildDataFilesWithJavaMetricsre-reads every data file's footer so that the committed metrics come from iceberg-java'sParquetUtil.footerMetrics. It opened each file withParquetFileReader.open(InputFile). On parquet-mr 1.14 and later, that call builds its read options from a new HadoopConfigurationand reads them back, which parses Hadoop's default XML resources every time: once per data file. That covers every Iceberg version Comet supports except 1.5.2 (Spark 3.4), which bundles parquet-mr 1.13.1.Profiling #6910's
CometIcebergEndToEndBenchmarkput most of the metrics rebuild inConfiguration.loadResources, and a fanout write pays it for every file it writes.What changes are included in this PR?
readParquetFooteropens the file withParquetFileReader.open(InputFile, ParquetReadOptions), passing parquet-mr's default read options built from aPlainParquetConfiguration, so no HadoopConfigurationis created. The classes are found next to the possibly relocatedParquetFileReader, as the existing reflection already does.ParquetConfiguration(1.13.1), the footer is still read throughopen(InputFile), whose defaults are built without Hadoop.parquet.*keys from an executor-sidecore-site.xml. Read options cover filtering, decryption, allocation, and how string statistics from old parquet-mr writers are read, none of which applies to a footer the native writer has just written.How are these changes tested?
IcebergReflectionSuitetest checks that the footer read options carry aPlainParquetConfigurationwhere parquet-mr has one, and that only parquet-mr withoutParquetConfigurationfalls back. It passes on the default profile (Iceberg 1.11, parquet-mr 1.17.1) and on Spark 3.4 (Iceberg 1.5.2, parquet-mr 1.13.1).CometIcebergWriteActionSuitepasses. Its metrics-parity tests compare the rebuilt metrics with iceberg-java's writer.local[5]on an Apple M3 Ultra, async-profiler attached to both runs):bucket(32, id)In the fanout case, the write stage's task CPU drops from 3.2 s to 1.7 s per statement, about 4.7 ms per file written.
The change is version-sensitive (parquet-mr differs per Iceberg version), so this PR carries
run-iceberg-testsandrun-all-spark-profiles.