comphead commented on PR #7603: URL: https://github.com/apache/datafusion-comet/pull/7603#issuecomment-6860786970
Thanks @andygrove, this was useful. Since your review the `parquet.*` rule changed in bd286dedb0: only `parquet.columnindex.truncate.length` still falls back, and the zstd level and worker count are now treated like the other codec-level side channels the user guide already lists. Details below. **1. A test that writes with the admitted settings.** `CometIcebergWriteActionSuite` has a new test, `native acceleration: parquet-mr settings iceberg-java ignores leave its files unchanged`. It writes one Parquet source to four tables, reusing the source and footer helpers of the dictionary-page tests. - With every inert key set as a table property, at a value that would change the file if the writer read it (`parquet.block.size=65536`, `parquet.page.size=1024`, `parquet.writer.version=PARQUET_2_0`, `parquet.compression=GZIP`, bloom filters on, statistics off, and so on), iceberg-java writes a file byte-identical to the table without them. Comparing bytes rather than footers matters. As a check I treated `parquet.columnindex.truncate.length` as inert: the test failed, while the two footer summaries still looked the same, because the column index sits outside them. - The native write of the same table engages `CometIcebergWriteExec` and matches iceberg-java's rows and dictionary pages. It passed locally on Spark 3.4 (Iceberg 1.5.2), 3.5 (1.8.1) and 4.1 (1.11.0). The 4.0 profile could not resolve `bcprov-jdk18on` 1.78.1 offline here, so 1.10.0 is not covered by a local run. **2. Making the re-audit fail the build.** The same test reflects over `ParquetOutputFormat` and `ZstandardCodec` and fails on any key it has not classified as inert, gated or a side channel. One finding changed what it has to read: iceberg-spark-runtime relocates its own parquet-mr under `org.apache.iceberg.shaded`, and that copy is what iceberg-java writes with, so the test finds the classes through `ParquetCodecFactory`'s superclass. The bundled versions are not Spark's: parquet-mr 1.13.1, 1.15.0, 1.16.0 and 1.17.1 for Iceberg 1.5.2, 1.8.1, 1.10.0 and 1.11.0. 1.17.1 adds `parquet.enable.bytestreamsplit`, which my audit of parquet-hadoop 1.16 had missed. Iceberg 1.11's writer never reads it, and the test now checks that along with the other inert keys. **3. The other spellings of the codec level.** I went the other way from gating them: all of them are side channels now. `zlib.compress.level`, `compression.brotli.quality` and `io.compression.codec.zstd.level` were already listed as accepted divergences on `main`, and the two `parquet.compression.codec.zstd.*` keys join them. They can only change compressed bytes, and the test checks that for the zstd level: the same row groups, encodings, codec and statistics, with different compressed sizes. The `io.compression` spelling only reaches the bytes indirectly. `ZstandardCodec` reads the `parquet.` spelling, and `ParquetCodecFactory.cacheKey` uses the `io.compression` one only as the key of parquet-mr's JVM-wide `CODEC_BY_NAME` cache, so its effect depends on what another writer cached under that key. Gating these would make every write fall back on a cluster that sets a zstd level, for a difference the user guide already accepts. The comment on `UnsupportedParquetMrKeys` now names a ll of them. If you would rather Comet honor these levels natively for size parity, I can open an issue to translate them. -- 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]
