Skip to content

Conversation

@xiaoyesoso
Copy link
Contributor

What changes were proposed in this pull request?

Notes

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Please review http://spark.apache.org/contributing.html before opening a pull request.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@srowen
Copy link
Member

srowen commented Feb 26, 2017

This has no explanation. This is also copied from the Android OSS project, so I don't think I'd change the comments without reason. It'd be easier to update later if needed

@xiaoyesoso
Copy link
Contributor Author

@srowen
The explanations are for TimSort from Android OSS。The code has bug (see it )

private void mergeCollapse() {
  while (stackSize > 1) {
    int n = stackSize - 2;
    if (n > 0 && runLen[n-1] <= runLen[n] + runLen[n+1]) {
      if (runLen[n - 1] < runLen[n + 1])
         n--;
      mergeAt(n);
    } else if (runLen[n] <= runLen[n + 1]) {
      mergeAt(n);
    } else {
      break; // Invariant is established
    }
  }
}

But we have fixed it. (see SPARK-5984) .So leaving these explanations may puzzling.

@srowen
Copy link
Member

srowen commented Feb 26, 2017

OK, but you're un-documenting an invariant that is still supposed to be true. Your new invariant is the same as the first too.

srowen added a commit to srowen/spark that referenced this pull request Mar 22, 2017
@srowen srowen mentioned this pull request Mar 22, 2017
@asfgit asfgit closed this in b70c03a Mar 23, 2017
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.

3 participants