ulrichurriola-parada-ops commented on PR #24271: URL: https://github.com/apache/datafusion/pull/24271#issuecomment-5315301291
> Ok this makes sense to me, but I think we should first double check that > > > HashJoinExec's elapsed_compute metric (as reported by EXPLAIN ANALYZE and ExecutionPlanMetricsSet) includes time spent executing the entire build-side subtree, not just the join's own work. > > isn't intentional, I can see why we may want to include the build-side elapse compute as part of the hashJoin `elapsed_compute`. > > I can see why it can cause errors if you were to sum all compute time but it may make more sense to view it as tree's where each nodes `elapsed_compute` is the sum of its children and its self @Rich-T-kid thank you for your response. Other types of nodes do not include the subtree times and only record time when their sub nodes are ready: * `FilterExec` in `datafusion/physical-plan/src/filter.rs:1265`, inside the `ready!(self.input.poll_next_unpin(cx))`, * `ProjectionExec` in `datafusion/physical-plan/src/projection.rs:647` that calls `batch_project` (which handles the timer), * `LimitExec (stream_limit) ` in datafusion/physical-plan/src/limit.rs:652 calls stream_limit method with the timer inside it, * `WindowAggExec` in `datafusion/physical-plan/src/windows/window_agg_exec.rs:598` with the timer inside the `compute_aggregates` method * `AggregateExec`, in `datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs345` (`AggregateStream`) and `datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs647` (`GroupedHashAggregateStream`) `UnionExec` in `datafusion/physical-plan/src/union.rs `does not record `poll_next` timing at all and only takes into account the time spent in the `execute` method. Considering those examples, recording a node own time and not its children in the `elapsed_time` (and thus in the `build_time`) feels like a common design to me. Finally, including children's time also makes duration analysis harder — you'd need to parse the execution plan to tell which time belongs to a node itself versus what's already rolled up from its children. -- 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]
