[SPARK-39680][CORE] Remove hasSpaceForAnotherRecord from UnsafeExternalSorter #37086
[SPARK-39680][CORE] Remove hasSpaceForAnotherRecord from UnsafeExternalSorter #37086LuciferYang wants to merge 1 commit intoapache:masterfrom
hasSpaceForAnotherRecord from UnsafeExternalSorter #37086Conversation
| } | ||
| } | ||
|
|
||
| @VisibleForTesting boolean hasSpaceForAnotherRecord() { |
There was a problem hiding this comment.
Thank you for making a PR, but I'd not remove this. Although this is not used currently, this was added once inevitably. I feel this removal might become a too hasty decision because UnsafeExternalSorter is designed to be based on UnsafeInMemorySorter and UnsafeInMemorySorter.hasSpaceForAnotherRecord() is a useful public method. It still looks okay to me to have this VisibleForTesting method to expose the underlying structure.
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
May I ask if you are currently some other refactoring on this area? I'm wondering if you want to encapsulate back for some reasons or not.
Not yet, just see this method is no longer used. I will close this pr |
|
thanks for your review @dongjoon-hyun |
|
Thank you for closing and sorry for your loss of this PR. If we want to remove this, we can reuse this JIRA and PR. |
OK ~ |
2 participants
What changes were proposed in this pull request?
This pr just remove an unused
@VisibleForTestingmethod fromUnsafeExternalSorter.Why are the changes needed?
Code clean.
SPARK-21907 introduce
hasSpaceForAnotherRecord()method forUnsafeExternalSorter, it is identified as@VisibleForTestingand only used byUnsafeExternalSorterSuite#testOOMDuringSpill.After SPARK-32901,
testOOMDuringSpillinUnsafeExternalSorterSuiterename totestNoOOMDuringSpillandhasSpaceForAnotherRecord()method is no longer used in current master now.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass GitHub Actions.