[I] SpillWriter::path() conflates "nothing spilled" with "backend has no local path" [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@datafusion.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [I] SpillWriter::path() conflates "nothing spilled" with "backend has no local path" [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove commented on issue #5705:
URL: https://github.com/apache/datafusion-comet/issues/5705#issuecomment-5552616632

   Tracked under #5709 ([EPIC] DataFusion 55 upgrade follow-ups).


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [I] SpillWriter::path() conflates "nothing spilled" with "backend has no local path" [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove closed issue #5705: SpillWriter::path() conflates "nothing spilled" with "backend has no local path"
URL: https://github.com/apache/datafusion-comet/issues/5705


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org