-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Allow filters to use extraction functions #2690
Conversation
updated docs |
rebased, removed conflicts with #2711 |
I think |
|
||
public DimensionPredicateFilter( | ||
String dimension, | ||
Predicate<String> predicate | ||
Predicate<String> predicate, | ||
ExtractionFn extractionFn |
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.
cause extraction itself is handled in predicate, I think we don't need this here (can be confusing).
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.
@navis thanks, extractionFn was unnecessary here, removed it
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.
but now it's necessary again, right?
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, it's being used now
sorry @jon-wei, more conflicts! |
re: changing SelectorFilter and JavascriptFilter to implement DimensionPredicateFilter There are some performance considerations if that change were made:
I think, if that change is desirable, it's better handled in a separate PR. |
@gianm np, rebasing now |
rebased |
@jon-wei Pair enough. 👍 |
this.extractionFn = extractionFn; | ||
} | ||
|
||
public BoundDimFilter( |
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.
Is this constructor needed? If it's only used for tests then usually we modify the tests rather than add another constructor.
(similar comment for all the other filters)
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.
IMO one nice thing about only having one constructor is that it makes it harder to accidentally create an object with a critical parameter missing.
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.
@gianm removed extra constructors
60a896a
to
a34c2f1
Compare
rebased |
@@ -98,11 +102,25 @@ public DimFilter optimize() | |||
{ | |||
if (this.getExtractionFn() instanceof LookupExtractionFn |
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.
why not return new SelectorDimFilter(dimension, value, extractionFn).optimize();
?
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.
@gianm good idea, I'll change that
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.
@gianm changed to use SelectorDimFilter optimize()
|
||
@JsonCreator | ||
public InDimFilter(@JsonProperty("dimension") String dimension, @JsonProperty("values") List<String> values) |
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 could be optimized in a similar way to the SelectorDimFilter.
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.
@gianm Can we handle the InDimFilter optimization in a separate PR?
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.
sure, you can file an issue for anything you don't think is appropriate for this PR.
// in the lookup map. Match on the selector value as well. | ||
// If the selector value is overwritten in the lookup map, don't add selector value to keys. | ||
if (exFn.isRetainMissingValue() && mappingForValue == null) { | ||
keys.add(convertedValue); |
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.
keys
isn't guaranteed to be mutable, so it'd be better to copy it and add convertedValue
to the copy.
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 looks good to me other than that
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.
done
lgtm 👍 |
This PR allows the user to define an extraction function on each of the filters (except SpatialFilter); the extraction function will be applied to the input before the filter criteria is applied.
This merges the SelectorFilter and ExtractionFilter into a single class (ExtractionDimFilter is deprecated in this patch, ExtractionFilter has been deleted).
Fixes #2643
Also fixes #2772, #2775, #2776