stackedsax commented on issue #45116: URL: https://github.com/apache/arrow/issues/45116#issuecomment-5981681567
**tl;dr:** I don't think this is a bug. I reproduced both the hang and the segfault with the sample code on Arrow 24.0.0 (macOS arm64, 14 cores, the attached `customer.parquet`), and both come down to how the sample uses the API: 1. Running `RecordBatchReader::ToTable()` with `use_threads(true)` *from inside* tasks on the CPU thread pool. The reader fans out onto the same pool and blocks, which deadlocks once the pool is full. 2. Creating a reader over an empty row-group range (the `<=` in the loop produces `[62,62]`), then indexing `vec_array[0]` on an empty vector after discarding the `Concatenate` error. 3. Discarding the `Status` from `GetRecordBatchReader`, which would hide any genuine read error the same way. Details below. **The hang** is a nested-parallelism deadlock: - `read_recordbatch` is submitted to `GetCpuThreadPool()`. - Inside it, `ToTable()` drives the batch reader. With `set_use_threads(true)`, each batch calls `OptionalParallelFor` over the columns (`cpp/src/parquet/arrow/reader.cc`, ~line 1172). - `ParallelFor` (`cpp/src/arrow/util/parallel.h`) submits one task per column to the *same* CPU pool, then blocks on `fut.status()` until they finish. - Once the outer tasks fill every worker, each worker is blocked waiting on column tasks that can never be scheduled. With 62 row groups and 14 cores, zero tasks complete. - This matches the report exactly: works while tasks < cores, stops once they exceed it. A bigger pool just moves the threshold. Either of these avoids it, and I verified both: - `arrow_reader_props.set_use_threads(false)` when you are already parallelising across readers yourself, so the reader doesn't fan out again inside your tasks. All 62 tasks complete on the default pool. - Don't block CPU-pool threads at all: `GetRecordBatchGenerator` is async and composes with the executor instead of blocking it. **The segfault** appears as soon as the deadlock is removed (e.g. `SetCapacity(128)`, which is what the reporter tried), and it's in the sample's lambda: - The loop `for (j = 0; j <= rg_batchsize; j++)` creates one extra reader with the empty range `[62,62]`. - For that reader, `ToTable()` correctly returns OK with a zero-row table. - `arrow::Concatenate` on its zero-chunk columns returns an error, which the code discards, so `vec_array` stays empty. - `vec_array[0]->length()` then dereferences garbage. Under lldb the crash is `std::shared_ptr<arrow::Array>::operator->` with `this == nullptr`, inside `read_whole_file`'s lambda, not in Arrow. - Adding a check on `vec_array.empty()` (or the `break` the reporter later added) makes the run complete with exit 0. Two small doc improvements fell out of this, which I'm happy to open a MINOR PR for: the `set_use_threads` comment in `properties.h` says "IO thread pool" but the work goes to the CPU pool, and `GetRecordBatchReader` could note that column decoding uses the CPU pool so callers shouldn't block CPU-pool threads on it. @thisisnic I think this can be closed as not an Arrow bug, with the doc fixes as a follow-up. -- 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]
