Repository navigation
fix: scope HashJoinExec build_time to hash-table construction only - #24271
ulrichurriola-parada-ops wants to merge 1 commit into
Conversation
build_time previously wrapped the whole collect_left_input future, including the build-side child's own stream-draining work, so its compute got double-counted into this join's elapsed_compute. Time only the post-fold hash-map construction instead.
Rich-T-kid
left a comment
Rich-T-kid
left a comment
There was a problem hiding this comment.
Im a bit confused by this PR, where is _build_timer actually used?
The
Note: a similar use exists in the codebase in Thank you for the review |
There was a problem hiding this comment.
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
|
@ulrichurriola-parada-ops thank your for the explanation it helped out alot! |
@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:
Considering those examples, recording a node own time and not its children in the 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. |
Rich-T-kid
left a comment
Rich-T-kid
left a comment
There was a problem hiding this comment.
make sense to me. are there any other operators that incorrectly account for their children's compute time?
I haven't had a chance to audit the other operators for the same issue, so I can't say for sure yet. Given that, would you agree to roll the fix out progressively rather than blocking on a full audit, starting with this PR ? |
@ulrichurriola-parada-ops I think it makes sense to create an issue explainging your findings and then create focused PR's. I'd be happy to review them feel free to ping me |
I just created the issue: #24459 |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Rationale for this change
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. Any tool that sums elapsed_compute across a physical plan to estimate total CPU time (e.g. to build a cost/telemetry model) will double-count that subtree's compute: once under the child operator(s) that did the work, and again under HashJoinExec.
What changes are included in this PR?
build_time was previously timed around the entire collect_left_input future (via left_fut.get_shared(cx) in collect_build_side). That future both drains the build-side child's stream and builds the hash table, so the child's own compute got billed to build_time whenever the driving partition executed both phases inline.
Are these changes tested?
Covered by the existing hash_join test suite (correctness of join output and other metrics is unaffected). No new test was added to assert the exact build_time/elapsed_compute value, since there isn't currently a way to attribute deterministic wall-clock cost to a specific subtree in a unit test.
Are there any user-facing changes?
elapsed_compute reported for HashJoinExec (e.g. via EXPLAIN ANALYZE) will be smaller and more accurate, reflecting only the join's own compute rather than including its build-side subtree. No public API changes.