You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "rluvaton (via GitHub)" <gi...@apache.org> on 2026/09/09 10:52:46 UTC
[PR] fix: report correct number of items left in the iterator for `IncrementalSortIterator` [datafusion]
rluvaton opened a new pull request, #25107:
URL: https://github.com/apache/datafusion/pull/25107
## Which issue does this PR close?
N/A
## Rationale for this change
`size_hint` for `IncrementalSortIterator` returns the initial number of items rather than how many items left in the iterator
## What changes are included in this PR?
update size hint to consider the number of elements already produced, impl `ExactSizeIterator` and added test
## What is the testing strategy for this PR?
unit test
## Are there any user-facing changes?
no
--
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: report correct number of items left in the iterator for `IncrementalSortIterator` [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton merged PR #25107:
URL: https://github.com/apache/datafusion/pull/25107
--
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: report correct number of items left in the iterator for `IncrementalSortIterator` [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #25107:
URL: https://github.com/apache/datafusion/pull/25107#discussion_r3977021963
##########
datafusion/physical-plan/src/sorts/stream.rs:
##########
@@ -377,13 +377,15 @@ impl Iterator for IncrementalSortIterator {
}
fn size_hint(&self) -> (usize, Option<usize>) {
- let num_rows = self.batch.num_rows();
+ let num_rows = self.batch.num_rows().saturating_sub(self.cursor);
let batch_size = self.batch_size;
let num_batches = num_rows.div_ceil(batch_size);
(num_batches, Some(num_batches))
}
}
+impl ExactSizeIterator for IncrementalSortIterator {}
Review Comment:
Could we omit `ExactSizeIterator` here? `next()` returns `Some(Err(_))` without advancing state when expression evaluation or sorting fails, so it can yield more items than `len()` promises. The remaining-row fix is valid on the success path, but does not establish an exact count on errors.
--
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: report correct number of items left in the iterator for `IncrementalSortIterator` [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25107:
URL: https://github.com/apache/datafusion/pull/25107#issuecomment-5606600451
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25107?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 `86.66667%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.80%. Comparing base ([`a5c809f`](https://app.codecov.io/gh/apache/datafusion/commit/a5c809f98dedcf4b22f5d317ea8efe0720834925?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5e96426`](https://app.codecov.io/gh/apache/datafusion/commit/5e9642695e6d455767c90049c031136cd6ac3f72?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 15 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25107?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/sorts/stream.rs](https://app.codecov.io/gh/apache/datafusion/pull/25107?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fsorts%2Fstream.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9zb3J0cy9zdHJlYW0ucnM=) | 86.66% | [0 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25107?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 #25107 +/- ##
==========================================
+ Coverage 81.74% 81.80% +0.05%
==========================================
Files 1128 1130 +2
Lines 416644 417793 +1149
Branches 416644 417793 +1149
==========================================
+ Hits 340592 341777 +1185
+ Misses 55995 55882 -113
- Partials 20057 20134 +77
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25107?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: report correct number of items left in the iterator for `IncrementalSortIterator` [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #25107:
URL: https://github.com/apache/datafusion/pull/25107#discussion_r3978146769
##########
datafusion/physical-plan/src/sorts/stream.rs:
##########
@@ -377,13 +377,15 @@ impl Iterator for IncrementalSortIterator {
}
fn size_hint(&self) -> (usize, Option<usize>) {
- let num_rows = self.batch.num_rows();
+ let num_rows = self.batch.num_rows().saturating_sub(self.cursor);
let batch_size = self.batch_size;
let num_batches = num_rows.div_ceil(batch_size);
(num_batches, Some(num_batches))
}
}
+impl ExactSizeIterator for IncrementalSortIterator {}
Review Comment:
great catch, thanks, fixed
--
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