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

Bin merge test & input validation #11275

Open
wants to merge 2 commits into
base: demo-rfc80-poc
Choose a base branch
from

Conversation

fuzhaoyuan
Copy link
Contributor

Describe changes proposed in this pull request:

  • Add more tests and provide input validation for future referrence

@fuzhaoyuan fuzhaoyuan force-pushed the demo-rfc80-poc-bin-range-input-validation branch 3 times, most recently from eb30807 to e55a616 Compare December 13, 2024 16:02
@fuzhaoyuan fuzhaoyuan force-pushed the demo-rfc80-poc-bin-range-input-validation branch 2 times, most recently from 8680423 to dde6703 Compare December 13, 2024 16:41
@fuzhaoyuan fuzhaoyuan force-pushed the demo-rfc80-poc-bin-range-input-validation branch from dde6703 to abdabc7 Compare December 13, 2024 16:56
@@ -138,29 +143,29 @@ public static <T extends DataFilter> List<T> mergeDataFilters(List<T> filters) {
// leave non-numerical values as they are
if (dataFilterValue.getValue() != null) {
nonNumericalValues.add(dataFilterValue);
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

@fuzhaoyuan can we add a comment explaining why non-null means non numerical? i know that is true i just think someone coming to this for the first time would be baffled by it.

Copy link
Contributor

Choose a reason for hiding this comment

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

also, a comment explaining why we're going to jump to next iteration (continue)

Copy link
Contributor

Choose a reason for hiding this comment

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

i actually think that making an else clause would be clearer/preferable to continue.

Copy link
Contributor

Choose a reason for hiding this comment

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

in general i think this logic here is very difficult to grok. because the number of items (bins) is always going to be negligible, i would prioritize readability and adopt a functional approach where we just filter out the non numericals and numericals into a separate array, sort the numericals and then use reduce to iteratively merge them depending on adjacency.

@alisman alisman added the RFC80 label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants