andygrove opened a new issue, #5705:
URL: https://github.com/apache/datafusion-comet/issues/5705
### Describe the bug
DataFusion 55 replaced the shuffle spill file's concrete
`RefCountedTempFile` with `Arc<dyn SpillFile>`, whose `path()` returns
`Option`. `SpillWriter::path()` now folds two different situations into one
`None`:
```rust
pub(crate) fn path(&self) -> Option<&std::path::Path> {
self.spill_file
.as_ref()
.and_then(|spill_file| spill_file.temp_file.path())
}
```
- nothing was spilled for this partition (`spill_file` is `None`), and
- the spill backend has no local path (`temp_file.path()` is `None`).
`local_partition_writer.rs` treats both as "skip the copy":
```rust
if let Some(writer) = spill_writers.get(pid) {
if let Some(spill_path) = writer.path() {
...std::io::copy(&mut spill_file, output_writer)?;
}
}
```
so a pathless spill backend would silently drop spilled data while the
recorded offsets keep advancing, producing a corrupt shuffle file rather than
an error.
### Steps to reproduce
Not reachable today: `create_tmp_file` always hands back a
`RefCountedTempFile`, whose `path()` always returns `Some`. `TempFileFactory`
is pluggable, so a future or custom backend could return `None`.
### Expected behavior
The two cases stay distinct, so a pathless backend fails loudly instead of
silently writing a truncated partition.
### Additional context
Found while reviewing #5262.
--
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]
