viirya commented on PR #6071: URL: https://github.com/apache/datafusion-comet/pull/6071#issuecomment-5879258081
@andygrove Thanks for the detailed review. The PR is now draft, and the description says “Part of #1204.” I agree that useful reuse on a realistic workload and resolution of the metrics-scaling issue remain merge requirements. The review fixes are in `b02b2b9f3`; `d42857ef8` subsequently merges upstream and resolves the conflicts. Besides the inline fixes, I added: - `shared_plan_hits`, counting tasks that successfully bind to a tree retrieved from the registry, separately from `shared_plan_tasks`. - Separate session-setup and physical-planning timers, reported once per native block root. - Admission rules and an execution-state checklist in the contributor guide. Local validation after the upstream merge passed 25 shared-pipeline native tests, one metrics-conversion test, and 186 JVM tests across the execution, lifecycle, and task-metrics suites. Formatting and Clippy also passed. I completed the setup profiling at `b02b2b9f3`, before the upstream merge. Each suite ran in a fresh JVM with Spark 4.1.3, `local[4]`, SF1 Parquet, sharing enabled, and normal algorithm defaults. All 22 TPC-H and 103 TPC-DS queries ran twice; the first pass was warmup. | Measured pass | Native block/task invocations | Session setup | Physical planning | Other setup | |---|---:|---:|---:|---:| | TPC-H | 436 | 110.75 ms | 52.85 ms | 11.53 ms | | TPC-DS | 3,419 | 850.03 ms | 466.97 ms | 107.64 ms | These are elapsed timers summed across invocations, not executor CPU time or query wall time. Session setup accounted for **67.7% and 64.5% of session-plus-planning time**, respectively. Both suites still had zero registry hits; all 45 admitted invocations had one partition. This supports your suggestion to investigate session setup before widening admission solely for performance. It does not yet establish that UDF registration itself dominates: the next useful split is session construction versus function registration. Any optimization there must preserve task-local configuration, memory pools, and runtime resources. Upstream metrics API integration, the matched large-stage rerun including retained bytes, and demonstrated useful reuse on a realistic workload remain outstanding. I am keeping the PR draft while those are addressed. -- 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]
