Skip to content
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

Merged
merged 13 commits into from
Feb 15, 2024
Merged

Improve partition boundaries #505

merged 13 commits into from
Feb 15, 2024

Conversation

leerho
Copy link
Contributor

@leerho leerho commented Feb 14, 2024

This fixes a bug discovered in recent Druid testing.

Makes other improvements to the getPartionBoundaries code.

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.
Copy link
Contributor

@jmalkin jmalkin left a 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;
Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably extraneous?

Copy link
Contributor Author

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.

@leerho leerho merged commit e89a10e into master Feb 15, 2024
4 checks passed
@leerho leerho deleted the improve_partition_boundaries branch February 15, 2024 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants