You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "qingfureal (via GitHub)" <gi...@apache.org> on 2026/09/14 23:39:54 UTC
[PR] [SPARK-59509][CORE] Fix record undercount in UnsafeSorterSpillMerger and document the getNumRecords() contract [spark]
qingfureal opened a new pull request, #58801:
URL: https://github.com/apache/spark/pull/58801
### What changes were proposed in this pull request?
`UnsafeSorterIterator.getNumRecords()` has two contracts, and the abstract method documents neither. Four of the five implementations report the *total* number of records and never change it as the iterator is consumed:
| Implementation | `getNumRecords()` returns | Decrements on `loadNext()`? |
|---|---|---|
| `UnsafeSorterSpillReader` | total from the spill file header | no |
| `UnsafeInMemorySorter.SortedIterator` | total | no |
| `UnsafeSorterSpillMerger` (anonymous) | sum of the counts added | no |
| `UnsafeExternalSorter.ChainedIterator` | sum taken at construction | no |
| `UnsafeExternalSorter.SpillableIterator` | records **remaining** | **yes** |
`UnsafeSorterSpillMerger.addSpillIfNotEmpty` reads the count *after* calling `loadNext()`. This PR moves the read to before it:
```java
public void addSpillIfNotEmpty(UnsafeSorterIterator spillReader) throws IOException {
if (spillReader.hasNext()) {
+ numRecords += spillReader.getNumRecords();
spillReader.loadNext();
priorityQueue.add(spillReader);
- numRecords += spillReader.getNumRecords();
}
}
```
and documents the contract on `UnsafeSorterIterator.getNumRecords()`.
### Why are the changes needed?
`UnsafeExternalSorter.getSortedIterator()` adds its in-memory `SpillableIterator` to the merger alongside the spill readers. Because the count was read after `loadNext()` had already decremented it, the merged iterator reported one record fewer than it will actually produce, per in-memory iterator. The two readings agree only while an iterator is untouched, so reading the count first is correct under either contract.
To be clear about the impact: **there is none today.** No caller in the repository reads a merged iterator's `getNumRecords()` — the value is computed and never consumed. Iteration itself has always been correct, because the merged iterator's `hasNext()` is driven by the priority queue rather than the counter. The other call sites that do read `getNumRecords()` all read fresh, unconsumed iterators, so none of them is affected. That is also why this has gone unnoticed since the line was introduced in SPARK-12295 (2016).
It is still a wrong value, and a trap for the next caller who hands a merged iterator to `UnsafeSorterSpillWriter`, which enforces the declared count and fails with `IllegalStateException: Number of records written exceeded numRecordsToWrite`. The undocumented split contract is what produced the bug, so this PR documents it rather than only fixing the one symptom.
### Does this PR introduce _any_ user-facing change?
No. `getNumRecords()` on the merged iterator now returns the correct value; no public API, behaviour or output changes.
### How was this patch tested?
Two tests added to `UnsafeExternalSorterSuite`:
- `testGetNumRecordsCountsUnspilledRecords` — two spills plus five records still in memory; asserts `getSortedIterator().getNumRecords()` is 15 and that the iterator yields exactly that many records.
- `testGetNumRecordsWithoutUnspilledRecords` — the all-spilled case, which was already correct, pinned against regression.
Verified the first test actually catches the bug. With the one-line change reverted:
```
[ERROR] UnsafeExternalSorterSuite.testGetNumRecordsCountsUnspilledRecords
org.opentest4j.AssertionFailedError: expected: <15> but was: <14>
```
`testGetNumRecordsWithoutUnspilledRecords` still passes with the fix reverted, confirming the miscount is specific to the in-memory iterator path.
With the fix, `UnsafeExternalSorterSuite` (28), `UnsafeExternalSorterRadixSortSuite` (28) and `UnsafeInMemorySorterSuite` (3) all pass, and `dev/lint-java` is clean.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59509][CORE] Make UnsafeSorterIterator.getNumRecords() a consistent total [spark]
Posted by "tdcmeehan (via GitHub)" <gi...@apache.org>.
tdcmeehan commented on code in PR #58801:
URL: https://github.com/apache/spark/pull/58801#discussion_r4028606014
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterIterator.java:
##########
@@ -33,6 +33,18 @@ public abstract class UnsafeSorterIterator {
public abstract long getKeyPrefix();
+ /**
+ * The total number of records this iterator was created over.
+ *
+ * <p>This is a fixed total rather than a remaining count: it does not change as
+ * {@link #loadNext()} is called, and it does not account for records the iterator was advanced
+ * past before being handed on. It therefore tells you how many records the iterator will still
+ * produce only while the iterator is untouched.
+ *
+ * <p>Iterators that aggregate others sum their inputs' totals and inherit the same caveat: the
+ * sum describes what the aggregate produces only if no input was advanced past records the
+ * aggregate will not go on to emit.
+ */
Review Comment:
nit: I believe the first sentence is the invariant and the truly important information for any user of this class, and we could remove the rest of the comment for brevity.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59509][CORE] Make UnsafeSorterIterator.getNumRecords() a consistent total [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #58801: [SPARK-59509][CORE] Make UnsafeSorterIterator.getNumRecords() a consistent total
URL: https://github.com/apache/spark/pull/58801
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59509][CORE] Make UnsafeSorterIterator.getNumRecords() a consistent total [spark]
Posted by "qingfureal (via GitHub)" <gi...@apache.org>.
qingfureal commented on code in PR #58801:
URL: https://github.com/apache/spark/pull/58801#discussion_r4030002682
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterIterator.java:
##########
@@ -33,6 +33,18 @@ public abstract class UnsafeSorterIterator {
public abstract long getKeyPrefix();
+ /**
+ * The total number of records this iterator was created over.
+ *
+ * <p>This is a fixed total rather than a remaining count: it does not change as
+ * {@link #loadNext()} is called, and it does not account for records the iterator was advanced
+ * past before being handed on. It therefore tells you how many records the iterator will still
+ * produce only while the iterator is untouched.
+ *
+ * <p>Iterators that aggregate others sum their inputs' totals and inherit the same caveat: the
+ * sum describes what the aggregate produces only if no input was advanced past records the
+ * aggregate will not go on to emit.
+ */
Review Comment:
Good call, trimmed. I kept one clause beyond the first sentence:
> it does not change as `loadNext()` is called, and it does not account for records the iterator was advanced past and will not emit.
The "advanced past" half is the part that actually catches people: `ChainedIterator` sums its children's totals, but `getIterator(startIndex)` advances those children via `moveOver()` first, so the sum includes records the aggregate never emits. Without that clause the doc reads as if the total always predicts what you'll get.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59509][CORE] Make UnsafeSorterIterator.getNumRecords() a consistent total [spark]
Posted by "qingfureal (via GitHub)" <gi...@apache.org>.
qingfureal commented on code in PR #58801:
URL: https://github.com/apache/spark/pull/58801#discussion_r4030002682
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterIterator.java:
##########
@@ -33,6 +33,18 @@ public abstract class UnsafeSorterIterator {
public abstract long getKeyPrefix();
+ /**
+ * The total number of records this iterator was created over.
+ *
+ * <p>This is a fixed total rather than a remaining count: it does not change as
+ * {@link #loadNext()} is called, and it does not account for records the iterator was advanced
+ * past before being handed on. It therefore tells you how many records the iterator will still
+ * produce only while the iterator is untouched.
+ *
+ * <p>Iterators that aggregate others sum their inputs' totals and inherit the same caveat: the
+ * sum describes what the aggregate produces only if no input was advanced past records the
+ * aggregate will not go on to emit.
+ */
Review Comment:
Good call, trimmed. I kept one clause beyond the first sentence:
> it does not change as `loadNext()` is called, and it does not account for records the iterator was advanced past and will not emit.
The "advanced past" half is the part that actually catches people: `ChainedIterator` sums its children's totals, but `getIterator(startIndex)` advances those children via `moveOver()` first, so the sum includes records the aggregate never emits. Without that clause the doc reads as if the total always predicts what you'll get.
Happy to cut it down to the first sentence alone if you still prefer — just say the word.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59509][CORE] Make UnsafeSorterIterator.getNumRecords() a consistent total [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #58801:
URL: https://github.com/apache/spark/pull/58801#issuecomment-5750060950
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/e151681a240a419f3921ecb337c6090572f0fbe9
- merged into branch-4.x https://github.com/apache/spark/commit/136e34b50f78804e1ca665c06c695411b7d0f922
- merged into branch-4.3 https://github.com/apache/spark/commit/39ade1981f33a395a75f81a9a5e5ac9616ff01b7
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org