Skip to content
This repository has been archived by the owner on Jul 7, 2020. It is now read-only.

Change type of TDigest.Group.id from int to long #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sgrj
Copy link

@sgrj sgrj commented Oct 29, 2019

The assumption is that the id is unique. If the id value overflows, it
can lead to NullPointerExceptions. Changing the type to long doesn't
make overflows impossible, but highly unlikely.

This fixes issues like #100

The assumption is that the id is unique. If the id value overflows, it
can lead to NullPointerExceptions. Changing the type to long doesn't
make overflows impossible, but highly unlikely.
Copy link
Contributor

@tdunning tdunning left a comment

Choose a reason for hiding this comment

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

This fix is fixing an ancient version of the tdigest. Why not move to the more modern MergingDigest? Accuracy is substantially improved, speed is better and dynamic allocation is avoided.

@@ -541,14 +541,14 @@ public String toString() {

@Override
public int hashCode() {
return id;
return Long.hashCode(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the point of hashing the id?

@sgrj
Copy link
Author

sgrj commented Oct 30, 2019

Thanks, switching to MergingDigest indeed shows some nice performance improvements.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants