Allow mixing set-based and regexp-based include and exclude #63325

hchargois · 2020-10-06T13:05:03Z

This PR adds the feature discussed in #62246

It allows mixing set-based and regexp-based "include" and "exclude" parameters in Terms (and SignificantTerms, RareTerms) aggregations. Both ways are supported: a set include with a regexp exclude, or a regexp include with a set exclude.

hchargois · 2020-10-06T13:25:38Z

There is one thing I'm a bit unsure of, around the stream serialization, and which version to set. I set the next major version (8.0.0) as default but I guess if this is merged in a 7.x branch this would need changing.

Also, let's say we wanted to backport this feature to our own build of ES v6, do you think it could be done?

elasticmachine · 2020-10-06T15:28:08Z

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

not-napoleon · 2020-10-07T18:53:23Z

@elasticmachine test this please

nik9000 · 2020-10-07T19:19:37Z

@elasticmachine, ok to test

not-napoleon · 2020-10-07T17:22:29Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/IncludeExclude.java

         */
        @Override
        public boolean accept(BytesRef value) {
-            return ((valids == null) || (valids.contains(value))) && ((invalids == null) || (!invalids.contains(value)));
+            if (valids != null && !valids.contains(value)) {


Suggested change

if (valids != null && !valids.contains(value)) {

if (valids != null && (valids.contains(value) == false)) {

Elastic coding standard prefers this form for readability

not-napoleon · 2020-10-07T17:23:58Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/IncludeExclude.java

+                return false;
+            }
+
+            if (runAutomaton != null && !runAutomaton.run(value.bytes, value.offset, value.length)) {


Suggested change

if (runAutomaton != null && !runAutomaton.run(value.bytes, value.offset, value.length)) {

if (runAutomaton != null && (runAutomaton.run(value.bytes, value.offset, value.length) == false)) {

not-napoleon · 2020-10-07T18:48:26Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/IncludeExclude.java

+    }
+
+    public IncludeExclude(RegExp include, RegExp exclude, SortedSet<BytesRef> includeValues, SortedSet<BytesRef> excludeValues) {
+        if (include == null && exclude == null && includeValues == null && excludeValues == null) {


I think the intention here is that at most one of (include, includeValues) and at most one of (exclude, excludeValues) will be non-null. In other words, while you can mix set-based includes and regex excludes (or vice versa), you can't have both set-based and regex-based includes. That seems like a requirement of the precedence rules, among other things.

I think we should enforce that rule here. I know the parser doesn't currently allow for specifying both a regex and a set at the same time, but it's ultimately this class's contract that both not be set, and this class should check it.

I think that in IncludeExclude any combination of the 4 (include, includeValues, exclude, excludeValues) can work correctly. Meaning that we can have both kinds of includes and/or both kind of excludes and it would "do the right/logical thing", i.e. it would accept terms that are in any of the include(s) but not in any of the exclude(s). I don't think there is precedence between both kinds of includes, nor between both kinds of excludes, just between include(s) and exclude(s).

Also, I don't think restricting to a single include and a single exclude would be more efficient (e.g. allow optimizations in the accept functions).

That's why I didn't forbid having both types of includes or both types of excludes. But I can definitely add a check to forbid it.

The case I am worried about, if you have something like include = "foo.*" and includeValues = ["bar", "quux"], it will incorrectly reject the term "foo" by returning false on line 211, I think. I don't think we need to support that case, but we do need to explicitly reject it, by not allowing both include and includeValues to be set.

You're right, I was wrong in my previous comment. I think the accept from the ordinals filter can work with both kinds of includes (or excludes), but indeed not the string filter, where I assumed there was only one kind of each.

I've added a check to forbid this

not-napoleon · 2020-10-07T19:10:29Z

...r/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java

+                    aggregationBuilder = new TermsAggregationBuilder("_name").userValueTypeHint(ValueType.STRING)
+                        .executionHint(executionHint)
+                        .includeExclude(new IncludeExclude("val00.+", null, null,
+                            new String[]{"val001", "val002", "val003", "val004", "val005", "val006", "val007", "val008"}))


I appreciate that you added tests to make sure that the precedence is preserved with the new possible configurations, but I think we should test this on IncludeExcludeTests as well.

I've refactored/added tests for all possible combinations of include/exclude in IncludeExcludeTests.

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/IncludeExclude.java

not-napoleon · 2020-10-12T14:28:20Z

I think the BWC failure is related to us cutting a branch around the time this test ran, I didn't see anything that looked related to this change. When you make the requested changes, we'll run the whole suite again and if BWC passes then, that's fine.

hchargois · 2020-10-15T17:17:47Z

I think the BWC failure is related to us cutting a branch around the time this test ran, I didn't see anything that looked related to this change. When you make the requested changes, we'll run the whole suite again and if BWC passes then, that's fine.

All right. Do you want me to merge master into my branch? Or rebase onto it?

not-napoleon · 2020-10-15T19:18:16Z

This looks great, thanks. Yeah, if you could merge master into the branch, that should fix the BWC tests. I'm OOO tomorrow, but hopefully we can get this merged Monday or Tuesday.

hchargois · 2020-10-19T15:20:23Z

I merged master. However, BWC is still failing on my machine, but actually it's even failing for the master branch. It looks like other recent PRs are also failing the same tests though. We'll see how it goes with the CI but I'm not very hopeful.

Update: OK so BWC passed in the CI but another task failed. It seems to me that it's not related to my changes. I tried reproducing the two failing tests on my machine with the command lines given in the log outputs, and they both pass successfully. I'm not sure what to do now

not-napoleon · 2020-10-20T13:37:15Z

I agree that test failure doesn't look related. I'll look into it.

not-napoleon · 2020-10-20T13:45:18Z

@elasticmachine run elasticsearch-ci/1

not-napoleon · 2020-10-20T13:58:45Z

One of those is a known issue: #63719. I'm rerunning the suite to see if a different random seed fixes it. I'll dig in a bit more on the second failure and open an issue if necessary.

not-napoleon · 2020-10-20T14:56:34Z

This should be all set. I'll get it merged and backported this week. Thanks for coding it up!

hchargois · 2020-10-21T09:24:57Z

Awesome! Thank you for the review!

…63325) * Allow mixing set-based and regexp-based include and exclude * Coding style * Disallow having both set and regexp include (resp. exclude) * Test correctness of every combination of include/exclude

…3325) (#64014) * Allow mixing set-based and regexp-based include and exclude (#63325) Co-authored-by: Hugo Chargois <hugo.chargois@free.fr>

Allow mixing set-based and regexp-based include and exclude

c37c0a5

cbuescher added the :Analytics/Aggregations Aggregations label Oct 6, 2020

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 6, 2020

nik9000 requested a review from not-napoleon October 7, 2020 16:42

nik9000 added >enhancement v7.11.0 v8.0.0 labels Oct 7, 2020

not-napoleon requested changes Oct 7, 2020

View reviewed changes

hchargois added 3 commits October 15, 2020 17:34

Coding style

0a2b7ab

Disallow having both set and regexp include (resp. exclude)

0754ea7

Test correctness of every combination of include/exclude

3e90b78

Merge branch 'master' into allow-mixing-set-and-regexp-include-exclude

9af1353

not-napoleon approved these changes Oct 20, 2020

View reviewed changes

not-napoleon merged commit ff736f0 into elastic:master Oct 21, 2020

not-napoleon added the backport pending label Oct 21, 2020

not-napoleon mentioned this pull request Oct 21, 2020

[7.x] Allow mixing set-based and regexp-based include and exclude (#63325) #64014

Merged

not-napoleon removed the backport pending label Nov 2, 2020

$@polyfractal$ polyfractal mentioned this pull request Nov 11, 2020

Mixing regexp-based and set-based include and exclude in *Terms aggregations #62246

Closed

stevejgordon mentioned this pull request Dec 17, 2020

7.11.0 Meta Ticket elastic/elasticsearch-net#5198

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

not-napoleon mentioned this pull request Jul 20, 2022

[Terms] Allow multiple regex patterns into include/exclude #88532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow mixing set-based and regexp-based include and exclude #63325

Allow mixing set-based and regexp-based include and exclude #63325

hchargois commented Oct 6, 2020

hchargois commented Oct 6, 2020

elasticmachine commented Oct 6, 2020

not-napoleon commented Oct 7, 2020

nik9000 commented Oct 7, 2020

not-napoleon Oct 7, 2020

hchargois Oct 15, 2020

not-napoleon Oct 7, 2020

hchargois Oct 15, 2020

not-napoleon Oct 7, 2020

hchargois Oct 9, 2020

not-napoleon Oct 12, 2020

hchargois Oct 15, 2020

not-napoleon Oct 7, 2020

hchargois Oct 15, 2020

not-napoleon commented Oct 12, 2020

hchargois commented Oct 15, 2020

not-napoleon commented Oct 15, 2020

hchargois commented Oct 19, 2020 •

edited

Loading

not-napoleon commented Oct 20, 2020

not-napoleon commented Oct 20, 2020

not-napoleon commented Oct 20, 2020

not-napoleon commented Oct 20, 2020

hchargois commented Oct 21, 2020

	if (valids != null && !valids.contains(value)) {
	if (valids != null && (valids.contains(value) == false)) {

	if (runAutomaton != null && !runAutomaton.run(value.bytes, value.offset, value.length)) {
	if (runAutomaton != null && (runAutomaton.run(value.bytes, value.offset, value.length) == false)) {

Allow mixing set-based and regexp-based include and exclude #63325

Allow mixing set-based and regexp-based include and exclude #63325

Conversation

hchargois commented Oct 6, 2020

hchargois commented Oct 6, 2020

elasticmachine commented Oct 6, 2020

not-napoleon commented Oct 7, 2020

nik9000 commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-napoleon commented Oct 12, 2020

hchargois commented Oct 15, 2020

not-napoleon commented Oct 15, 2020

hchargois commented Oct 19, 2020 • edited Loading

not-napoleon commented Oct 20, 2020

not-napoleon commented Oct 20, 2020

not-napoleon commented Oct 20, 2020

not-napoleon commented Oct 20, 2020

hchargois commented Oct 21, 2020

hchargois commented Oct 19, 2020 •

edited

Loading