Implement native in filter #2577
Conversation
| @Override | ||
| public boolean apply(String input) | ||
| { | ||
| return values.contains(Strings.nullToEmpty(input)); |
There was a problem hiding this comment.
values cannot be null and Strings.nullToEmpty(input) also cannot be null.
There was a problem hiding this comment.
values may contain null, this should match "" but with the current code it will not. Something should be translating null to empty within values.
| new DefaultDimensionSpec(dimension, dimension) | ||
| ); | ||
| if (dimensionSelector == null) { | ||
| return new BooleanValueMatcher(values.contains(Strings.nullToEmpty(null))); |
There was a problem hiding this comment.
you can just pass in empty string
There was a problem hiding this comment.
wanted to look same with line 69. I'll address that.
There was a problem hiding this comment.
is it a allowed for values to be empty set ?
should we consider empty set as equivalent to filtering against null ?
There was a problem hiding this comment.
could be regarded false for all values imo, but seeing it had thrown exception in OR filter in runtime in original implementation, it would be better to add precondition in InDimFilter not to accept null or empty.
|
👍 |
|
👍 after travis, @navis please squash commits. |
|
@nishantmonu51 squashed |
|
Ah, It has merge conflicts. @navis can you rebase with the latest master ? |
| @@ -462,13 +463,9 @@ public TimeseriesQueryBuilder filters(String dimensionName, String value) | |||
| return this; | |||
| } | |||
|
|
|||
| public TimeseriesQueryBuilder filters(String dimensionName, String value, String... values) | |||
There was a problem hiding this comment.
IMO the old signature was better- it enforces at least one value must be passed in. an IN () with empty list seems strange to me and not very meaningful.
There was a problem hiding this comment.
I believe the old impl would actually have thrown an exception in this case. It would have tried to construct an empty OrFilter, which isn't allowed.
There was a problem hiding this comment.
ah I see you're checking this in InDimFilter's constructor. In that case why not keep value, values....
|
would be nice to see some additional tests too, especially tests involving null handling. |
|
Looks good other than some minor comments and a desire for tests. |
|
👍, looks good once the tests can compile. BaseFilterTest was adjusted in #2753 such that it could also test filtered aggregators; the tests need to call it a little differently since then. |
|
rebased to master and squashed |
|
👍 |
Currently, InDimFilter is translated to "or + selector filters". Value matcher can use hash set for faster filtering.