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 |
2 participants
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.