You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ulrichurriola-parada-ops (via GitHub)" <gi...@apache.org> on 2026/08/06 13:14:27 UTC
[PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
ulrichurriola-parada-ops opened a new pull request, #24142:
URL: https://github.com/apache/datafusion/pull/24142
BuildProbeJoinMetrics derives Clone and previously ran its elapsed_compute update from a Drop impl on itself. HashJoinExec::execute() clones this struct into the build-side future (collect_left_input) while the original is kept by HashJoinStream, so there are always two live instances for a CollectLeft/Partitioned join, and Drop fired once per instance.
Since build_time/join_time are Arc<AtomicUsize>-backed and shared between clones, the clone dropped at build-completion time re-added whatever build_time had already accumulated (via ScopedTimerGuard flushing on every Pending poll) into elapsed_compute. The original then added the same build_time again at the end, plus join_time - inflating elapsed_compute by up to build_time on any hash join whose build side yields control before completing.
Move the elapsed_compute update into a new ElapsedComputeFinalizer, held behind an Arc on BuildProbeJoinMetrics. Cloning the outer struct now only bumps the Arc's refcount instead of creating a second independent Drop, so the update runs exactly once, when the last clone is dropped, using the fully-accumulated final values.
Added a regression test that reproduces the clone/drop sequence directly and asserts elapsed_compute == build_time + join_time exactly.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24142:
URL: https://github.com/apache/datafusion/pull/24142#discussion_r3794319933
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2725,6 +2752,48 @@ mod tests {
check_join_set_is_valid(&left, &right, on)
}
+ #[test]
+ fn build_probe_join_metrics_elapsed_compute_not_double_counted_on_clone() {
Review Comment:
Nice regression test. One additional test that might be worth considering is an execution-level `HashJoinExec` test where the build stream yields before completing. This unit test covers the shared-finalizer ownership behavior well, while an execution-level test would also exercise the `HashJoinExec::execute` clone and future wiring that originally exposed the issue. Not blocking for this PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
Posted by "ulrichurriola-parada-ops (via GitHub)" <gi...@apache.org>.
ulrichurriola-parada-ops commented on code in PR #24142:
URL: https://github.com/apache/datafusion/pull/24142#discussion_r3795161452
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2725,6 +2752,48 @@ mod tests {
check_join_set_is_valid(&left, &right, on)
}
+ #[test]
+ fn build_probe_join_metrics_elapsed_compute_not_double_counted_on_clone() {
Review Comment:
Thank you for the review,I'll make a follow-up for the additional test.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #24142:
URL: https://github.com/apache/datafusion/pull/24142
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24142:
URL: https://github.com/apache/datafusion/pull/24142#issuecomment-5494997889
Thank you @ulrichurriola-parada-ops
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24142:
URL: https://github.com/apache/datafusion/pull/24142#issuecomment-5207937925
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24142?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `91.42857%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.05%. Comparing base ([`70c26a0`](https://app.codecov.io/gh/apache/datafusion/commit/70c26a06716eb552f0fc1d2115957a4089504f11?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5b7cb26`](https://app.codecov.io/gh/apache/datafusion/commit/5b7cb26ddff1bcc58b71f1b7aab7c44988935642?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24142?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-plan/src/joins/utils.rs](https://app.codecov.io/gh/apache/datafusion/pull/24142?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fjoins%2Futils.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9qb2lucy91dGlscy5ycw==) | 91.42% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24142 +/- ##
========================================
Coverage 81.04% 81.05%
========================================
Files 1105 1106 +1
Lines 380163 380591 +428
Branches 380163 380591 +428
========================================
+ Hits 308111 308495 +384
- Misses 53834 53874 +40
- Partials 18218 18222 +4
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24142?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24142:
URL: https://github.com/apache/datafusion/pull/24142#issuecomment-5441514101
THanks @ulrichurriola-parada-ops and @kosiew
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
Posted by "ulrichurriola-parada-ops (via GitHub)" <gi...@apache.org>.
ulrichurriola-parada-ops commented on code in PR #24142:
URL: https://github.com/apache/datafusion/pull/24142#discussion_r3795161452
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2725,6 +2752,48 @@ mod tests {
check_join_set_is_valid(&left, &right, on)
}
+ #[test]
+ fn build_probe_join_metrics_elapsed_compute_not_double_counted_on_clone() {
Review Comment:
Thank you for the review, I'll merge this PR and make a follow-up for the additional test.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24142:
URL: https://github.com/apache/datafusion/pull/24142#issuecomment-5327015382
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-physical-plan v54.1.0 (current)
Built [ 42.989s] (current)
Parsing datafusion-physical-plan v54.1.0 (current)
Parsed [ 0.144s] (current)
Building datafusion-physical-plan v54.1.0 (baseline)
Built [ 37.965s] (baseline)
Parsing datafusion-physical-plan v54.1.0 (baseline)
Parsed [ 0.145s] (baseline)
Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.667s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure enum_variant_missing: pub enum variant removed or renamed ---
Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/enum_variant_missing.ron
Failed in:
variant WindowFnKind::DenseRank, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/8fed7309b03fa674ae00e0a26ea2ff1075a2dedd/datafusion/physical-plan/src/sorts/partitioned_topk.rs:78
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 83.191s] datafusion-physical-plan
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: avoid double-counting build_time in HashJoinExec elapsed_compute [datafusion]
Posted by "ulrichurriola-parada-ops (via GitHub)" <gi...@apache.org>.
ulrichurriola-parada-ops commented on PR #24142:
URL: https://github.com/apache/datafusion/pull/24142#issuecomment-5490927409
> THanks @ulrichurriola-parada-ops and @kosiew
Dear @alamb, thank you for the final approval/auto-merge scheduling. Unfortunately it looks like the CI has been pending for 4 days now. Is there anything I/you can do?
Thank you
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org