This is an automated email from the ASF dual-hosted git repository.
uros-b pushed a commit to branch branch-4.x
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-4.x by this push:
new 79a453c6fb5d [SPARK-58680][CORE] Use isEmpty/nonEmpty for emptiness
checks in ExternalAppendOnlyMap
79a453c6fb5d is described below
commit 79a453c6fb5d8d2276b91b03b7076fd0a4dddfb9
Author: Uros Bojanic <[email protected]>
AuthorDate: Mon Aug 10 04:26:47 2026 +0000
[SPARK-58680][CORE] Use isEmpty/nonEmpty for emptiness checks in
ExternalAppendOnlyMap
### What changes were proposed in this pull request?
Uses the idiomatic emptiness checks for the three `ArrayBuffer` length
comparisons in `ExternalAppendOnlyMap`:
```scala
if (kcPairs.nonEmpty) { // was kcPairs.length > 0
def isEmpty: Boolean = pairs.isEmpty // was pairs.length == 0
assert(pairs.nonEmpty) // was assert(pairs.length > 0)
```
### Why are the changes needed?
The same file already uses this form for its other emptiness checks
(`spilledMaps.isEmpty`, `mergeHeap.nonEmpty`, `mergeHeap.isEmpty`), as does the
sibling `ExternalSorter`, so these three were the odd ones out.
To be clear about scope: this is a **readability change, not a performance
optimization**. For `ArrayBuffer` both forms are O(1) and exactly equivalent --
`isEmpty` resolves to `SeqOps.isEmpty`, which is `lengthCompare(0) == 0` over
the buffer's cached size -- so there is no measurable difference either way.
The `currentMap.size > 0` check in `forceSpill` is deliberately **not**
changed. `AppendOnlyMap` is an `Iterable` rather than a `Seq` and does not
override `knownSize`, so `isEmpty` there would fall through to `iterator()`,
which asserts the map has not been destructively sorted. Since `forceSpill` can
run under memory pressure after `destructiveSortedIterator` has been called,
that site must stay a size comparison.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Behavior-preserving, so the existing `ExternalAppendOnlyMapSuite` coverage
applies, in particular the spill and hash-collision tests that drive
`StreamBuffer` with more than one pair per hash. The three predicates have
identical truth values for every possible buffer state, so no branch can be
taken differently.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)
Closes #57883 from uros-b/perf-externalappendonlymap.
Authored-by: Uros Bojanic <[email protected]>
Signed-off-by: Uros Bojanic <[email protected]>
(cherry picked from commit 3922ec77020f3885823ebf335541f94a2070a068)
Signed-off-by: Uros Bojanic <[email protected]>
---
.../org/apache/spark/util/collection/ExternalAppendOnlyMap.scala | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git
a/core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala
b/core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala
index d892fa0e4706..06dcd136faac 100644
---
a/core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala
+++
b/core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala
@@ -309,7 +309,7 @@ class ExternalAppendOnlyMap[K, V, C](
inputStreams.foreach { it =>
val kcPairs = new ArrayBuffer[(K, C)]
readNextHashCode(it, kcPairs)
- if (kcPairs.length > 0) {
+ if (kcPairs.nonEmpty) {
mergeHeap.enqueue(new StreamBuffer(it, kcPairs))
}
}
@@ -426,11 +426,11 @@ class ExternalAppendOnlyMap[K, V, C](
val pairs: ArrayBuffer[(K, C)])
extends Comparable[StreamBuffer] {
- def isEmpty: Boolean = pairs.length == 0
+ def isEmpty: Boolean = pairs.isEmpty
// Invalid if there are no more pairs in this stream
def minKeyHash: Int = {
- assert(pairs.length > 0)
+ assert(pairs.nonEmpty)
hashKey(pairs.head)
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
