parthchandra commented on PR #5331: URL: https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5941804515
> 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? Both fixed. (1) The dataFileConcurrencyLimit docs now say it only bounds the unordered read, and point to sortMerge.maxFilesPerPartition for the merge path, since on the merge each file is its own reader regardless of that limit — CometConf.scala:179 and tuning.md:350. (2) Brought the stale comments in line: planner.rs now says the sort order is written regardless of the sortMerge.enabled flag (disabling just sets the cap to 0), and the reportableOrdering comment now says there's one caller and that an empty result with a reported ordering keeps the scan on Spark. -- 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]
