honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Make sure SortBuilders rewrite inner nested sorts (#26532) 

The three SortBuilders that can have inner NestedSortBuilders currently don't
rewrite any of the filters contained in them. This change adds a rewrite method
to NestedSortBuilder and changes rewriting in FieldSortBuilder,
ScriptSortBuilder and GeoDistanceSortBuilder to make sure inner nested sorts get
rewritten if they need to.
This commit is contained in:
Christoph Büscher 2017-09-07 14:04:50 +02:00 committed by GitHub
parent 6f69b25f61
commit ba02485541
10 changed files with 241 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java
View File

@ -83,6 +83,7 @@ public interface QueryBuilder extends NamedWriteable, ToXContentObject, Rewritea
* Rewrites this query builder into its primitive form. By default this method return the builder itself. If the builder
* did not change the identity reference must be returned otherwise the builder will be rewritten infinitely.
*/
@Override
default QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException {
return this;
}

10
core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java
View File

@ -409,13 +409,21 @@ public class FieldSortBuilder extends SortBuilder<FieldSortBuilder> {
@Override
public FieldSortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
if (nestedFilter == null) {
if (nestedFilter == null && nestedSort == null) {
return this;
}
if (nestedFilter != null) {
QueryBuilder rewrite = nestedFilter.rewrite(ctx);
if (nestedFilter == rewrite) {
return this;
}
return new FieldSortBuilder(this).setNestedFilter(rewrite);
} else {
NestedSortBuilder rewrite = nestedSort.rewrite(ctx);
if (nestedSort == rewrite) {
return this;
}
return new FieldSortBuilder(this).setNestedSort(rewrite);
}
}
}

12
core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java
View File

@ -681,14 +681,22 @@ public class GeoDistanceSortBuilder extends SortBuilder<GeoDistanceSortBuilder>
}
@Override
public SortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
if (nestedFilter == null) {
public GeoDistanceSortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
if (nestedFilter == null && nestedSort == null) {
return this;
}
if (nestedFilter != null) {
QueryBuilder rewrite = nestedFilter.rewrite(ctx);
if (nestedFilter == rewrite) {
return this;
}
return new GeoDistanceSortBuilder(this).setNestedFilter(rewrite);
} else {
NestedSortBuilder rewrite = nestedSort.rewrite(ctx);
if (nestedSort == rewrite) {
return this;
}
return new GeoDistanceSortBuilder(this).setNestedSort(rewrite);
}
}
}

20
core/src/main/java/org/elasticsearch/search/sort/NestedSortBuilder.java
View File

@ -27,6 +27,7 @@ import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryRewriteContext;
import java.io.IOException;
import java.util.Objects;
@ -149,4 +150,23 @@ public class NestedSortBuilder implements Writeable, ToXContentObject {
public int hashCode() {
return Objects.hash(path, filter, nestedSort);
}
public NestedSortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
if (filter == null && nestedSort == null) {
return this;
}
QueryBuilder rewriteFilter = this.filter;
NestedSortBuilder rewriteNested = this.nestedSort;
if (filter != null) {
rewriteFilter = filter.rewrite(ctx);
}
if (nestedSort != null) {
rewriteNested = nestedSort.rewrite(ctx);
}
if (rewriteFilter != this.filter || rewriteNested != this.nestedSort) {
return new NestedSortBuilder(this.path).setFilter(rewriteFilter).setNestedSort(rewriteNested);
} else {
return this;
}
}
}

2
core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java
View File

@ -124,7 +124,7 @@ public class ScoreSortBuilder extends SortBuilder<ScoreSortBuilder> {
}
@Override
public SortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
public ScoreSortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
return this;
}
}

10
core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java
View File

@ -451,13 +451,21 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> {
@Override
public ScriptSortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
if (nestedFilter == null) {
if (nestedFilter == null && nestedSort == null) {
return this;
}
if (nestedFilter != null) {
QueryBuilder rewrite = nestedFilter.rewrite(ctx);
if (nestedFilter == rewrite) {
return this;
}
return new ScriptSortBuilder(this).setNestedFilter(rewrite);
} else {
NestedSortBuilder rewrite = nestedSort.rewrite(ctx);
if (nestedSort == rewrite) {
return this;
}
return new ScriptSortBuilder(this).setNestedSort(rewrite);
}
}
}

38
core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java
View File

@ -33,9 +33,13 @@ import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.N
import org.elasticsearch.index.mapper.KeywordFieldMapper;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.TypeFieldMapper;
import org.elasticsearch.index.query.MatchNoneQueryBuilder;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.index.query.QueryRewriteContext;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.index.query.QueryShardException;
import org.elasticsearch.index.query.RangeQueryBuilder;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.MultiValueMode;
@ -367,6 +371,40 @@ public class FieldSortBuilderTests extends AbstractSortTestCase<FieldSortBuilder
assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage());
}
/**
* Test the the nested Filter gets rewritten
*/
public void testNestedRewrites() throws IOException {
FieldSortBuilder sortBuilder = new FieldSortBuilder(MAPPED_STRING_FIELDNAME);
RangeQueryBuilder rangeQuery = new RangeQueryBuilder("fieldName") {
@Override
public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException {
return new MatchNoneQueryBuilder();
}
};
sortBuilder.setNestedPath("path").setNestedFilter(rangeQuery);
FieldSortBuilder rewritten = (FieldSortBuilder) sortBuilder
.rewrite(createMockShardContext());
assertNotSame(rangeQuery, rewritten.getNestedFilter());
}
/**
* Test the the nested sort gets rewritten
*/
public void testNestedSortRewrites() throws IOException {
FieldSortBuilder sortBuilder = new FieldSortBuilder(MAPPED_STRING_FIELDNAME);
RangeQueryBuilder rangeQuery = new RangeQueryBuilder("fieldName") {
@Override
public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException {
return new MatchNoneQueryBuilder();
}
};
sortBuilder.setNestedSort(new NestedSortBuilder("path").setFilter(rangeQuery));
FieldSortBuilder rewritten = (FieldSortBuilder) sortBuilder
.rewrite(createMockShardContext());
assertNotSame(rangeQuery, rewritten.getNestedSort().getFilter());
}
@Override
protected void assertWarnings(FieldSortBuilder testItem) {
List<String> expectedWarnings = new ArrayList<>();

37
core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java
View File

@ -39,8 +39,12 @@ import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.TypeFieldMapper;
import org.elasticsearch.index.query.GeoValidationMethod;
import org.elasticsearch.index.query.MatchAllQueryBuilder;
import org.elasticsearch.index.query.MatchNoneQueryBuilder;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.index.query.QueryRewriteContext;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.index.query.RangeQueryBuilder;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.MultiValueMode;
import org.elasticsearch.test.geo.RandomGeoGenerator;
@ -581,7 +585,40 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase<GeoDistanc
iae = expectThrows(IllegalArgumentException.class,
() -> sortBuilder.setNestedSort(new NestedSortBuilder("otherPath")).setNestedFilter(QueryBuilders.matchAllQuery()));
assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage());
}
/**
* Test the the nested Filter gets rewritten
*/
public void testNestedRewrites() throws IOException {
GeoDistanceSortBuilder sortBuilder = new GeoDistanceSortBuilder("fieldName", 0.0, 0.0);
RangeQueryBuilder rangeQuery = new RangeQueryBuilder("fieldName") {
@Override
public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException {
return new MatchNoneQueryBuilder();
}
};
sortBuilder.setNestedPath("path").setNestedFilter(rangeQuery);
GeoDistanceSortBuilder rewritten = (GeoDistanceSortBuilder) sortBuilder
.rewrite(createMockShardContext());
assertNotSame(rangeQuery, rewritten.getNestedFilter());
}
/**
* Test the the nested sort gets rewritten
*/
public void testNestedSortRewrites() throws IOException {
GeoDistanceSortBuilder sortBuilder = new GeoDistanceSortBuilder("fieldName", 0.0, 0.0);
RangeQueryBuilder rangeQuery = new RangeQueryBuilder("fieldName") {
@Override
public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException {
return new MatchNoneQueryBuilder();
}
};
sortBuilder.setNestedSort(new NestedSortBuilder("path").setFilter(rangeQuery));
GeoDistanceSortBuilder rewritten = (GeoDistanceSortBuilder) sortBuilder
.rewrite(createMockShardContext());
assertNotSame(rangeQuery, rewritten.getNestedSort().getFilter());
}
}

66
core/src/test/java/org/elasticsearch/search/sort/NestedSortBuilderTests.java
View File

@ -27,11 +27,17 @@ import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.query.ConstantScoreQueryBuilder;
import org.elasticsearch.index.query.MatchAllQueryBuilder;
import org.elasticsearch.index.query.MatchNoneQueryBuilder;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryRewriteContext;
import org.elasticsearch.search.SearchModule;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.EqualsHashCodeTestUtils;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.mockito.Mockito;
import java.io.IOException;
@ -136,4 +142,64 @@ public class NestedSortBuilderTests extends ESTestCase {
NestedSortBuilderTests::mutate);
}
}
/**
* Test that filters and inner nested sorts get rewritten
*/
public void testRewrite() throws IOException {
QueryBuilder filterThatRewrites = new MatchNoneQueryBuilder() {
@Override
protected QueryBuilder doRewrite(org.elasticsearch.index.query.QueryRewriteContext queryShardContext) throws IOException {
return new MatchAllQueryBuilder();
};
};
// test that filter gets rewritten
NestedSortBuilder original = new NestedSortBuilder("path").setFilter(filterThatRewrites);
QueryRewriteContext mockRewriteContext = Mockito.mock(QueryRewriteContext.class);
NestedSortBuilder rewritten = original.rewrite(mockRewriteContext);
assertNotSame(rewritten, original);
assertNotSame(rewritten.getFilter(), original.getFilter());
// test that inner nested sort gets rewritten
original = new NestedSortBuilder("path");
original.setNestedSort(new NestedSortBuilder("otherPath").setFilter(filterThatRewrites));
rewritten = original.rewrite(mockRewriteContext);
assertNotSame(rewritten, original);
assertNotSame(rewritten.getNestedSort(), original.getNestedSort());
// test that both filter and inner nested sort get rewritten
original = new NestedSortBuilder("path");
original.setFilter(filterThatRewrites);
original.setNestedSort(new NestedSortBuilder("otherPath").setFilter(filterThatRewrites));
rewritten = original.rewrite(mockRewriteContext);
assertNotSame(rewritten, original);
assertNotSame(rewritten.getFilter(), original.getFilter());
assertNotSame(rewritten.getNestedSort(), original.getNestedSort());
// test that original stays unchanged if no element rewrites
original = new NestedSortBuilder("path");
original.setFilter(new MatchNoneQueryBuilder());
original.setNestedSort(new NestedSortBuilder("otherPath").setFilter(new MatchNoneQueryBuilder()));
rewritten = original.rewrite(mockRewriteContext);
assertSame(rewritten, original);
assertSame(rewritten.getFilter(), original.getFilter());
assertSame(rewritten.getNestedSort(), original.getNestedSort());
// test that rewrite works recursively
original = new NestedSortBuilder("firstLevel");
ConstantScoreQueryBuilder constantScoreQueryBuilder = new ConstantScoreQueryBuilder(filterThatRewrites);
original.setFilter(constantScoreQueryBuilder);
NestedSortBuilder nestedSortThatRewrites = new NestedSortBuilder("thirdLevel")
.setFilter(filterThatRewrites);
original.setNestedSort(new NestedSortBuilder("secondLevel").setNestedSort(nestedSortThatRewrites));
rewritten = original.rewrite(mockRewriteContext);
assertNotSame(rewritten, original);
assertNotSame(rewritten.getFilter(), constantScoreQueryBuilder);
assertNotSame(((ConstantScoreQueryBuilder) rewritten.getFilter()).innerQuery(), constantScoreQueryBuilder.innerQuery());
assertEquals("secondLevel", rewritten.getNestedSort().getPath());
assertNotSame(rewritten.getNestedSort(), original.getNestedSort());
assertEquals("thirdLevel", rewritten.getNestedSort().getNestedSort().getPath());
assertNotSame(rewritten.getNestedSort().getNestedSort(), nestedSortThatRewrites);
}
}

38
core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java
View File

@ -33,6 +33,10 @@ import org.elasticsearch.index.fielddata.fieldcomparator.DoubleValuesComparatorS
import org.elasticsearch.index.mapper.TypeFieldMapper;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.index.query.MatchNoneQueryBuilder;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryRewriteContext;
import org.elasticsearch.index.query.RangeQueryBuilder;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.DocValueFormat;
@ -355,6 +359,40 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase<ScriptSortBuild
assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage());
}
/**
* Test the the nested Filter gets rewritten
*/
public void testNestedRewrites() throws IOException {
ScriptSortBuilder sortBuilder = new ScriptSortBuilder(mockScript("something"), ScriptSortType.STRING);
RangeQueryBuilder rangeQuery = new RangeQueryBuilder("fieldName") {
@Override
public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException {
return new MatchNoneQueryBuilder();
}
};
sortBuilder.setNestedPath("path").setNestedFilter(rangeQuery);
ScriptSortBuilder rewritten = (ScriptSortBuilder) sortBuilder
.rewrite(createMockShardContext());
assertNotSame(rangeQuery, rewritten.getNestedFilter());
}
/**
* Test the the nested sort gets rewritten
*/
public void testNestedSortRewrites() throws IOException {
ScriptSortBuilder sortBuilder = new ScriptSortBuilder(mockScript("something"), ScriptSortType.STRING);
RangeQueryBuilder rangeQuery = new RangeQueryBuilder("fieldName") {
@Override
public QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException {
return new MatchNoneQueryBuilder();
}
};
sortBuilder.setNestedSort(new NestedSortBuilder("path").setFilter(rangeQuery));
ScriptSortBuilder rewritten = (ScriptSortBuilder) sortBuilder
.rewrite(createMockShardContext());
assertNotSame(rangeQuery, rewritten.getNestedSort().getFilter());
}
@Override
protected ScriptSortBuilder fromXContent(XContentParser parser, String fieldName) throws IOException {
return ScriptSortBuilder.fromXContent(parser, fieldName);