kosiew commented on code in PR #24491:
URL: https://github.com/apache/datafusion/pull/24491#discussion_r3885918457
##########
datafusion/physical-plan/src/joins/nested_loop_join.rs:
##########
@@ -2304,13 +2319,20 @@ impl NestedLoopJoinStream {
}
// Poll the replay stream for the next right batch
- match self
+ let result = match self
.right_data
.as_mut()
.expect("right_data must be present")
.poll_next_unpin(cx)
{
- Poll::Ready(Some(Ok(right_batch))) => {
+ Poll::Ready(result) => result,
+ Poll::Pending => return ControlFlow::Break(Poll::Pending),
+ };
+
+ let join_metric = self.metrics.join_metrics.join_time.clone();
Review Comment:
Nice improvement here. One thing I noticed is that the new timing tests
currently use only `JoinType::Inner`, so they do not exercise
`EmitGlobalRightUnmatched`, even though the replay-poll timing in that path
changed as part of this PR. It might be worth adding a memory-limited RIGHT or
FULL join regression test that reaches this state and verifies that replay
polling is still excluded from `join_time`. This is non-blocking, but it would
give us coverage for this distinct outer-join path.
--
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]
