diff --git a/docs/changelog/96265.yaml b/docs/changelog/96265.yaml new file mode 100644 index 0000000000000..653dd1ddea099 --- /dev/null +++ b/docs/changelog/96265.yaml @@ -0,0 +1,6 @@ +pr: 96265 +summary: Reduce nesting of same bool queries +area: Query Languages +type: enhancement +issues: + - 96236 diff --git a/x-pack/plugin/eql/src/test/resources/querytranslator_tests.txt b/x-pack/plugin/eql/src/test/resources/querytranslator_tests.txt index e72260fbffc52..75df2796848aa 100644 --- a/x-pack/plugin/eql/src/test/resources/querytranslator_tests.txt +++ b/x-pack/plugin/eql/src/test/resources/querytranslator_tests.txt @@ -208,14 +208,14 @@ twoFunctionsEqualsBooleanLiterals-caseSensitive process where endsWith(process_path, "x") == true and endsWith(process_path, "yx") != true ; {"bool":{"must":[{"wildcard":{"process_path":{"wildcard":"*x","boost":1.0}}}, -{"bool":{"must_not":[{"wildcard":{"process_path":{"wildcard":"*yx","boost":1.0}}}],"boost":1.0}}],"boost":1.0}} +{"bool":{"must_not":[{"wildcard":{"process_path":{"wildcard":"*yx","boost":1.0}}}],"boost":1.0}} ; twoFunctionsEqualsBooleanLiterals-insensitive process where endsWith~(process_path, "x") == true and endsWith~(process_path, "yx") != true ; {"bool":{"must":[{"wildcard":{"process_path":{"wildcard":"*x","case_insensitive":true,"boost":1.0}}}, -{"bool":{"must_not":[{"wildcard":{"process_path":{"wildcard":"*yx","case_insensitive":true,"boost":1.0}}}],"boost":1.0}}],"boost":1.0}} +{"bool":{"must_not":[{"wildcard":{"process_path":{"wildcard":"*yx","case_insensitive":true,"boost":1.0}}}],"boost":1.0}} ; endsWithKeywordFieldFunction-caseSensitive diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java index 8a658b628bec2..cbc433b128072 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java @@ -53,12 +53,14 @@ import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataTypes; import org.elasticsearch.xpack.ql.util.Check; +import org.elasticsearch.xpack.ql.util.CollectionUtils; import java.time.OffsetTime; import java.time.ZoneId; import java.time.ZonedDateTime; import java.time.temporal.TemporalAccessor; import java.util.ArrayList; +import java.util.Arrays; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; @@ -464,6 +466,15 @@ private static Query boolQuery(Source source, Query left, Query right, boolean i if (right == null) { return left; } - return new BoolQuery(source, isAnd, left, right); + List queries; + // check if either side is already a bool query to an extra bool query + if (left instanceof BoolQuery bool && bool.isAnd() == isAnd) { + queries = CollectionUtils.combine(bool.queries(), right); + } else if (right instanceof BoolQuery bool && bool.isAnd() == isAnd) { + queries = CollectionUtils.combine(bool.queries(), left); + } else { + queries = Arrays.asList(left, right); + } + return new BoolQuery(source, isAnd, queries); } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/querydsl/query/BoolQuery.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/querydsl/query/BoolQuery.java index 18b4ba2d0339e..5792a63cdf73a 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/querydsl/query/BoolQuery.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/querydsl/query/BoolQuery.java @@ -9,9 +9,15 @@ import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.search.sort.NestedSortBuilder; +import org.elasticsearch.xpack.ql.QlIllegalArgumentException; import org.elasticsearch.xpack.ql.tree.Source; +import org.elasticsearch.xpack.ql.util.CollectionUtils; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; import java.util.Objects; +import java.util.StringJoiner; import static org.elasticsearch.index.query.QueryBuilders.boolQuery; @@ -23,52 +29,59 @@ public class BoolQuery extends Query { * {@code true} for boolean {@code AND}, {@code false} for boolean {@code OR}. */ private final boolean isAnd; - private final Query left; - private final Query right; + private final List queries; public BoolQuery(Source source, boolean isAnd, Query left, Query right) { + this(source, isAnd, Arrays.asList(left, right)); + } + + public BoolQuery(Source source, boolean isAnd, List queries) { super(source); - if (left == null) { - throw new IllegalArgumentException("left is required"); - } - if (right == null) { - throw new IllegalArgumentException("right is required"); + if (CollectionUtils.isEmpty(queries) || queries.size() < 2) { + throw new QlIllegalArgumentException("At least two queries required by bool query"); } this.isAnd = isAnd; - this.left = left; - this.right = right; + this.queries = queries; } @Override public boolean containsNestedField(String path, String field) { - return left.containsNestedField(path, field) || right.containsNestedField(path, field); + for (Query query : queries) { + if (query.containsNestedField(path, field)) { + return true; + } + } + return false; } @Override public Query addNestedField(String path, String field, String format, boolean hasDocValues) { - Query rewrittenLeft = left.addNestedField(path, field, format, hasDocValues); - Query rewrittenRight = right.addNestedField(path, field, format, hasDocValues); - if (rewrittenLeft == left && rewrittenRight == right) { - return this; + boolean unchanged = true; + List rewritten = new ArrayList<>(queries.size()); + for (Query query : queries) { + var rewrittenQuery = query.addNestedField(path, field, format, hasDocValues); + unchanged &= rewrittenQuery == query; + rewritten.add(rewrittenQuery); } - return new BoolQuery(source(), isAnd, rewrittenLeft, rewrittenRight); + return unchanged ? this : new BoolQuery(source(), isAnd, rewritten); } @Override public void enrichNestedSort(NestedSortBuilder sort) { - left.enrichNestedSort(sort); - right.enrichNestedSort(sort); + for (Query query : queries) { + query.enrichNestedSort(sort); + } } @Override public QueryBuilder asBuilder() { BoolQueryBuilder boolQuery = boolQuery(); - if (isAnd) { - boolQuery.must(left.asBuilder()); - boolQuery.must(right.asBuilder()); - } else { - boolQuery.should(left.asBuilder()); - boolQuery.should(right.asBuilder()); + for (Query query : queries) { + if (isAnd) { + boolQuery.must(query.asBuilder()); + } else { + boolQuery.should(query.asBuilder()); + } } return boolQuery; } @@ -77,17 +90,13 @@ public boolean isAnd() { return isAnd; } - public Query left() { - return left; - } - - public Query right() { - return right; + public List queries() { + return queries; } @Override public int hashCode() { - return Objects.hash(super.hashCode(), isAnd, left, right); + return Objects.hash(super.hashCode(), isAnd, queries); } @Override @@ -96,11 +105,15 @@ public boolean equals(Object obj) { return false; } BoolQuery other = (BoolQuery) obj; - return isAnd == other.isAnd && left.equals(other.left) && right.equals(other.right); + return isAnd == other.isAnd && queries.equals(other.queries); } @Override protected String innerToString() { - return left + (isAnd ? " AND " : " OR ") + right; + StringJoiner sb = new StringJoiner(isAnd ? " AND " : " OR "); + for (Query query : queries) { + sb.add(query.toString()); + } + return sb.toString(); } } diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/querydsl/query/BoolQueryTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/querydsl/query/BoolQueryTests.java index 74249793b72ba..6af539e015bfa 100644 --- a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/querydsl/query/BoolQueryTests.java +++ b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/querydsl/query/BoolQueryTests.java @@ -19,6 +19,8 @@ import static java.util.Collections.singletonMap; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.hamcrest.Matchers.hasSize; public class BoolQueryTests extends ESTestCase { static BoolQuery randomBoolQuery(int depth) { @@ -35,15 +37,15 @@ public void testEqualsAndHashCode() { } private static BoolQuery copy(BoolQuery query) { - return new BoolQuery(query.source(), query.isAnd(), query.left(), query.right()); + return new BoolQuery(query.source(), query.isAnd(), query.queries()); } private static BoolQuery mutate(BoolQuery query) { List> options = Arrays.asList( - q -> new BoolQuery(SourceTests.mutate(q.source()), q.isAnd(), q.left(), q.right()), - q -> new BoolQuery(q.source(), false == q.isAnd(), q.left(), q.right()), - q -> new BoolQuery(q.source(), q.isAnd(), randomValueOtherThan(q.left(), () -> NestedQueryTests.randomQuery(5)), q.right()), - q -> new BoolQuery(q.source(), q.isAnd(), q.left(), randomValueOtherThan(q.right(), () -> NestedQueryTests.randomQuery(5))) + q -> new BoolQuery(SourceTests.mutate(q.source()), q.isAnd(), left(q), right(q)), + q -> new BoolQuery(q.source(), false == q.isAnd(), left(q), right(q)), + q -> new BoolQuery(q.source(), q.isAnd(), randomValueOtherThan(left(q), () -> NestedQueryTests.randomQuery(5)), right(q)), + q -> new BoolQuery(q.source(), q.isAnd(), left(q), randomValueOtherThan(right(q), () -> NestedQueryTests.randomQuery(5))) ); return randomFrom(options).apply(query); } @@ -124,4 +126,19 @@ public void testToString() { ).toString() ); } + + public static Query left(BoolQuery bool) { + return indexOf(bool, 0); + } + + public static Query right(BoolQuery bool) { + return indexOf(bool, 1); + } + + private static Query indexOf(BoolQuery bool, int index) { + List queries = bool.queries(); + assertThat(queries, hasSize(greaterThanOrEqualTo(2))); + return queries.get(index); + } + } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index 48457b757fff3..0c20a69d2186d 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -95,6 +95,8 @@ import static java.util.Arrays.asList; import static org.elasticsearch.xpack.ql.expression.Literal.TRUE; +import static org.elasticsearch.xpack.ql.querydsl.query.BoolQueryTests.left; +import static org.elasticsearch.xpack.ql.querydsl.query.BoolQueryTests.right; import static org.elasticsearch.xpack.ql.type.DataTypes.DATETIME; import static org.elasticsearch.xpack.ql.type.DataTypes.DOUBLE; import static org.elasticsearch.xpack.ql.type.DataTypes.INTEGER; @@ -111,6 +113,7 @@ import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.everyItem; import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.startsWith; @@ -740,11 +743,11 @@ public void testInExpressionWhereClauseDatetime() { assertTrue(query instanceof BoolQuery); BoolQuery bq = (BoolQuery) query; assertFalse(bq.isAnd()); - assertTrue(bq.left() instanceof RangeQuery); - assertTrue(bq.right() instanceof RangeQuery); + assertTrue(left(bq) instanceof RangeQuery); + assertTrue(right(bq) instanceof RangeQuery); List> tuples = asList( - new Tuple<>(dates[0], (RangeQuery) bq.left()), - new Tuple<>(dates[1], (RangeQuery) bq.right()) + new Tuple<>(dates[0], (RangeQuery) left(bq)), + new Tuple<>(dates[1], (RangeQuery) right(bq)) ); for (Tuple t : tuples) { @@ -834,12 +837,12 @@ public void testDifferentLikeAndNotLikePatterns() { assertEquals(BoolQuery.class, qt.query.getClass()); BoolQuery bq = ((BoolQuery) qt.query); assertTrue(bq.isAnd()); - assertTrue(bq.left() instanceof WildcardQuery); - assertTrue(bq.right() instanceof NotQuery); + assertTrue(left(bq) instanceof WildcardQuery); + assertTrue(right(bq) instanceof NotQuery); - NotQuery nq = (NotQuery) bq.right(); + NotQuery nq = (NotQuery) right(bq); assertTrue(nq.child() instanceof WildcardQuery); - WildcardQuery lqsq = (WildcardQuery) bq.left(); + WildcardQuery lqsq = (WildcardQuery) left(bq); WildcardQuery rqsq = (WildcardQuery) nq.child(); assertEquals("X*", lqsq.query()); @@ -879,12 +882,12 @@ private void assertDifferentRLikeAndNotRLikePatterns(String firstPattern, String assertEquals(BoolQuery.class, qt.query.getClass()); BoolQuery bq = ((BoolQuery) qt.query); assertTrue(bq.isAnd()); - assertTrue(bq.left() instanceof RegexQuery); - assertTrue(bq.right() instanceof NotQuery); + assertTrue(left(bq) instanceof RegexQuery); + assertTrue(right(bq) instanceof NotQuery); - NotQuery nq = (NotQuery) bq.right(); + NotQuery nq = (NotQuery) right(bq); assertTrue(nq.child() instanceof RegexQuery); - RegexQuery lqsq = (RegexQuery) bq.left(); + RegexQuery lqsq = (RegexQuery) left(bq); RegexQuery rqsq = (RegexQuery) nq.child(); assertEquals(firstPattern, lqsq.regex()); @@ -906,14 +909,14 @@ public void testStartsWithUsesPrefixQuery() { BoolQuery bq = (BoolQuery) translation.query; assertFalse(bq.isAnd()); - assertTrue(bq.left() instanceof PrefixQuery); - assertTrue(bq.right() instanceof PrefixQuery); + assertTrue(left(bq) instanceof PrefixQuery); + assertTrue(right(bq) instanceof PrefixQuery); - PrefixQuery pqr = (PrefixQuery) bq.right(); + PrefixQuery pqr = (PrefixQuery) right(bq); assertEquals("keyword", pqr.field()); assertEquals("y", pqr.query()); - PrefixQuery pql = (PrefixQuery) bq.left(); + PrefixQuery pql = (PrefixQuery) left(bq); assertEquals("keyword", pql.field()); assertEquals("x", pql.query()); } @@ -934,20 +937,21 @@ public void testStartsWithUsesPrefixQueryAndScript() { BoolQuery bq = (BoolQuery) translation.query; assertTrue(bq.isAnd()); - assertTrue(bq.left() instanceof BoolQuery); - assertTrue(bq.right() instanceof ScriptQuery); + List queries = bq.queries(); + assertThat(queries, hasSize(3)); + assertTrue(queries.get(0) instanceof PrefixQuery); + assertTrue(queries.get(1) instanceof PrefixQuery); + assertTrue(queries.get(2) instanceof ScriptQuery); - BoolQuery bbq = (BoolQuery) bq.left(); - assertTrue(bbq.isAnd()); - PrefixQuery pqr = (PrefixQuery) bbq.right(); + PrefixQuery pqr = (PrefixQuery) queries.get(0); assertEquals("keyword", pqr.field()); - assertEquals("xy", pqr.query()); + assertEquals("x", pqr.query()); - PrefixQuery pql = (PrefixQuery) bbq.left(); + PrefixQuery pql = (PrefixQuery) queries.get(1); assertEquals("keyword", pql.field()); - assertEquals("x", pql.query()); + assertEquals("xy", pql.query()); - ScriptQuery sq = (ScriptQuery) bq.right(); + ScriptQuery sq = (ScriptQuery) queries.get(2); assertEquals( "InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.startsWith(" + "InternalSqlScriptUtils.lcase(InternalQlScriptUtils.docValue(doc,params.v0)), " diff --git a/x-pack/plugin/sql/src/test/resources/org/elasticsearch/xpack/sql/planner/querytranslator_tests.txt b/x-pack/plugin/sql/src/test/resources/org/elasticsearch/xpack/sql/planner/querytranslator_tests.txt index f33ed4aa643e3..27a56ec29c3e6 100644 --- a/x-pack/plugin/sql/src/test/resources/org/elasticsearch/xpack/sql/planner/querytranslator_tests.txt +++ b/x-pack/plugin/sql/src/test/resources/org/elasticsearch/xpack/sql/planner/querytranslator_tests.txt @@ -350,6 +350,21 @@ SELECT * FROM test WHERE some.string LIKE '%a%'; "query":{"wildcard":{"some.string.typical":{"wildcard":"*a*", ; +LikeOnInexactWithOR +SELECT * FROM test WHERE some.string LIKE '%a%' OR some.string LIKE '%b%' OR some.string LIKE '%c%'; +{"should":[{"wildcard":{"some.string.typical":{"wildcard":"*a*","boost":1.0}}},{"wildcard":{"some.string.typical":{"wildcard":"*b*","boost":1.0}}},{"wildcard":{"some.string.typical":{"wildcard":"*c*","boost":1.0}}}],"boost":1.0}} +; + +LikeOnInexactWithORandAND +SELECT * FROM test WHERE some.string LIKE '%a%' OR some.string LIKE '%b%' AND some.string LIKE '%c%' OR some.string LIKE '%d%' OR some.string LIKE '%e%'; +{"bool":{"should":[{"wildcard":{"some.string.typical":{"wildcard":"*a*","boost":1.0}}},{"bool":{"must":[{"wildcard":{"some.string.typical":{"wildcard":"*b*","boost":1.0}}},{"wildcard":{"some.string.typical":{"wildcard":"*c*","boost":1.0}}}],"boost":1.0}},{"wildcard":{"some.string.typical":{"wildcard":"*d*","boost":1.0}}},{"wildcard":{"some.string.typical":{"wildcard":"*e*","boost":1.0}}}],"boost":1.0}} +; + +LikeOnInexactWithANDandGroupedOR +SELECT * FROM test WHERE (some.string LIKE '%a%' OR some.string LIKE '%b%') AND (some.string LIKE '%c%' OR some.string LIKE '%d%' OR some.string LIKE '%e%'); +{"bool":{"must":[{"bool":{"should":[{"wildcard":{"some.string.typical":{"wildcard":"*a*","boost":1.0}}},{"wildcard":{"some.string.typical":{"wildcard":"*b*","boost":1.0}}}],"boost":1.0}},{"bool":{"should":[{"wildcard":{"some.string.typical":{"wildcard":"*c*","boost":1.0}}},{"wildcard":{"some.string.typical":{"wildcard":"*d*","boost":1.0}}},{"wildcard":{"some.string.typical":{"wildcard":"*e*","boost":1.0}}}],"boost":1.0}}],"boost":1.0}} +; + RLikeOnInexact SELECT * FROM test WHERE some.string RLIKE '.*a.*'; "query":{"regexp":{"some.string.typical":{"value":".*a.*",