-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 Bug - Handle unsigned long in sorting order assertion of LongHashSet #17207
base: main
Are you sure you want to change the base?
Fix Bug - Handle unsigned long in sorting order assertion of LongHashSet #17207
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17207 +/- ##
============================================
- Coverage 72.42% 72.31% -0.11%
+ Complexity 65775 65754 -21
============================================
Files 5318 5318
Lines 305739 305745 +6
Branches 44349 44351 +2
============================================
- Hits 221421 221094 -327
- Misses 66184 66546 +362
+ Partials 18134 18105 -29 ☔ View full report in Codecov by Sentry. |
5844005
to
c967d83
Compare
Signed-off-by: Shailesh Singh <[email protected]>
c967d83
to
f5a6b7f
Compare
❌ Gradle check result for f5a6b7f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
import java.util.stream.LongStream; | ||
|
||
/** Set of unsigned-longs, optimized for docvalues usage */ | ||
public final class UnsignedLongHashSet implements Accountable { |
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.
Can we add tests similar to TestDocValuesLongHashSet
in Lucene ?
@Shailesh-Kumar-Singh thank you, please add CHANGELOG.md entry for this change |
Description
The assertion logic in LongHashSet.java assumes signed long values, which leads to incorrect validation when dealing with unsigned long values. The sorted order check fails because value >= previousValue does not correctly handle unsigned comparisons.
I have described the Bug in detail in the issue - #17206
FIX
isUnsignedLong
) to LongHashSet to indicate whether the values are unsigned.Long.compareUnsigned(value, previousValue) >= 0
for unsigned long values.value >= previousValue
for signed values.assertBasedOnDataType()
.true
when initializing LongHashSet for unsigned values.Related Issues
Resolves #17206
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.