andygrove opened a new pull request, #5810:
URL: https://github.com/apache/datafusion-comet/pull/5810
## Which issue does this PR close?
Closes #5776.
## Rationale for this change
iceberg-rust's `FanoutWriter` keeps its per-partition writers in a `HashMap`
and closes them by iterating it directly:
```rust
async fn close(mut self) -> Result<O> {
for (_, mut writer) in self.partition_writers {
self.output.extend(writer.close().await?);
}
Ok(O::from_iter(self.output))
}
```
Under Rust's per-process `RandomState` that gives a different `DataFile`
order on every run. That order becomes the manifest entry order, which becomes
the scan-task order, which becomes the row order of an unordered `SELECT *`.
Iceberg's own
`TestMetadataTablesWithPartitionEvolution.testPartitionColumnNamedPartition`
compares such a `SELECT *` positionally against what iceberg-java wrote, so it
failed whenever the two disagreed.
Measured on Spark 4.1 + Iceberg 1.11 by writing eight partitions through the
fanout writer and printing the manifest order on three consecutive runs:
```
run 1: partition=1, partition=2, partition=5, partition=0, partition=7,
partition=4, partition=3, partition=6
run 2: partition=6, partition=7, partition=4, partition=5, partition=0,
partition=3, partition=2, partition=1
run 3: partition=2, partition=0, partition=4, partition=5, partition=7,
partition=3, partition=6, partition=1
```
The clustered writer produced `partition=0 .. partition=7` on every run.
Two notes on the issue as filed. The failing parameter set includes ORC and
AVRO, which looked like evidence the Parquet writer could not be involved; it
is not, because that test does not use the class's `createTable` helper and so
writes Parquet whatever the `fileFormat` parameter says. And the test only has
two partitions, so an unfixed writer already passes about half the time, which
is why only a subset of the matrix reported failures.
## What changes are included in this PR?
- Sort the fanout writer's output by file path in `InnerWriter::close`.
It cannot be creation order instead: `RecordBatchPartitionSplitter` splits
a batch through a map as well, so which partition is written first, and
therefore which file-name counter it gets, is itself unstable. Path order sorts
by partition directory and then by that counter within a partition, and needs
nothing from the write order. The clustered and unpartitioned writers append in
creation order and are already deterministic, so only the fanout arm sorts.
- Record the behaviour under accepted divergences in `iceberg-writes.md`.
This is determinism, not parity: iceberg-java's fanout writer iterates its own
`StructLikeMap`, so the two agree only where that order and path order
coincide, which they do for the ascending partition values the upstream test
uses.
Part of #5649, and one of the two remaining failure buckets blocking #5644.
## How are these changes tested?
A Rust test (`fanout_write_returns_data_files_in_a_deterministic_order`) and
a Scala test (`a fanout write lists its data files in a stable order`), both
over eight partitions rather than the upstream test's two, so an unfixed writer
lands in the right order by luck 1 time in 8! rather than half the time. Both
were confirmed to fail three times out of three on the unfixed writer and pass
three times out of three with the fix.
The end-to-end scenario from the issue was also swept over
`spark.sql.shuffle.partitions` in `{1, 2, 4, 200}` against both writers:
`parts=2` with the native writer was the combination that reproduced the
reordering, and all eight combinations now return the expected rows on three
consecutive runs.
`CometIcebergWriteActionSuite` (62), `CometIcebergWriteDetectionSuite` (48),
`CometIcebergSystemFunctionSuite` (13), `CometIcebergRewriteActionSuite` (5)
and the native `iceberg` tests (53) pass.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_018sqhRddDS2gFfJRT21QDsE
--
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]
