andygrove commented on PR #5331: URL: https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5299313313
One thing I'd like to dig into more before this lands: the reader fan-out in the ordered path. `SortPreservingMergeExec` calls `execute()` on all of its input partitions up front, and `IcebergScanExec::execute` eagerly builds the `ArrowReader` and kicks off the read, so we end up with a live Parquet reader per data file in the Spark partition plus a buffered batch per stream inside the merge. `data_file_concurrency_limit` doesn't bound any of that anymore. The comment in `iceberg_scan.rs` describes the fan-out as intrinsic to a k-way merge. That's true of the number of *streams*, but not of how many readers we have decoding concurrently or how much we buffer — a cascaded merge, or just falling back to the unordered read when a partition has too many files, would bound both. The tables in the tests are fine, but the workload this targets is a sorted table, and a sorted table that has accumulated a lot of small commits is exactly where you get hundreds of files in one task. Since the merge reserves against Comet's memory pool, hitting the limit there is a query failure rather than a slowdown, which is a worse failure mode than the extra sort we're trying to avoid. Could we add a config for the maximum files per partition we're willing to merge, and fall back to the unordered read above it? That keeps the default safe and lets people opt into deeper merges once we have a better sense of the memory profile. -- 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]
