Aggs enhancement - allow Include/Exclude clauses to use array of terms #7529
Conversation
| } | ||
| -------------------------------------------------- | ||
|
|
||
|
|
There was a problem hiding this comment.
It would be nice to also have an example for exclude?
There was a problem hiding this comment.
Will do. Exclude French cars? :P
There was a problem hiding this comment.
It might also be useful to have an example showing how this supports numbers, e.g.,
"exclude": [1, 22, 33] since this is something you can't do with the related regex feature.
There was a problem hiding this comment.
Exclude French cars?
Haha :)
@nezda This feature makes sense, but it is not implemeted by this pull request yet, which only works on string fields.
There was a problem hiding this comment.
@markharwood can you add support for numbers too? the string functionality can be done with regexes now, but numeric field exclusions cannot
|
The change looks good to me overall. I think you just missed to add this new API to |
|
Thanks for the review. Will make suggested changes |
|
@jpountz Any chance of another review on this? I've got a big performance improvement blocked on this feature (150ms vs 74 seconds for a query) |
|
Bundled the optimisation for non-regex terms referenced in #7526 as part of this PR |
| // 3) Traversing the term enum for all terms and running past regexes | ||
| // Option 3 is known to be very slow in the case of high-cardinality fields and | ||
| // should be avoided if possible. | ||
| if (includeValues != null) { |
There was a problem hiding this comment.
What if both includeValues and excludeValues are specified?
There was a problem hiding this comment.
What if both includeValues and excludeValues are specified?
The excludeValues take precedence over the include. It's a weird scenario because the user is saying "I need this but also don't want this". An example scenario is a user might throw us a bag of top words to analyse but might use a small "exclude" list of stopwords. Arguably they could remove them from the include list but I'm choosing not to penalise users for non-sensical queries here.
There was a problem hiding this comment.
Ok, I was missing that piece of reasoning. This global ord lookup logic looks good!
…s as alternative to a regex Closes #6782
|
Rebased - good to go on this? Fixes a nasty performance issue. |
| return false; | ||
|
|
||
| if(hasRegexTest){ | ||
| //We need to perform UTF8 to UTF16 conversion for use in the regex matching |
There was a problem hiding this comment.
Can you add a space after the if and the //?
|
LGTM |
|
In the future I'm wondering that it might be cleaner to switch this to Lucene's logic to intersect terms enums with automatons (there is already existing logic to translate a regexp to an automaton). |
|
Thanks for the review, Adrien! Will patch and push to master and 1.x.
Agreed there is scope for future work here. IncludeExclude feels weird having both regex and straight-match logic in a single class right now but introducing pluggable term filtering impls feels like a bigger change. There's still a big cost in using regexes on high-cardinality fields (it iterates ALL terms) and this is not caught by existing timeout logic (but is by #4586 which is also something we need to decide on). |
Agreed, that is where I hope Lucene's automaton-based intersection would help since it would allow to not evaluate the regexp on every term (it takes advantage of the structure of the automaton and of the fact that the terms are sorted to rapidly skip over non-matching terms). |
|
Committed as 3c8f8cc |
This is provided as an alternative means to filtering with a regex string.
Using this approach clients no longer need to escape terms provided in the query DSL.
Closes #6782