sumedhsakdeo commented on PR #3046: URL: https://github.com/apache/iceberg-python/pull/3046#issuecomment-3976565338
Thanks @kevinjqliu! You're right that removing `list()` is the core fix — and `ArrivalOrder` does exactly that. Two reasons I kept `TaskOrder` as the default rather than just swapping `list()` out: 1. **Semantic change**: today's behavior groups batches by file in submission order. Replacing `list()` with interleaved streaming would be a silent breaking change for any code that relies on per-file grouping. `TaskOrder` preserves the existing contract; `ArrivalOrder` opts in. 2. **No backpressure**: `executor.map` submits all N tasks immediately even with lazy iteration — 1000 files still means 1000 concurrent threads. `ArrivalOrder(concurrent_streams=N)` bounds both concurrency and memory via a bounded queue. Time-to-first-record (TTFR) is particularly critical for ML training workloads where the GPU stalls waiting for the first batch — `executor.map` blocks on file 0 completing before yielding anything, even if files 1–15 are already done (head-of-line blocking). On simulated S3 workloads (variable TTFB), `ArrivalOrder` shows **7x lower TTFR** and **~11% higher throughput** vs `TaskOrder` at the same thread count. Locally it's ~15% faster for the same reason. The PR is broken into a stack if you'd prefer to review incrementally — happy to land them in order. -- 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]
