-
Notifications
You must be signed in to change notification settings - Fork 210
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
Improve partition boundaries #505
Conversation
ItemsSketchSortedView::getPartitionBoundaries(..). This change dramatically improved the handling of the end points and isolates this adjustment from the SortedView class members. The changes to SortedViewIterator and GenericSortedViewIterator are to add some functionality of getting the key iteration variables without having to specify the sorting criteria. Added a specific test to check the upper and lower boundary adjustments of a partition.
Makes other improvements to the getPartionBoundaries code.
changing the last line to use ArrayOfLongsSerDe() instead.
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.
I don't know that I 100% get the tests in ItemsSketchPartitionBoundariesTest but I think they seemed fine.
Two minor comments (one a request for a comment, the other something that's probably not necessary). Nice to fix, not gonna block this on them.
adjLen += adjLow ? 1 : 0; | ||
adjLen += adjHigh ? 1 : 0; | ||
|
||
final T[] adjQuantiles; |
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.
This is the start of a long code block with no indication of what it's doing. It'd be useful to have a little description of what's going on here.
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.
Good feedback. Will do.
|
||
import java.util.ArrayList; | ||
import java.util.Arrays; |
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.
This is probably extraneous?
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.
Yes, and so is: import static org.testng.Assert.fail;
I will fix.
This fixes a bug discovered in recent Druid testing.
Makes other improvements to the getPartionBoundaries code.