Fix UT and documentation to the extraction filter #1578
Conversation
|
closing/reopening for rebuild |
| if (allDimVals != null) { | ||
| for (int i = 0; i < allDimVals.size(); i++) { | ||
| if (allDimVals != null) | ||
| { |
There was a problem hiding this comment.
old code looked ok, I think druid codebase style is to keep parenthesis on the same line except for class/method opening.
|
@b-slim your PR description mentions "Unit Test for exractionFn with empty result", however, if I understand correctly this would be a violation of the ExtractionFn contract, which explicitly states that an ExtractionFn should always return null, and never return empty string. Maybe I am missing something? |
| throw new UnsupportedOperationException(); | ||
| final DimensionSelector dimensionSelector = columnSelectorFactory.makeDimensionSelector(dimension, null); | ||
| if (dimensionSelector == null) { | ||
| return new BooleanValueMatcher(Strings.isNullOrEmpty(fn.apply(value))); |
There was a problem hiding this comment.
if a column was missing, should we ever match it in this case? fn.apply(value) looks a little bit unusual as value is really an element from the Range of the function and not Domain. may be it should be...
return new BooleanValueMatcher(false);
@xvrl ?
There was a problem hiding this comment.
i am not sure when dimensionSelector == null but following the same logic in other filter it should be return new BooleanValueMatcher(value.equals(fn.apply(null))) i guess.
|
@xvrl i have updated the test name, it is true that empty result is confusing |
Fix getBitmapIndex to consider the case were dim is null Unit Test for exractionFn with empty result and null_column UT for TopN queries with Extraction filter refactor in Extractiuon fileter makematcher for realtime segment and clean code in b/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java fix to make sure that empty string are converted to null
b-slim
force-pushed
the
fix_extraction_filter_2
branch
from
65dab6c
to
83de5a4
Compare
| "product_3": "bar_1" | ||
| } | ||
| }, | ||
| "replaceMissingValueWith": "", |
There was a problem hiding this comment.
Are these 3 properties required? I think the example would be a bit easier to digest with an initial example showing the minimum required set and then an extra explanation/example showing the optional parameters as well.
|
@cheddar please kindly check the new changes |
b-slim
force-pushed
the
fix_extraction_filter_2
branch
from
81e04cc
to
64d638a
Compare
|
@b-slim can you point to me which check you're talking about? |
|
@b-slim I may have blindly added that check out of habit, I think we should allow nulls in the extractiondimfilter |
|
@xvrl thanks for looking, can you please review the overall contribution please ? |
|
I think i have addressed all the comments |
|
👍 |
Fix UT and documentation to the extraction filter
|
otherwise it makes it hard to backport fixes, cherry-pick, or investigate regressions, because of the way commits are ordered. |
|
Now would be a nice time to be able to backport this. It can easily cause QTL to break for peons if they do not have values for the extracted dimension yet. |
This PR has several fix and unit test for extraction filter:
ExtractionFilter