Repository navigation
Conversation
…nnot be loaded parquet-mr compresses and decompresses brotli pages with Hadoop's org.apache.hadoop.io.compress.BrotliCodec, which neither Spark nor Iceberg ships. Without it, iceberg-java fails a brotli write, but the native writer wrote the files anyway, and Spark without Comet could not read them back. The eligibility gate now declines brotli unless that class loads. A gzip write with a numeric level, which iceberg-java also fails, stays native: its files are ordinary gzip pages that every reader can read. The user guide lists it as an accepted divergence. Closes apache#6960.
4c2788b to
0eb9d0c
Compare
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the full four-file diff from d0f367dc02520fe68f524db22083cc0af6e58bca to f7d2593b8eca6987f36c3a7fa2ece7c1ce63812a. The PR is not a draft. The snapshot and live discussion contain no reviews, conversation comments, or inline comments.
Summary
- Prior state and problem: Native Iceberg writes admitted Brotli without Hadoop’s
BrotliCodec, allowing successful commits that the same installation’s JVM readers could not read. - Design approach: Add a driver-side eligibility rule that declines effective Brotli writes when the codec class cannot load, leaving iceberg-java to handle the write.
- Correctness: Checked Spark’s codec mapping and application class-loader setup, Iceberg’s effective write properties, and Parquet’s codec loading. The new rule uses the resolved configuration, preserves compression-level rejection precedence, and catches missing-class and linkage failures. No introduced P1/P2 correctness issue was identified.
- Compatibility analysis: Compared relevant sources for Spark
3.4.3,3.5.9,4.0.4,4.1.3, and4.2.0, with Iceberg1.5.2,1.8.1,1.10.0, and1.11.0. The Brotli class name and option/session/table precedence support this shared implementation. No version-specific shim or wire-format change is needed. - Key design decisions: Checking class availability without initialization avoids constructing a codec during planning. Looking through the context class loader accommodates Spark application jars. Running the rule after level validation preserves the more specific diagnostic.
- Implementation sketch:
CometIcebergNativeWriteadds the rule,IcebergWriteProtoTranslationshares codec-name normalization, the detection suite checks table/session settings and both availability outcomes, and the user guide documents the fallback and existing gzip divergence. - Performance: Additional work is confined to eligibility checks: codec-name normalization and, only for Brotli, class lookup. There is no added per-row or per-batch work. No evidence-backed performance regression was found.
- Design: The change fits the existing ordered eligibility rules and prevents incompatible output before writing starts. Its scope is appropriately small.
- Abstraction & complexity: Sharing
codecNameprevents the gate and serializer from interpreting codec names differently. ReusingClassLoaders.contextOrDefaultavoids another class-loader policy. No unnecessary abstraction was identified. - Behavioral changes worth calling out: Compared with latest release branch
branch-1.1at89c729a9697467e8f8d89e3eadf1797a75e2328c, Brotli writes without the codec now fall back instead of writing natively. This is the intended compatibility correction. Numeric gzip-level behavior is unchanged and newly documented. - Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified.
Routed skills: review-comet-pr, review-comet-iceberg-write-pr, and review-comet-expression-pr for the serde paths. No scalar expression implementation changed.
Exact-head CI: 25 checks succeeded, 18 were skipped, and none failed or remained running. The CI merge commit 29cae167d6a33f8ff0ccbbf3a067e00914ed3a0e has the same tree as the reviewed head. The Spark 4.1 scans job explicitly ran the new Brotli case and both affected suites, with 738 tests passing overall. Upstream Iceberg/Spark SQL suites and macOS were skipped.
Validation: Bounded local probes passed for the extracted new helper methods, including normalization, unknown/default codecs, missing classes, linkage errors, null context loaders, and loading without initialization. Actual Parquet 1.16.0 and Iceberg 1.11.0 shaded Parquet reproduced the missing-Brotli failures. Hadoop 3.4.2 reproduced numeric gzip-level rejection.
Limits: No local full Comet build, end-to-end native write, or upstream Iceberg/Spark SQL suite was run. The local helper checks were isolated probes, and no real external Brotli codec jar was tested. Older-profile runtime results reported by the author were not independently rerun. Project files remain unchanged.
No introduced P1/P2 issues found within this review. No substantiated unresolved existing P1/P2 blockers were identified.
Which issue does this PR close?
Closes #6960.
Rationale for this change
parquet-mr compresses and decompresses brotli pages with Hadoop's
org.apache.hadoop.io.compress.BrotliCodec, which it loads by name. Neither Spark nor Icebergships that class, and the Iceberg runtime's bundled copy of Parquet looks up the same one. Without
a brotli codec jar, iceberg-java fails a brotli write. The eligibility gate admitted brotli
anyway, and parquet-rs compresses it without the class, so a native write committed a table that
Spark without Comet cannot read. It fails with
BadConfigurationException: Class org.apache.hadoop.io.compress.BrotliCodec was not found, and sowould Comet whenever its scan falls back to Spark's reader.
#6960 also reported that iceberg-java fails a gzip write with a numeric
write.parquet.compression-level, which the native writer accepts. That case is different: Hadoopreads
zlib.compress.levelas the name of aZlibCompressor.CompressionLevelconstant, so5throws, but the native files are ordinary gzip pages that every reader can read. Spark 4.1.1's
parquet-mr reads a gzip level 5 file without trouble. This PR documents that difference instead of
making the native path fail as well.
What changes are included in this PR?
requireLoadableBrotliCodec, declines a brotli write unlessorg.apache.hadoop.io.compress.BrotliCodecloads through the thread context class loader, whichSpark adds application jars to. It loads the class without initializing it, so the check runs no
codec code. It reads the effective codec, so
spark.sql.iceberg.compression-codecand thecompression-codecwrite option count too. It runs after the compression level rule, so anout-of-range brotli level still reports the level.
IcebergWriteProtoTranslation.compressesWithBrotlireads the codec name asresolveCompressiondoes, but never throws, so an unknown codec such as
lzostill fails where it did before.iceberg-writes.md): the eligibility table's compression row and a new paragraphdescribe the brotli rule, and the accepted-divergences entry on codec settings now covers a
numeric gzip level. The table cell keeps its width, so the rest of the table is not re-padded.
#6961 adds a list of plans for each fallback to the same page. Whichever of the two merges second
should add this rule to its "Not planned" list, beside the other writes iceberg-java itself
rejects.
How are these changes tested?
A new
CometIcebergWriteDetectionSuitecase plans two brotli writes without running them, oneset by a table property and one by
spark.sql.iceberg.compression-codec, and checks that eachfalls back with the new reason. Under a context class loader that resolves the class name, as one
with a brotli codec jar would, both are
Compatible. Breaking the rule either way, never decliningor always declining, fails the test.
Locally,
CometIcebergWriteDetectionSuiteandIcebergWriteProtoTranslationSuitepass on thedefault Spark 4.1 profile (104 tests) and on the Spark 3.4 profile with Iceberg 1.5.2 (100 passed,
4 canceled by their version checks).
An end-to-end check, not committed, ran a brotli and a zstd
INSERTwithspark.comet.write.iceberg.enabled=trueandspark.comet.exec.localTableScan.enabled=true:main: the brotli plan showsCometIcebergWriteand the insertsucceeds, and reading the table with
spark.comet.enabled=falsefails withBadConfigurationException: Class org.apache.hadoop.io.compress.BrotliCodec was not found.IcebergWrite, and the insert fails with that same error,as it does without Comet.
The Iceberg Spark test suites are unaffected: none of them writes brotli (checked on a recent
Iceberg
main), and Iceberg's build adds no brotli codec.