-
Notifications
You must be signed in to change notification settings - Fork 28k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-26955][CORE] Align Spark's TimSort to jdk11 implementation #23858
Conversation
*/ | ||
private void mergeCollapse() { | ||
while (stackSize > 1) { | ||
int n = stackSize - 2; | ||
if ( (n >= 1 && runLen[n-1] <= runLen[n] + runLen[n+1]) | ||
|| (n >= 2 && runLen[n-2] <= runLen[n] + runLen[n-1])) { | ||
if (n > 0 && runLen[n-1] <= runLen[n] + runLen[n+1] || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about removing the parentheses? that changes the semantics, I believe, to (((a and b) or c) and d) . What's the implementation you're copying here, just to be sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. TIL that &&
has higher precedence that ||
so this doesn't change things. Actually the whole change on this line doesn't cause any actual change, which is good; aligning the implementation exactly is useful though.
Test build #102582 has finished for PR 23858 at commit
|
jenkins, retest this, please |
Test build #102603 has finished for PR 23858 at commit
|
Merged to master |
What changes were proposed in this pull request?
Spark's TimSort deviates from JDK 11 TimSort in a couple places:
stackLen
was increased in jdkmergeCollapse
:n < 0
In the PR, I propose to align Spark TimSort to jdk implementation.
How was this patch tested?
By existing test suites, in particular,
SorterSuite
.