Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Level-0 sorting for KllItemsSketch<T> is not based on Comparator<T> #527

Closed
fanyang01 opened this issue Mar 11, 2024 · 3 comments
Closed

Comments

@fanyang01
Copy link

fanyang01 commented Mar 11, 2024

if ((curLevel == 0) && !isLevelZeroSorted) {
Arrays.sort(inBuf, adjBeg, adjBeg + adjPop);
}

The Arrays.sort API in the above code

sorts the specified range of the specified array of objects into ascending order, according to the natural ordering of its elements. ... All elements in this range must implement the Comparable interface`.

It appears that Arrays.sort(inBuf, adjBeg, adjBeg + adjPop, comp) should be called instead.

@leerho
Copy link
Contributor

leerho commented Mar 11, 2024

Thank you!
This was a bit subtle. It only occurs during merge, when a compaction is required, and when the object T cannot be naturally ordered.
Will fix this and add a test for it.

@jmalkin
Copy link
Contributor

jmalkin commented Mar 20, 2024

This has been merged into the main branch (next release with this still TBD). It was also back-ported into the 5.0.X branch, and I'm working on starting a release vote for a v5.0.2 right now, which will include this fix.

@jmalkin jmalkin closed this as completed Mar 20, 2024
@fanyang01
Copy link
Author

Thank you very much! I have checked the related changes and the test case looks great. I will wait for the next release.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants