Refactor loser tree code in SortPreservingMerge per PR comments #4407
Conversation
| /// The tree update could not be completed (e.g. the input was not | ||
| /// ready or had an error). The caller should return the `Poll` | ||
| /// result to its caller | ||
| Incomplete(Poll<Option<ArrowResult<RecordBatch>>>), |
There was a problem hiding this comment.
| Incomplete(Poll<Option<ArrowResult<RecordBatch>>>), | |
| Pending, | |
| Error(ArrowError), |
Given we never seem to return TreeUpdate::Incomplete(Poll::Ready(None)) or TreeUpdate::Incomplete(Poll::Ready(Some(Ok(_))))
|
Here are performance results. It is somewhat of a mixed bag (some show a few percent less) results are The first run is the second time I ran |
|
|
||
| /// The result of updating the loser tree. It is the same as an Option | ||
| /// but with specific names for easier readability | ||
| enum TreeUpdate { |
There was a problem hiding this comment.
I don't really see the need for this over Poll<Result<()>> in particular it is confusing what its semantics are w.r.t wakers
if let Err(e) = self.build()? {
return Poll::Ready(Err(e))
}
Is not significantly more verbose
There was a problem hiding this comment.
I tried it out and you are right -- I think Poll made the code better -- in 2237abe
| self.aborted = true; | ||
| return TreeUpdate::Error(e); | ||
| } | ||
| Poll::Pending => return TreeUpdate::Pending, |
There was a problem hiding this comment.
If this method returned Poll this could just use ready! or even ?
|
Benchmark runs are scheduled for baseline = 0d27fcb and contender = 82bbaa3. 82bbaa3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
re #4300.
Rationale for this change
What changes are included in this PR?
Implements suggestions from @tustvold and @viirya in #4301
Are these changes tested?
covered by existing tests
TODO: need to run benchmarks
Are there any user-facing changes?
No