andygrove opened a new pull request, #5726: URL: https://github.com/apache/datafusion-comet/pull/5726
## Which issue does this PR close? Closes #5705. ## Rationale for this change `SpillWriter::path()` returned `Option<&Path>`, which folds two different situations into `None`: - nothing was spilled for this partition (`spill_file` is `None`), and - a spill file exists but its backend exposes no local path (`SpillFile::path()` is `None`). `LocalPartitionWriter::finish_partition` reads `None` as "nothing to copy" and moves on, but `self.offsets[pid]` and the trailing total length keep advancing. A pathless backend would therefore write an index claiming bytes that never landed in the data file, so the reader would decode the next partition's block as this one's — a corrupt shuffle file rather than an error. This is not reachable today: the default `DiskManager` backend always hands back a spill file with a local path. But `TempFileFactory` is pluggable, so a future or custom backend could return `None`, and this should fail loudly rather than silently truncate. ## What changes are included in this PR? - `SpillWriter::path()` now returns `datafusion::common::Result<Option<&Path>>`, keeping the three outcomes distinct: `Ok(None)` for nothing spilled, `Ok(Some(path))` for reachable spilled bytes, and `Err(..)` for spilled-but-pathless. - The `finish_partition` call site becomes `writer.path()?`, so an unreachable spill file fails the shuffle task instead of skipping the copy. ## How are these changes tested? Three new unit tests, plus a test-only spill backend (`pathless_backend`) that installs a `TempFileFactory` whose `SpillFile` leaves `path()` at the trait default and sinks its bytes, via `DiskManagerBuilder::with_temp_file_factory`: - `path_is_none_when_nothing_spilled` — the benign case stays benign. - `path_errors_when_backend_has_no_local_path` — asserts `has_spill_file()` before checking the error, so the test cannot pass vacuously by simply never spilling. - `finish_partition_fails_when_spill_has_no_local_path` — covers the corruption path end to end through `LocalPartitionWriter`. I checked the end-to-end test is not vacuous by temporarily swapping `writer.path()?` back to a lenient `unwrap_or(None)`: the test fails there and passes again once the `?` is restored. `cargo test -p datafusion-comet-shuffle` passes (124 tests), and `cargo clippy -p datafusion-comet-shuffle --all-targets -- -D warnings` is clean. -- 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]
