zhuqi-lucas commented on code in PR #23702:
URL: https://github.com/apache/datafusion/pull/23702#discussion_r3645423082
##########
datafusion/physical-plan/src/sorts/merge.rs:
##########
@@ -369,18 +414,21 @@ impl<C: CursorValues> SortPreservingMergeStream<C> {
/// Advances the actual cursor. If it reaches its end, update the
/// previous cursor with it.
///
- /// If the given partition is not exhausted, the function returns `true`.
+ /// If the given partition batch is exhausted, return `true` to signal a
poll is needed
fn advance_cursors(&mut self, stream_idx: usize) -> bool {
if let Some(cursor) = &mut self.cursors[stream_idx] {
let _ = cursor.advance();
- if cursor.is_finished() {
+ return if cursor.is_finished() {
// Take the current cursor, leaving `None` in its place
self.prev_cursors[stream_idx] =
self.cursors[stream_idx].take();
- }
- true
- } else {
- false
+
+ true
+ } else {
+ false
+ };
}
+
+ true
}
Review Comment:
`return if … { true } else { false }` can collapse to the condition; also
worth noting the trailing `true` is unreachable from the (only) call site now
that the loop is guarded by `is_exhausted`.
(Verified locally: applied on this branch, compiles, all 74 `sorts::` tests
pass.)
```suggestion
fn advance_cursors(&mut self, stream_idx: usize) -> bool {
if let Some(cursor) = &mut self.cursors[stream_idx] {
let _ = cursor.advance();
let finished = cursor.is_finished();
if finished {
// Take the current cursor, leaving `None` in its place
self.prev_cursors[stream_idx] =
self.cursors[stream_idx].take();
}
return finished;
}
// Unreachable from the merge loop (guarded by `is_exhausted`),
// kept for defensive completeness: no cursor ⇒ a poll is needed.
true
}
```
--
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]
