-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix HllSketchHolderObjectStrategy#isSafeToConvertToNullSketch. #15860
Conversation
The prior code from apache#15162 was reading only the low-order byte of an int representing the size of a coupon set. As a result, it would erroneously believe that a coupon set with a multiple of 256 elements was empty.
16d8b05
to
b2f6d48
Compare
return false; | ||
} | ||
// Based on org.apache.datasketches.hll.PreambleUtil.extractHashSetCount | ||
int setCount = buf.get(position + 8); // get(HASH_SET_COUNT_INT) | ||
// Endianness of buf doesn't matter, since we're checking for equality with zero. | ||
int setCount = buf.getInt(position + 8); // getInt(HASH_SET_COUNT_INT) |
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 right to me 👍
The changes generally look good to me. We should probably go and ask the apache-datasketches folks if we can get an "isEffectivelyEmpty" method added to the Sketch so that we can remove all of this introspective logic to begin with. That should enable a much more future-proof way of dealing with this. |
…e#15860) * Fix HllSketchHolderObjectStrategy#isSafeToConvertToNullSketch. The prior code from apache#15162 was reading only the low-order byte of an int representing the size of a coupon set. As a result, it would erroneously believe that a coupon set with a multiple of 256 elements was empty.
…e#15860) * Fix HllSketchHolderObjectStrategy#isSafeToConvertToNullSketch. The prior code from apache#15162 was reading only the low-order byte of an int representing the size of a coupon set. As a result, it would erroneously believe that a coupon set with a multiple of 256 elements was empty.
… (#15861) * Fix HllSketchHolderObjectStrategy#isSafeToConvertToNullSketch. The prior code from #15162 was reading only the low-order byte of an int representing the size of a coupon set. As a result, it would erroneously believe that a coupon set with a multiple of 256 elements was empty. Co-authored-by: Gian Merlino <gianmerlino@gmail.com>
The prior code from #15162 was reading only the low-order byte of an int representing the size of a coupon set. As a result, it would erroneously believe that a coupon set with a multiple of 256 elements was empty. At the default lgK (12) this is possible when the sketch has exactly 256 items in it. (At lgK = 12, before it reaches 512, it would be promoted from coupon set to full HLL.)