Skip to content

[SPARK-58680][CORE] Use isEmpty/nonEmpty for emptiness checks in ExternalAppendOnlyMap - #57883

Open
uros-b wants to merge 1 commit into
apache:masterfrom
uros-b:perf-externalappendonlymap
Open

[SPARK-58680][CORE] Use isEmpty/nonEmpty for emptiness checks in ExternalAppendOnlyMap#57883
uros-b wants to merge 1 commit into
apache:masterfrom
uros-b:perf-externalappendonlymap

Conversation

@uros-b

@uros-b uros-b commented Aug 9, 2026

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Uses the idiomatic emptiness checks for the three ArrayBuffer length comparisons in ExternalAppendOnlyMap:

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)

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for CI

@dongjoon-hyun dongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants