Skip to content

docs: correct native Iceberg write docs that drifted from the code #7009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andygrove merged 1 commit into from
Oct 15, 2026
Merged

docs: correct native Iceberg write docs that drifted from the code #7009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions .ai/skills/review-comet-iceberg-write-pr/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,11 @@ not, inherits it. There is no query that fails to tell you.
| Shared with the scan | `iceberg_common.rs` (`load_file_io`, `storage_factory_for`, `scheme_of`), `IcebergReflection`, `NativeConfig` S3 translation |

`spark.comet.write.iceberg.enabled` switches both write layers. It is off by default, but once a
user turns it on, a change to the split plan affects every Iceberg write, including writes that
never reach the native writer, and a change to the native writer affects every eligible one. The
testing-only `spark.comet.write.iceberg.splitOperator.enabled` plans the split operator with the
native writer off. A change to a shared file affects the native Iceberg scan too.
user turns it on, a change to the split plan affects every append, overwrite and copy-on-write
Iceberg write, including writes that never reach the native writer, and a change to the native
writer affects every eligible one. The testing-only
`spark.comet.write.iceberg.splitOperator.enabled` plans the split operator with the native writer
off. A change to a shared file affects the native Iceberg scan too.

## 2. Direction of the Gate Change

Expand Down Expand Up @@ -205,7 +206,7 @@ Read the ownership table in the contributor guide before reviewing any change ne
| Suite | Covers |
| ----------------------------------- | ----------------------------------------------------------------------------------------------- |
| `CometIcebergWriteActionSuite` | Both layers end to end, parity with iceberg-java, DML, failure cleanup, AQE re-plans |
| `CometIcebergWriteDetectionSuite` | One case per eligibility rule |
| `CometIcebergWriteDetectionSuite` | Most eligibility rules, accepted and declined |
| `CometIcebergSystemFunctionSuite` | Native partition transforms keeping partitioned writes native |
| `CometIcebergRewriteActionSuite` | `rewrite_data_files` through the split plan and the native writer |
| `IcebergWriteProtoTranslationSuite` | Property to `IcebergParquetWriteSettings` translation |
Expand All @@ -231,7 +232,8 @@ Ask specifically:
default Spark profile. Reflection, metrics conventions and path spelling differ across
Iceberg versions, so ask for the `run-iceberg-tests` label or a local run where they matter.
- [ ] **Is a green Iceberg Spark test run being cited as evidence?** It does not show which writer
handled a test ([#6148](https://github.com/apache/datafusion-comet/issues/6148)).
handled a test. The job summary counts writes per writer without naming tests (see "Which
writer ran each Iceberg write" in `contributor-guide/iceberg-spark-tests.md`).

## 8. Do the PR Changes Make the Docs Stale?

Expand Down
17 changes: 9 additions & 8 deletions docs/source/contributor-guide/iceberg-spark-tests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,11 @@ are still the reference, and are what you want when updating a diff.

## 1. Install Comet

Run `make release` in Comet to install the Comet JAR into the local Maven repository, specifying the Spark version.
Run `make release` in Comet to install the Comet JAR into the local Maven repository, specifying the Spark
and Scala versions the Iceberg tests will run against. The example below matches step 3.

```shell
PROFILES="-Pspark-4.1" make release
PROFILES="-Pspark-3.5 -Pscala-2.13" make release
```

## 2. Clone Iceberg and Apply Diff
Expand Down Expand Up @@ -121,12 +122,12 @@ diff must be generated against its own tag.

## Running Tests in CI

The `iceberg_spark_test_<version>.yml` workflows apply these diffs and run the three Gradle targets above
against each Iceberg version, all with Java 17. Iceberg 1.8.1 runs against Spark 3.4.3; Iceberg 1.9.1 and 1.10.0
run against Spark 3.5.9; Iceberg 1.11.0 runs against Spark 4.1.3. Iceberg 1.11.0 runs in the
merge queue; 1.8.1, 1.9.1 and 1.10.0 run once a night against `main`. All four run earlier on a
pull request labeled `run-iceberg-tests`; none runs on an unlabeled pull request. All caller
workflows delegate to `iceberg_spark_test_reusable.yml`, which holds the build and test job logic. See
The `iceberg_1_8`, `iceberg_1_9`, `iceberg_1_10` and `iceberg_1_11` jobs in `ci.yml` apply these diffs and
run the three Gradle targets above against each Iceberg version, all with Java 17. Iceberg 1.8.1 runs against
Spark 3.4.3; Iceberg 1.9.1 and 1.10.0 run against Spark 3.5.9; Iceberg 1.11.0 runs against Spark 4.1.3.
Iceberg 1.11.0 runs in the merge queue; 1.8.1, 1.9.1 and 1.10.0 run once a night against `main`. All four
run earlier on a pull request labeled `run-iceberg-tests`; none runs on an unlabeled pull request. Each job
calls `iceberg_spark_test_reusable.yml`, which holds the build and test job logic. See
[.github/workflows/README.md](https://github.com/apache/datafusion-comet/blob/main/.github/workflows/README.md)
for how the pull-request, merge-queue and nightly tiers differ.

Expand Down
42 changes: 25 additions & 17 deletions docs/source/contributor-guide/iceberg-writes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ Two layers, the second built on the first, both switched by `spark.comet.write.i
| The native writer | Who writes the data files. An eligible `IcebergWrite` becomes `CometIcebergWrite`, which writes Parquet with iceberg-rust. |

The flag defaults to `false`, which plans Spark's own write operator. With it on, a change to the
split plan reaches every Iceberg write and a change to the native writer reaches every eligible one.
split plan reaches every append, overwrite and copy-on-write Iceberg write, and a change to the
native writer reaches every eligible one.
The testing-only `spark.comet.write.iceberg.splitOperator.enabled` plans the split operator with the
native writer off, which the suites use to compare the two writers under the same plan. The
criteria for turning the flag on by default are tracked in
Expand Down Expand Up @@ -170,7 +171,8 @@ The gate has these properties, and a new rule has to keep them:
`scheme_of` split a location's scheme differently). Prefer sharing one implementation, or pin the
pair with a test that feeds both the same inputs.

Every rule is pinned by `CometIcebergWriteDetectionSuite`. Add a case there for any new rule, for
Most rules are pinned by `CometIcebergWriteDetectionSuite`, and the row lineage rule by
`CometIcebergWriteActionSuite`. Add a case to the detection suite for any new rule, for
both the accepted and the declined side. Update the eligibility table in the user guide in the same
change. Whether each existing restriction is permanent is tracked in
[#5643](https://github.com/apache/datafusion-comet/issues/5643).
Expand Down Expand Up @@ -238,9 +240,9 @@ Points where Comet adapts iceberg-rust to match iceberg-java:
- **Location generation.** `CometLocationGenerator` (`iceberg_partition_path.rs`) replaces
iceberg-rust's `DefaultLocationGenerator` and renders partition directories the way iceberg-java's
`PartitionSpec.partitionToPath` does. iceberg-rust's own rendering differs for several types, and
panics for pre-1970 `timestamptz` and for dates past `chrono`'s calendar. Float and double values
use Comet's Java `Double.toString` renderer (`java_float_string`), shared with
`cast(float as string)`. The partition type is resolved once when the generator is built, because
panics for dates past `chrono`'s calendar. Float and double values use Comet's Java
`Double.toString` renderer (`java_float_string`), shared with `cast(float as string)`. The
partition type is resolved once when the generator is built, because
`LocationGenerator::generate_location` cannot return an error.
- **Partition values.** `PartitionValueCalculator` (`iceberg_partition_value.rs`) replaces
iceberg-rust's calculator of the same name, and `PartitionSplitter` replaces its
Expand All @@ -264,9 +266,10 @@ Points where Comet adapts iceberg-rust to match iceberg-java:
column's encoding when the file opens
([#6114](https://github.com/apache/datafusion-comet/issues/6114)). So each partition's
`PartitionFeed` holds back the partition's first page of rows (`write.parquet.page-row-limit`,
capped by `write.parquet.row-group-size-bytes`, which the fanout feeds share) before any of them
reaches the writer. `DictionaryChooser` (`iceberg_dictionary.rs`) replays parquet-mr's accounting
over those rows and turns dictionary encoding off for the columns parquet-mr would write plain.
at least 100, capped by `write.parquet.row-group-size-bytes`, which the fanout feeds share) before
any of them reaches the writer, unless no column has a dictionary choice to make.
`DictionaryChooser` (`iceberg_dictionary.rs`) replays parquet-mr's accounting over those rows and
turns dictionary encoding off for the columns parquet-mr would write plain.
It reads only the columns there is a choice for, one at a time, and walks each lazily only as far
as its first page, so beyond the held rows it keeps no more than one column's dictionary. The
accounting follows `FallbackValuesWriter`, `DictionaryValuesWriter` and
Expand Down Expand Up @@ -347,7 +350,8 @@ backend ignores them. A fallback reason must contain property names only, never
Each task emits exactly one batch with one row and two `BINARY` columns (`build_output_schema`):

1. The task's `DataFile`s encoded as an Iceberg v2 data manifest, written by iceberg-rust's
`ManifestWriter` into an in-process `memory:` `FileIO`. Empty when the task wrote no files.
`ManifestWriter` into an in-process `memory:` `FileIO`. A task that wrote no files sends a
manifest with no entries.
2. Every location the task's writers were handed (`encode_locations`: a big-endian count, then a
length-prefixed UTF-8 string per location).

Expand All @@ -368,7 +372,8 @@ Each task emits exactly one batch with one row and two `BINARY` columns (`build_
Rebuilding metrics on the JVM is the main reason manifest parity holds: metrics modes, truncation,
the inferred-column cap and list/map bounds suppression are decided by iceberg-java's code. When
touching the native writer, do not try to make iceberg-rust's own metrics authoritative; the JVM
discards them. The price is one footer read per written file.
discards them. The price is reading each written file's footer back from storage, two small reads
per file.

## Failure Handling and Cleanup Ownership

Expand Down Expand Up @@ -438,7 +443,7 @@ the writer: run the write suites and the Iceberg Spark tests. The pin policy is
| Suite | What it covers |
| ---------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| `CometIcebergWriteActionSuite` | End-to-end writes through the split plan and the native writer: parity with iceberg-java, row-level DML, partition evolution, file order, cleanup on task and job failure, writes outgrowing the memory pool, AQE re-planning. |
| `CometIcebergWriteDetectionSuite` | One case per eligibility rule, accepted and declined. |
| `CometIcebergWriteDetectionSuite` | Most eligibility rules, each accepted and declined. |
| `CometIcebergSystemFunctionSuite` | Native `bucket`, `truncate`, `years`/`months`/`days`/`hours`, which keep a partitioned write's hash distribution and sort native end to end. |
| `CometIcebergRewriteActionSuite` | Iceberg's `rewrite_data_files` with the split plan and the native writer. |
| `IcebergWriteProtoTranslationSuite` | Translation of properties into `IcebergParquetWriteSettings` and the writer mode. |
Expand All @@ -448,9 +453,11 @@ the writer: run the write suites and the Iceberg Spark tests. The pin policy is
| `CometIcebergEndToEndBenchmark` | An Iceberg read, hash repartition and Iceberg write against stock Spark, over primitive, struct, array, map and nested columns. It checks each arm's plan and the rows it wrote before timing it. |

The Comet suites run against the Iceberg version each Spark profile pins in `spark/pom.xml`: 1.5.2
for Spark 3.4, 1.8.1 for 3.5, 1.10.0 for 4.0 and 4.2, and 1.11.0 for 4.1. Only the default profile
runs on every pull request. Behaviour that differs between Iceberg versions, such as reflection
targets, metrics conventions and partition path spelling, needs a test that runs on each.
for Spark 3.4, 1.8.1 for 3.5, 1.10.0 for 4.0, and 1.11.0 for 4.1. No Iceberg runtime is published
for Spark 4.2, so the 4.2 profile builds against the 4.0 runtime and the Iceberg suites skip there
(`icebergAvailable` is false on Spark 4.2). Only the default profile runs on every pull request.
Behaviour that differs between Iceberg versions, such as reflection targets, metrics conventions
and partition path spelling, needs a test that runs on each.

When writing a native-write test:

Expand All @@ -471,9 +478,10 @@ When writing a native-write test:

The upstream Iceberg Spark tests also run with the native writer and `localTableScan` enabled (see
[Running Iceberg Spark Tests](iceberg-spark-tests.md)). They are a broad regression net, but they do
not assert which writer ran, and Comet's fallback reasons do not appear in their CI logs, so a green
run is not evidence that the native writer handled a given test
([#6148](https://github.com/apache/datafusion-comet/issues/6148)).
not assert which writer ran, so a green run is not evidence that the native writer handled a given
test. Each job's summary page counts the writes each writer ran and lists the most common reasons
Comet declined the rest (see
[Which writer ran each Iceberg write](iceberg-spark-tests.md#which-writer-ran-each-iceberg-write)).

## Pitfalls

Expand Down
Loading