-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve hashjoin execution metrics #4394
Conversation
let hashjoin_metrics = HashJoinMetrics::new(partition, &self.metrics); | ||
let timer = hashjoin_metrics.build_time.timer(); | ||
let left_fut = match self.mode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to calculate the build time here? The left_fut
returned here is a Future
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to calculate the build time here? The
left_fut
returned here is aFuture
.
Thanks!I'll check this code later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
@@ -1551,11 +1557,13 @@ impl HashJoinStream { | |||
| JoinType::RightAnti => {} | |||
} | |||
} | |||
Some(result.map(|x| x.0)) | |||
let final_result = Some(result.map(|x| x.0)); | |||
timer.done(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think drop will handle this for you (so the explicit done()
is not needed)
@@ -1487,10 +1492,12 @@ impl HashJoinStream { | |||
&mut self, | |||
cx: &mut std::task::Context<'_>, | |||
) -> Poll<Option<ArrowResult<RecordBatch>>> { | |||
let build_timer = self.join_metrics.build_time.timer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be timing the overall clock time (not the cpu time) of the build. As long as that is what you are trying to time 👍
LGTM |
Benchmark runs are scheduled for baseline = fa4bea8 and contender = 66c95e7. 66c95e7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Thanks @AssHero ! |
Which issue does this PR close?
Closes #4009
Rationale for this change
improve hashjoin execution metrics, include hashmap build time.