Skip to content

Fixes a bug in interval filter serialization#49793

Merged
romseygeek merged 2 commits intoelastic:masterfrom
romseygeek:intervals-serialization
Dec 4, 2019
Merged

Fixes a bug in interval filter serialization#49793
romseygeek merged 2 commits intoelastic:masterfrom
romseygeek:intervals-serialization

Conversation

@romseygeek
Copy link
Copy Markdown
Contributor

@romseygeek romseygeek commented Dec 3, 2019

@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
Copy Markdown
Collaborator

elasticmachine commented Dec 3, 2019

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

@mattweber
Copy link
Copy Markdown
Contributor

mattweber commented Dec 3, 2019

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

@romseygeek
Copy link
Copy Markdown
Contributor Author

romseygeek commented Dec 3, 2019

Good catch, thanks @mattweber !

cbuescher
cbuescher approved these changes Dec 3, 2019
View reviewed changes
Copy link
Copy Markdown
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?

server/src/test/java/org/elasticsearch/index/query/IntervalQueryBuilderTests.java
return new IntervalsSourceProvider.IntervalFilter(createRandomSource(depth + 1), randomFrom(filters));
}
return new IntervalsSourceProvider.IntervalFilter(
new Script(ScriptType.INLINE, "mockscript", "1", Collections.emptyMap()));
Copy link
Copy Markdown
Member

@cbuescher cbuescher Dec 3, 2019

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
Copy Markdown
Contributor Author

@romseygeek romseygeek Dec 4, 2019

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
@romseygeek
Fixes a bug in interval filter serialization (#49793)
408f25e 
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 romseygeek mentioned this pull request Dec 4, 2019
Merged
romseygeek added a commit that referenced this pull request Dec 4, 2019
@romseygeek
[CI] Interval queries cannot be cached if they use scripts (#49824)
f38ce06 
#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
@cbuescher cbuescher mentioned this pull request Dec 10, 2019
Closed
@prasad2kin
Copy link
Copy Markdown

prasad2kin commented Dec 11, 2019

@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
Copy Markdown

gpcmol commented Dec 19, 2019

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

@cbuescher
Copy link
Copy Markdown
Member

cbuescher commented Dec 19, 2019

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
@romseygeek @SivagurunathanV
Fixes a bug in interval filter serialization (elastic#49793)
03bf4fc 
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
@romseygeek @SivagurunathanV
[CI] Interval queries cannot be cached if they use scripts (elastic#4…
b1f4a85 
…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
This was referenced Feb 3, 2020
Closed
Closed
@prasad2kin
Copy link
Copy Markdown

prasad2kin commented Oct 28, 2020

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
Copy Markdown
Contributor Author

romseygeek commented Oct 29, 2020

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
Copy Markdown

prasad2kin commented Oct 29, 2020

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

Reviewers

@cbuescher cbuescher cbuescher approved these changes

Assignees

@romseygeek romseygeek

Labels

>bug :Search/Search Search-related issues that do not fall into other categories v7.6.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@romseygeek @elasticmachine @mattweber @prasad2kin @gpcmol @cbuescher @jakelandis