kita-renji commented on issue #51495:
URL: https://github.com/apache/arrow/issues/51495#issuecomment-5843131416

   I can reproduce this on Linux with pyarrow 23.0.1 and 25.0.1, and I think 
I've found the cause. It's a race in `MergedGenerator`, not the filesystem.
   
   On a plain `LocalFileSystem` I couldn't hit it in ~15k iterations. Routing 
the same files through a `pyarrow.fs.PyFileSystem` wrapper (which adds thread 
handoffs) makes it show up at roughly 1-3% with 8 files and one 25-byte bad 
file. That's with both `use_threads=False` and `True`, and even with 
`fragment_readahead=1`.
   
   The root cause is in `cpp/src/arrow/util/async_generator.h`. When an inner 
subscription (here: the fragment whose `GetReaderAsync` fails) delivers an 
error while no caller is waiting, `InnerCallback` sets `state->broken = true` 
under the mutex, releases it, and only then stores the error in 
`MarkFinalError()` (`final_error = err`). If the consumer calls `operator()` in 
that window, it sees `broken` with `final_error` still OK and gets a plain 
end-of-stream. The source node finishes normally, the plan completes OK, and 
`ToTable()` returns whatever batches were already delivered. `OuterCallback` 
has the same pattern.
   
   This also fits the "shape" observation. The unlocked path is only taken when 
nobody is waiting, i.e. while the consumer is still processing the previous 
batch. A 25-byte file fails its footer check almost immediately after that 
batch is handed off, so the two collide. A larger half-written file fails 
later, when the consumer is usually already waiting, and that path delivers the 
error safely.
   
   A standalone C++ loop hits it naturally about 1 in 200k runs. With a 200µs 
sleep injected before the `final_error` store it goes to 1990/2000. For 
pyarrow, I rebuilt `libarrow_dataset.so` from apache-arrow-25.0.1 and swapped 
it into the 25.0.1 wheel: 57/4500 truncated with the unmodified source, 0/11.6k 
with the fix.
   
   The fix is small: store `final_error` while still holding the lock (and only 
defer the `all_finished` callback for the case where a caller is already 
waiting). I'm happy to open a PR with it and a regression test if that sounds 
right.
   
   Until then, the workaround from the issue (opening each footer with 
`pq.ParquetFile(path).metadata` before the scan) is a reasonable guard.
   


-- 
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]

Reply via email to