andygrove opened a new pull request, #7009: URL: https://github.com/apache/datafusion-comet/pull/7009
## Which issue does this PR close? No issue. I audited the docs that describe native Iceberg writes against the code on `main`. ## Rationale for this change The Iceberg write guides had statements that were wrong from the start or went stale as the native writer changed, and other pages that mention native writes had drifted the same way. Most of the errors were in the user guide's accepted-divergences list, which is what a user reads to decide whether natively written files are acceptable, so it should describe what the two writers actually write. ## What changes are included in this PR? User guide (`iceberg-writes.md`): - A native file's `created_by` is `Apache Iceberg <version> (Comet)`, which `CometIcebergNativeWrite` builds, not a parquet-rs string. - iceberg-java writes a CRC in every page header and parquet-rs writes none. Neither writes page-header statistics, so the old item's argument about missing page statistics had nothing to explain. - Fixed-length binary columns are not a divergence. parquet-mr's v1 writer and parquet-rs at `PARQUET_1_0` both write them plain. The item is removed, and so is the closing sentence's claim that every other encoding matches, which the dictionary-choice item already contradicted. - parquet-rs closes row groups by size here (`set_max_row_group_bytes`), not by row count. - Both writers name files `<partition>-<task attempt>-<operation>-<count>` with a counter per task. iceberg-rust's counter is not process-local. - A `zstd` table with no level is written at parquet-mr's default of 3, so the default level is not a source of different compressed bytes. - The accepted-divergences intro no longer promises the path-layout caveat that #5651 removed. - Iceberg's own `SparkWrite.abort` skips cleanup only from Iceberg 1.6. Iceberg 1.5, which the Spark 3.4 profile pins, deletes completed tasks' files. - `rewrite_data_files` is listed as supported and streaming writes as a fallback. The fallback example "a `ReplaceData` projection it cannot map" is replaced, since the Spark 4.x shim always maps it. - The early-close file count matches the test (four to five files per partition), and the guide says most eligibility rules, not every one, are pinned by `CometIcebergWriteDetectionSuite`. Contributor guide (`iceberg-writes.md`): - The Iceberg suites skip on Spark 4.2, where `icebergAvailable` is false, so the 4.2 profile is not tested against Iceberg 1.10.0. - The caveat that Comet's fallback reasons never reach the Iceberg Spark test logs is stale since #6155. It now points at the write report on each job's summary page. - A task that wrote no files sends a manifest with no entries, not empty bytes. - iceberg-rust stopped panicking on pre-1970 `timestamptz` values with the pin bump in #6649. - The row lineage rule is pinned by `CometIcebergWriteActionSuite`. The dictionary hold-back gives its 100-row floor, and the footer-read cost is two small reads per file, as in the user guide. Other pages: - `iceberg-spark-tests.md` named per-version workflow files that #4494 deleted. It now names the `ci.yml` jobs, and the manual install step builds Comet for the Spark and Scala versions that step 3 tests. - `understanding-comet-plans.md` no longer says the write operators fuse with the native operators below them. Each runs its own native plan over its child. - `operators.md` says a native Iceberg write needs `LocalTableScanExec` converted to write `INSERT ... VALUES` input natively. - `tuning/memory.md` says a fanout Iceberg write closes files early instead of failing, and counts the files a native writer holds open among what the pool tracks. - `iceberg.md`, the 1.2.0 upgrade-guide entry and the Iceberg write review skill are reworded where they overstated what the default plan or the split plan covers. The audit also found that the gate accepts `write.parquet.compression-codec=none`, which iceberg-java rejects. That needs a code change, so it is filed as #7008. ## How are these changes tested? Docs only, so there are no tests. `npx prettier@latest --check` passes on every changed file. Each corrected statement was checked against the code on `main`, the pinned iceberg-rust revision, the parquet-rs 59.3.0 source, and the iceberg-java and parquet-mr sources for the versions the Spark profiles pin. #6851 and #6996 rewrite the float-partition text and the gzip and brotli text, so this PR leaves those parts alone. Both still merge cleanly with it, and so does #6961. -- 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]
