Skip to content
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

Fixes a bug in interval filter serialization #49793

Merged
merged 2 commits into from
Dec 4, 2019

Conversation

romseygeek
Copy link
Contributor

@mattweber found an error in interval script filter serialization as part of #49519,
which I'm pulling out and fixing separately here.

@romseygeek romseygeek added >bug :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.6.0 labels Dec 3, 2019
@romseygeek romseygeek self-assigned this Dec 3, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@mattweber
Copy link
Contributor

@romseygeek you will also want to fix equals and hashCode as they are missing script checks as well.

@romseygeek
Copy link
Contributor Author

Good catch, thanks @mattweber !

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romseygeek the fix LGTM, but we have a bit of a hole in our testing here that I pointed out in a comment. Would you mind opening a follow up issue that we need to maybe need some unit test for the various IntervalsSourceProviders directly?

return new IntervalsSourceProvider.IntervalFilter(createRandomSource(depth + 1), randomFrom(filters));
}
return new IntervalsSourceProvider.IntervalFilter(
new Script(ScriptType.INLINE, "mockscript", "1", Collections.emptyMap()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding these two variations is great to catch the xContent NPE but unfortunately doesn't directly trigger the Equals/Hashcode tests for the query builder due to a lack of coverage we have here (see https://github.com/elastic/elasticsearch/blob/master/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java#L578). Without the equals/hashCode fixes this will trigger some of the "cachability" tests in this class, but very rarely.
I think adding these tests is out of scope for this PR but we should open an issue to maybe test wire- and xContent serialization of all the IntervalsSourceProvider implementations directly in unit tests. There seem to be none at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cbuescher , I opened #49820 as a followup

@romseygeek romseygeek merged commit 3008659 into elastic:master Dec 4, 2019
@romseygeek romseygeek deleted the intervals-serialization branch December 4, 2019 08:47
romseygeek added a commit that referenced this pull request Dec 4, 2019
There is a possible NPE in IntervalFilter xcontent serialization when scripts are
used, and `equals` and `hashCode` are also incorrectly implemented for script
filters.  This commit fixes both.
romseygeek added a commit that referenced this pull request Dec 4, 2019
#49793 added test coverage for interval queries that contain script filters, but did
not adjust testCacheability(), which how fails occasionally when given a random
interval source containing a script. This commit overrides testCacheability() to
explicitly sources with and without script filters.

Fixes #49821
@prasad2kin
Copy link

@mattweber @romseygeek, thanks for reporting and fixing this. I too was struggling with this problem for a week and trying to search the documentation/source code if I'm making any mistake.

Finally, I've reported this yesterday found myself convinced that it is an issue in the elastic code.: #50036

Just a quick question. Is it possible to backport this to 7.3.x branch?

@gpcmol
Copy link

gpcmol commented Dec 19, 2019

@romseygeek @cbuescher Any ideas if this can be backported to 7.3.x fix branch?

@cbuescher
Copy link
Member

7.3.x fix branch

@gpcmol we don't do patch releases on older minor versions, and since 7.5.1 is already out, thats the version we are currently releasing patches for.
This change however is currently only merged to the 7.6 branch.

SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
There is a possible NPE in IntervalFilter xcontent serialization when scripts are
used, and `equals` and `hashCode` are also incorrectly implemented for script
filters.  This commit fixes both.
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
…9824)

elastic#49793 added test coverage for interval queries that contain script filters, but did
not adjust testCacheability(), which how fails occasionally when given a random
interval source containing a script. This commit overrides testCacheability() to
explicitly sources with and without script filters.

Fixes elastic#49821
@prasad2kin
Copy link

Hi @romseygeek, thanks for fixing this bug.

Just a question, how do I use this filter if I have to query for noWithin? Say, e.g. If I want to find the documents containing the term cake but not within the distance of 3 of the term tea?

Something like: description:(cake NOTNEAR3 tea) using the Intervals query or filter?

@romseygeek
Copy link
Contributor Author

Hi @prasad2kin, you should be able to use something like:

"match" : { 
  "query" : "cake", 
  "filter" : { 
    "not_overlapping" : {
      "match" : {
        "query" : "cake tea",
        "max_gaps" : 3
      }
    }
  }
}

@prasad2kin
Copy link

Thanks for this Alan ( @romseygeek ). I'll try it. The above was a simple example, but I've use cases where I have a mix of wildcards + terms or fuzzy snippets.

Essentially, I was after getting an equivalent of span_not query. In fact, I've raised a feature request for the same: #64329

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v7.6.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants