andygrove commented on PR #5331: URL: https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5876436489
This is a light fully automated review since there are so many PRs open. 1. The new paragraph in `iceberg.md` says a merge opens one reader per file, but `docs/source/user-guide/latest/tuning.md:212` still says the native Iceberg scan reads each task's data files one at a time by default, and the `dataFileConcurrencyLimit` doc at `spark/src/main/scala/org/apache/comet/CometConf.scala:178` describes it as the number of files read concurrently within a task. Neither holds on the merge path. Each file is its own partition under the `SortPreservingMergeExec`, so a task has up to `sortMerge.maxFilesPerPartition` (64 by default) readers open at once, whatever `dataFileConcurrencyLimit` is set to. Someone lowering that limit to cap scan memory on a sorted table would be turning the wrong knob. Could both say the limit only bounds the unordered read, and point at `sortMerge.maxFilesPerPartition` for the merge? 2. Two comments still describe earlier revisions. `native/core/src/execution/planner.rs:1852` says `table_sort_orders` is empty unless sortMerge is on, but the ordering is bound in `CometScanRule` and written by the serde regardless of that flag, and `enabled=false` only sets `max_files_per_partition` to 0. That is what keeps the disabled case correct, since an empty list there would mean an unordered read under a `Sort` that Spark has already dropped. The `reportableOrdering` scaladoc at `spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:899` and `:906` still says two callers share the gate and that a transform key falls through to `Nil` and we read unordered. Today `CometScanRule` is the only caller, and `Nil` with a reported ordering keeps the scan on Spark. Could these be brought in line, so nobody later changes the serde to match the planner comment and stops sending the ordering when the merge is off? -- 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]
