From da51a0f2932300ad217ae6a6f658a01c65ff4697 Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Mon, 18 Sep 2023 13:34:25 -0700 Subject: [PATCH 1/9] Implementation of Visitor Design Pattern in QueryBuilder Signed-off-by: Varun Jain --- .../index/query/BoolQueryBuilder.java | 31 +++++++++++++++++++ .../index/query/BoostingQueryBuilder.java | 12 +++++++ .../query/ConstantScoreQueryBuilder.java | 8 +++++ .../index/query/DisMaxQueryBuilder.java | 12 +++++++ .../query/FieldMaskingSpanQueryBuilder.java | 7 +++++ .../index/query/NoopQueryVisitor.java | 28 +++++++++++++++++ .../opensearch/index/query/QueryBuilder.java | 9 ++++++ .../index/query/QueryBuilderVisitor.java | 30 ++++++++++++++++++ .../query/SpanContainingQueryBuilder.java | 8 +++++ .../index/query/SpanFirstQueryBuilder.java | 7 +++++ .../query/SpanMultiTermQueryBuilder.java | 9 ++++++ .../index/query/SpanNearQueryBuilder.java | 12 +++++++ .../index/query/SpanNotQueryBuilder.java | 13 ++++++++ .../index/query/SpanOrQueryBuilder.java | 12 +++++++ .../index/query/SpanWithinQueryBuilder.java | 8 +++++ 15 files changed, 206 insertions(+) create mode 100644 server/src/main/java/org/opensearch/index/query/NoopQueryVisitor.java create mode 100644 server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java diff --git a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java index 65c2dfa9c5a8b..c44a7ef6a397c 100644 --- a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java @@ -427,4 +427,35 @@ private static boolean rewriteClauses( } return changed; } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + if (mustClauses.isEmpty() == false) { + QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST); + for (QueryBuilder mustClause : mustClauses) { + mustClause.visit(subVisitor); + } + } + if (shouldClauses.isEmpty() == false) { + QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.SHOULD); + for (QueryBuilder shouldClause : shouldClauses) { + shouldClause.visit(subVisitor); + } + } + if (mustNotClauses.isEmpty() == false) { + QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST_NOT); + for (QueryBuilder mustNotClause : mustNotClauses) { + mustNotClause.visit(subVisitor); + } + } + if (filterClauses.isEmpty() == false) { + QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.FILTER); + for (QueryBuilder filterClause : filterClauses) { + filterClause.visit(subVisitor); + } + } + + } + } diff --git a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java index 26124b422f26f..1b52ae2f03605 100644 --- a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java @@ -33,6 +33,7 @@ package org.opensearch.index.query; import org.apache.lucene.queries.function.FunctionScoreQuery; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.Query; import org.opensearch.core.ParseField; import org.opensearch.core.common.ParsingException; @@ -252,4 +253,15 @@ protected void extractInnerHitBuilders(Map inner InnerHitContextBuilder.extractInnerHits(positiveQuery, innerHits); InnerHitContextBuilder.extractInnerHits(negativeQuery, innerHits); } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + if (positiveQuery != null) { + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery); + } + if (negativeQuery != null) { + visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(negativeQuery); + } + } } diff --git a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java index 6a29ad8a0a401..b2764d29da80a 100644 --- a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java @@ -32,6 +32,7 @@ package org.opensearch.index.query; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.ConstantScoreQuery; import org.apache.lucene.search.Query; import org.opensearch.core.ParseField; @@ -183,4 +184,11 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws protected void extractInnerHitBuilders(Map innerHits) { InnerHitContextBuilder.extractInnerHits(filterBuilder, innerHits); } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.FILTER).accept(filterBuilder); + } + } diff --git a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java index 91f4a02fac6c0..29b6b4da3f738 100644 --- a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java @@ -32,6 +32,7 @@ package org.opensearch.index.query; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.DisjunctionMaxQuery; import org.apache.lucene.search.Query; import org.opensearch.common.lucene.search.Queries; @@ -246,4 +247,15 @@ protected void extractInnerHitBuilders(Map inner InnerHitContextBuilder.extractInnerHits(query, innerHits); } } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + if (queries.isEmpty() == false) { + QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); + for (QueryBuilder subQb : queries) { + subVisitor.accept(subQb); + } + } + } } diff --git a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java index 09a71795a3f27..1162689a54689 100644 --- a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java @@ -34,6 +34,7 @@ import org.apache.lucene.queries.spans.FieldMaskingSpanQuery; import org.apache.lucene.queries.spans.SpanQuery; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.Query; import org.opensearch.core.ParseField; import org.opensearch.core.common.ParsingException; @@ -207,4 +208,10 @@ protected boolean doEquals(FieldMaskingSpanQueryBuilder other) { public String getWriteableName() { return SPAN_FIELD_MASKING_FIELD.getPreferredName(); } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(queryBuilder); + } } diff --git a/server/src/main/java/org/opensearch/index/query/NoopQueryVisitor.java b/server/src/main/java/org/opensearch/index/query/NoopQueryVisitor.java new file mode 100644 index 0000000000000..bbe4f4861dfc5 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/query/NoopQueryVisitor.java @@ -0,0 +1,28 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.query; + +import org.apache.lucene.search.BooleanClause; + +/** + * NoopQueryVisitor is a default implementation of QueryBuilderVisitor. + * When a user does not want to implement QueryBuilderVisitor and have to just pass an empty object then this class will be used. + * + */ +public class NoopQueryVisitor implements QueryBuilderVisitor { + @Override + public void accept(QueryBuilder qb) { + // It is a no operation method + } + + @Override + public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { + return null; + } +} diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java index a40ccf427794a..090f74c5be7fe 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java @@ -95,4 +95,13 @@ public interface QueryBuilder extends NamedWriteable, ToXContentObject, Rewritea default QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { return this; } + + /** + * Recurse through the QueryBuilder tree, visiting any child QueryBuilder. + * @param visitor a query builder visitor to be called by each query builder in the tree. + */ + default void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + }; + } diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java new file mode 100644 index 0000000000000..a8a17057cab79 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java @@ -0,0 +1,30 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.query; + +import org.apache.lucene.search.BooleanClause; + +/** + * QueryBuilderVisitor is an interface to define Visitor Object to be traversed in QueryBuilder tree. + */ +public interface QueryBuilderVisitor { + + /** + * Accept method is called when the visitor accepts the queryBuilder object to be traversed in the query tree. + * @param qb is a queryBuilder object which is accepeted by the visitor. + */ + void accept(QueryBuilder qb); + + /** + * Fetches the child sub visitor from the main QueryBuilderVisitor Object. + * @param occur defines the occurrence of the result fetched from the search query in the final search result. + * @return a child queryBuilder Visitor Object. + */ + QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur); +} diff --git a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java index ed4f5c6848b06..32a19ea3e9b50 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java @@ -34,6 +34,7 @@ import org.apache.lucene.queries.spans.SpanContainingQuery; import org.apache.lucene.queries.spans.SpanQuery; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.Query; import org.opensearch.core.ParseField; import org.opensearch.core.common.ParsingException; @@ -188,4 +189,11 @@ protected boolean doEquals(SpanContainingQueryBuilder other) { public String getWriteableName() { return NAME; } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); + } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java index 7427b13463284..bcbc64ddf386d 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java @@ -34,6 +34,7 @@ import org.apache.lucene.queries.spans.SpanFirstQuery; import org.apache.lucene.queries.spans.SpanQuery; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.Query; import org.opensearch.core.ParseField; import org.opensearch.core.common.ParsingException; @@ -186,4 +187,10 @@ protected boolean doEquals(SpanFirstQueryBuilder other) { public String getWriteableName() { return NAME; } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(matchBuilder); + } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java index ce28391ca478b..96d03c91964e3 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java @@ -33,6 +33,7 @@ import org.apache.lucene.queries.SpanMatchNoDocsQuery; import org.apache.lucene.queries.spans.SpanMultiTermQueryWrapper; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.BoostQuery; import org.apache.lucene.search.ConstantScoreQuery; import org.apache.lucene.search.MatchNoDocsQuery; @@ -213,4 +214,12 @@ protected boolean doEquals(SpanMultiTermQueryBuilder other) { public String getWriteableName() { return NAME; } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + if (multiTermQueryBuilder != null) { + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(multiTermQueryBuilder); + } + } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java index ba7625d94a5a6..30a1c29c29126 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java @@ -34,6 +34,7 @@ import org.apache.lucene.queries.spans.SpanNearQuery; import org.apache.lucene.queries.spans.SpanQuery; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.Query; import org.opensearch.core.ParseField; import org.opensearch.core.common.ParsingException; @@ -299,6 +300,17 @@ public String getWriteableName() { return NAME; } + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + if (this.clauses.isEmpty() == false) { + QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); + for (QueryBuilder subQb : this.clauses) { + subVisitor.accept(subQb); + } + } + } + /** * SpanGapQueryBuilder enables gaps in a SpanNearQuery. * Since, SpanGapQuery is private to SpanNearQuery, SpanGapQueryBuilder cannot diff --git a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java index 98e7f287749f5..59ec5b9d77fc8 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java @@ -34,6 +34,7 @@ import org.apache.lucene.queries.spans.SpanNotQuery; import org.apache.lucene.queries.spans.SpanQuery; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.Query; import org.opensearch.core.ParseField; import org.opensearch.core.common.ParsingException; @@ -284,4 +285,16 @@ protected boolean doEquals(SpanNotQueryBuilder other) { public String getWriteableName() { return NAME; } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + if (include != null) { + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(include); + } + + if (exclude != null) { + visitor.getChildVisitor(BooleanClause.Occur.MUST_NOT).accept(exclude); + } + } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java index 2f63e6d7403f7..fae1e318c66bd 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java @@ -34,6 +34,7 @@ import org.apache.lucene.queries.spans.SpanOrQuery; import org.apache.lucene.queries.spans.SpanQuery; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.Query; import org.opensearch.core.ParseField; import org.opensearch.core.common.ParsingException; @@ -188,4 +189,15 @@ protected boolean doEquals(SpanOrQueryBuilder other) { public String getWriteableName() { return NAME; } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + if (clauses.isEmpty() == false) { + QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); + for (QueryBuilder subQb : this.clauses) { + subVisitor.accept(subQb); + } + } + } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java index 5d02cc0026dfd..3a32b3371f17e 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java @@ -34,6 +34,7 @@ import org.apache.lucene.queries.spans.SpanQuery; import org.apache.lucene.queries.spans.SpanWithinQuery; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.Query; import org.opensearch.core.ParseField; import org.opensearch.core.common.ParsingException; @@ -197,4 +198,11 @@ protected boolean doEquals(SpanWithinQueryBuilder other) { public String getWriteableName() { return NAME; } + + @Override + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(big); + visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(little); + } } From 107b5c02d8b95c146e522ef95a265ce1ee23c5a7 Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Mon, 18 Sep 2023 13:54:42 -0700 Subject: [PATCH 2/9] Adding Changelog Signed-off-by: Varun Jain --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d374a725c84ee..8dad71b37b232 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add events correlation engine plugin ([#6854](https://github.com/opensearch-project/OpenSearch/issues/6854)) - Introduce new dynamic cluster setting to control slice computation for concurrent segment search ([#9107](https://github.com/opensearch-project/OpenSearch/pull/9107)) - Implement on behalf of token passing for extensions ([#8679](https://github.com/opensearch-project/OpenSearch/pull/8679)) +- Implement Visitor Design pattern in QueryBuilder to enable the capability to update the search request. ([#10110](https://github.com/opensearch-project/OpenSearch/pull/10110)) ### Dependencies - Bump `log4j-core` from 2.18.0 to 2.19.0 @@ -107,4 +108,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Security [Unreleased 3.0]: https://github.com/opensearch-project/OpenSearch/compare/2.x...HEAD -[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.11...2.x \ No newline at end of file +[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.11...2.x From 1d898bb77832e29209bca2c4d35133808d81680c Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Mon, 18 Sep 2023 14:01:38 -0700 Subject: [PATCH 3/9] Adding Changelog Signed-off-by: Varun Jain --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dad71b37b232..0d0a8b8ca8a17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add events correlation engine plugin ([#6854](https://github.com/opensearch-project/OpenSearch/issues/6854)) - Introduce new dynamic cluster setting to control slice computation for concurrent segment search ([#9107](https://github.com/opensearch-project/OpenSearch/pull/9107)) - Implement on behalf of token passing for extensions ([#8679](https://github.com/opensearch-project/OpenSearch/pull/8679)) -- Implement Visitor Design pattern in QueryBuilder to enable the capability to update the search request. ([#10110](https://github.com/opensearch-project/OpenSearch/pull/10110)) +- Implement Visitor Design pattern in QueryBuilder to enable the capability to traverse through the complex QueryBuilder tree. ([#10110](https://github.com/opensearch-project/OpenSearch/pull/10110)) ### Dependencies - Bump `log4j-core` from 2.18.0 to 2.19.0 From 3ffa0677e6a8ee35740d1dd38935c50e500bf9a1 Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Mon, 18 Sep 2023 15:05:02 -0700 Subject: [PATCH 4/9] Comments addressed of michael Signed-off-by: Varun Jain --- .../index/query/DisMaxQueryBuilder.java | 4 ++- .../index/query/NoopQueryVisitor.java | 28 ------------------- .../index/query/QueryBuilderVisitor.java | 18 ++++++++++++ .../index/query/SpanWithinQueryBuilder.java | 4 +-- 4 files changed, 23 insertions(+), 31 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/index/query/NoopQueryVisitor.java diff --git a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java index 29b6b4da3f738..39b211a0abf12 100644 --- a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java @@ -251,8 +251,10 @@ protected void extractInnerHitBuilders(Map inner @Override public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); + + QueryBuilderVisitor qbv = QueryBuilderVisitor.NO_OP_VISITOR; if (queries.isEmpty() == false) { - QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); + QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); for (QueryBuilder subQb : queries) { subVisitor.accept(subQb); } diff --git a/server/src/main/java/org/opensearch/index/query/NoopQueryVisitor.java b/server/src/main/java/org/opensearch/index/query/NoopQueryVisitor.java deleted file mode 100644 index bbe4f4861dfc5..0000000000000 --- a/server/src/main/java/org/opensearch/index/query/NoopQueryVisitor.java +++ /dev/null @@ -1,28 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.query; - -import org.apache.lucene.search.BooleanClause; - -/** - * NoopQueryVisitor is a default implementation of QueryBuilderVisitor. - * When a user does not want to implement QueryBuilderVisitor and have to just pass an empty object then this class will be used. - * - */ -public class NoopQueryVisitor implements QueryBuilderVisitor { - @Override - public void accept(QueryBuilder qb) { - // It is a no operation method - } - - @Override - public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { - return null; - } -} diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java index a8a17057cab79..af5a125f9dd95 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java @@ -27,4 +27,22 @@ public interface QueryBuilderVisitor { * @return a child queryBuilder Visitor Object. */ QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur); + + /** + * NoopQueryVisitor is a default implementation of QueryBuilderVisitor. + * When a user does not want to implement QueryBuilderVisitor and have to just pass an empty object then this class will be used. + * + */ + QueryBuilderVisitor NO_OP_VISITOR = new QueryBuilderVisitor() { + @Override + public void accept(QueryBuilder qb) { + // Do nothing + } + + @Override + public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { + return this; + } + }; + } diff --git a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java index 3a32b3371f17e..4d5a6dde61a70 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java @@ -202,7 +202,7 @@ public String getWriteableName() { @Override public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); - visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(big); - visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(little); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); } } From a6a1588190948e5cfbf97729db1dd0a53ae1048e Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Wed, 20 Sep 2023 11:05:01 -0700 Subject: [PATCH 5/9] Adding test case and some minor fixes in DisMaxQueryBuilder Signed-off-by: Varun Jain --- .../index/query/BoolQueryBuilder.java | 2 +- .../index/query/BoostingQueryBuilder.java | 2 +- .../query/ConstantScoreQueryBuilder.java | 2 +- .../index/query/DisMaxQueryBuilder.java | 4 +-- .../query/FieldMaskingSpanQueryBuilder.java | 2 +- .../opensearch/index/query/QueryBuilder.java | 2 +- .../index/query/QueryBuilderVisitor.java | 4 ++- .../query/SpanContainingQueryBuilder.java | 2 +- .../index/query/SpanFirstQueryBuilder.java | 2 +- .../query/SpanMultiTermQueryBuilder.java | 2 +- .../index/query/SpanNearQueryBuilder.java | 2 +- .../index/query/SpanNotQueryBuilder.java | 2 +- .../index/query/SpanOrQueryBuilder.java | 2 +- .../index/query/SpanWithinQueryBuilder.java | 2 +- .../index/query/BoolQueryBuilderTests.java | 10 +++++++ .../query/BoostingQueryBuilderTests.java | 10 +++++++ .../index/query/DisMaxQueryBuilderTests.java | 8 +++++ .../FieldMaskingSpanQueryBuilderTests.java | 5 ++++ .../test/AbstractBuilderTestCase.java | 29 +++++++++++++++++++ 19 files changed, 78 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java index c44a7ef6a397c..9465da4ab5b1c 100644 --- a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java @@ -429,7 +429,7 @@ private static boolean rewriteClauses( } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); if (mustClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST); diff --git a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java index 1b52ae2f03605..c7dafc919e8a0 100644 --- a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java @@ -255,7 +255,7 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); if (positiveQuery != null) { visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery); diff --git a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java index b2764d29da80a..e6c321e7bd40e 100644 --- a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java @@ -186,7 +186,7 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.FILTER).accept(filterBuilder); } diff --git a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java index 39b211a0abf12..6f5b3bcf75a9b 100644 --- a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java @@ -249,10 +249,8 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); - - QueryBuilderVisitor qbv = QueryBuilderVisitor.NO_OP_VISITOR; if (queries.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); for (QueryBuilder subQb : queries) { diff --git a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java index 1162689a54689..576d86b818b1b 100644 --- a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java @@ -210,7 +210,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(queryBuilder); } diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java index 090f74c5be7fe..ca97227fc0be5 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java @@ -100,7 +100,7 @@ default QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOExc * Recurse through the QueryBuilder tree, visiting any child QueryBuilder. * @param visitor a query builder visitor to be called by each query builder in the tree. */ - default void visit(QueryBuilderVisitor visitor) { + default void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); }; diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java index af5a125f9dd95..97eb21cd9504b 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java @@ -10,6 +10,8 @@ import org.apache.lucene.search.BooleanClause; +import java.io.IOException; + /** * QueryBuilderVisitor is an interface to define Visitor Object to be traversed in QueryBuilder tree. */ @@ -19,7 +21,7 @@ public interface QueryBuilderVisitor { * Accept method is called when the visitor accepts the queryBuilder object to be traversed in the query tree. * @param qb is a queryBuilder object which is accepeted by the visitor. */ - void accept(QueryBuilder qb); + void accept(QueryBuilder qb) throws IOException; /** * Fetches the child sub visitor from the main QueryBuilderVisitor Object. diff --git a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java index 32a19ea3e9b50..724fbf908ccfc 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java @@ -191,7 +191,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); diff --git a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java index bcbc64ddf386d..b79ae44e94d59 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java @@ -189,7 +189,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(matchBuilder); } diff --git a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java index 96d03c91964e3..b69e2e9e7c518 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java @@ -216,7 +216,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); if (multiTermQueryBuilder != null) { visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(multiTermQueryBuilder); diff --git a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java index 30a1c29c29126..7d76608b7ba50 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java @@ -301,7 +301,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); if (this.clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); diff --git a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java index 59ec5b9d77fc8..68bf80af3299e 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java @@ -287,7 +287,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); if (include != null) { visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(include); diff --git a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java index fae1e318c66bd..543d5220109b1 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java @@ -191,7 +191,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); if (clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); diff --git a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java index 4d5a6dde61a70..3bbedcec41950 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java @@ -200,7 +200,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { + public void visit(QueryBuilderVisitor visitor) throws IOException { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); diff --git a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java index 2160ab6220866..a84921b48c9e1 100644 --- a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java @@ -456,4 +456,14 @@ public void testMustRewrite() throws IOException { IllegalStateException e = expectThrows(IllegalStateException.class, () -> boolQuery.toQuery(context)); assertEquals("Rewrite first", e.getMessage()); } + + public void testVisit() { + BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder(); + boolQueryBuilder.should(new BoolQueryBuilder()); + boolQueryBuilder.must(new BoolQueryBuilder()); + boolQueryBuilder.mustNot(new BoolQueryBuilder()); + boolQueryBuilder.filter(new BoolQueryBuilder()); + IOException e = expectThrows(IOException.class, () -> boolQueryBuilder.visit(QueryBuilderVisitor.NO_OP_VISITOR)); + assertEquals("Boosting Query Builder Traversed", e.getMessage()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/BoostingQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/BoostingQueryBuilderTests.java index 94ded24975be4..935fe28b6aaf0 100644 --- a/server/src/test/java/org/opensearch/index/query/BoostingQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/BoostingQueryBuilderTests.java @@ -153,4 +153,14 @@ public void testMustRewrite() throws IOException { e = expectThrows(IllegalStateException.class, () -> queryBuilder2.toQuery(context)); assertEquals("Rewrite first", e.getMessage()); } + + public void testVisit() { + BoostingQueryBuilder builder = new BoostingQueryBuilder( + new TermQueryBuilder("unmapped_field", "value"), + new TermQueryBuilder(KEYWORD_FIELD_NAME, "other_value") + ); + + IOException e = expectThrows(IOException.class, () -> builder.visit(createTestVisitor())); + assertEquals("Boosting Query Builder Traversed", e.getMessage()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/DisMaxQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/DisMaxQueryBuilderTests.java index 8d589bd76f2bb..dc029e8104b2a 100644 --- a/server/src/test/java/org/opensearch/index/query/DisMaxQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/DisMaxQueryBuilderTests.java @@ -156,4 +156,12 @@ public void testRewriteMultipleTimes() throws IOException { assertEquals(rewrittenAgain, expected); assertEquals(Rewriteable.rewrite(dismax, createShardContext()), expected); } + + public void testVisit() { + DisMaxQueryBuilder dismax = new DisMaxQueryBuilder(); + dismax.add(new WrapperQueryBuilder(new WrapperQueryBuilder(new MatchAllQueryBuilder().toString()).toString())); + + IOException e = expectThrows(IOException.class, () -> dismax.visit(createTestVisitor())); + assertEquals("DisMax Query Builder Traversed", e.getMessage()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilderTests.java index 7e893a0947531..3d3447365e963 100644 --- a/server/src/test/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilderTests.java @@ -147,4 +147,9 @@ public void testDeprecatedName() throws IOException { "Deprecated field [field_masking_span] used, expected [" + SPAN_FIELD_MASKING_FIELD.getPreferredName() + "] instead" ); } + + public void testVisit() { + IOException e = expectThrows(IOException.class, () -> doCreateTestQueryBuilder().visit(createTestVisitor())); + assertEquals("Field Masking Query Builder Traversed", e.getMessage()); + } } diff --git a/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java b/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java index 0cfa64df55659..4c8963d6567eb 100644 --- a/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java @@ -35,6 +35,7 @@ import com.carrotsearch.randomizedtesting.RandomizedTest; import com.carrotsearch.randomizedtesting.SeedUtils; +import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.util.Accountable; import org.opensearch.Version; @@ -69,6 +70,12 @@ import org.opensearch.index.fielddata.IndexFieldDataService; import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.index.mapper.MapperService; +import org.opensearch.index.query.BoolQueryBuilder; +import org.opensearch.index.query.BoostingQueryBuilder; +import org.opensearch.index.query.DisMaxQueryBuilder; +import org.opensearch.index.query.FieldMaskingSpanQueryBuilder; +import org.opensearch.index.query.QueryBuilder; +import org.opensearch.index.query.QueryBuilderVisitor; import org.opensearch.index.query.QueryShardContext; import org.opensearch.index.similarity.SimilarityService; import org.opensearch.indices.IndicesModule; @@ -316,6 +323,28 @@ protected static QueryShardContext createShardContext() { return createShardContext(null); } + protected static QueryBuilderVisitor createTestVisitor() { + return new QueryBuilderVisitor() { + @Override + public void accept(QueryBuilder qb) throws IOException { + if (qb instanceof BoolQueryBuilder) { + throw new IOException("Bool Query Builder Traversed"); + } else if (qb instanceof BoostingQueryBuilder) { + throw new IOException("Boosting Query Builder Traversed"); + } else if (qb instanceof DisMaxQueryBuilder) { + throw new IOException("DisMax Query Builder Traversed"); + } else if (qb instanceof FieldMaskingSpanQueryBuilder) { + throw new IOException("Field Masking Query Builder Traversed"); + } + } + + @Override + public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { + return NO_OP_VISITOR; + } + }; + } + private static class ClientInvocationHandler implements InvocationHandler { AbstractBuilderTestCase delegate; From c6f6715a5e2c757795e52592eb57c14603154f6d Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Wed, 20 Sep 2023 13:45:32 -0700 Subject: [PATCH 6/9] Modifying test cases and remove exeception Signed-off-by: Varun Jain --- .../index/query/BoolQueryBuilder.java | 2 +- .../index/query/BoostingQueryBuilder.java | 2 +- .../query/ConstantScoreQueryBuilder.java | 2 +- .../index/query/DisMaxQueryBuilder.java | 2 +- .../query/FieldMaskingSpanQueryBuilder.java | 2 +- .../opensearch/index/query/QueryBuilder.java | 2 +- .../index/query/QueryBuilderVisitor.java | 4 +-- .../query/SpanContainingQueryBuilder.java | 2 +- .../index/query/SpanFirstQueryBuilder.java | 2 +- .../query/SpanMultiTermQueryBuilder.java | 2 +- .../index/query/SpanNearQueryBuilder.java | 2 +- .../index/query/SpanNotQueryBuilder.java | 2 +- .../index/query/SpanOrQueryBuilder.java | 2 +- .../index/query/SpanWithinQueryBuilder.java | 2 +- .../index/query/BoolQueryBuilderTests.java | 5 ++-- .../query/BoostingQueryBuilderTests.java | 8 +++-- .../index/query/DisMaxQueryBuilderTests.java | 7 +++-- .../FieldMaskingSpanQueryBuilderTests.java | 7 +++-- .../index/query/QueryBuilderVisitorTests.java | 29 +++++++++++++++++++ .../test/AbstractBuilderTestCase.java | 20 +++---------- 20 files changed, 66 insertions(+), 40 deletions(-) create mode 100644 server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java diff --git a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java index 9465da4ab5b1c..c44a7ef6a397c 100644 --- a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java @@ -429,7 +429,7 @@ private static boolean rewriteClauses( } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); if (mustClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST); diff --git a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java index c7dafc919e8a0..1b52ae2f03605 100644 --- a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java @@ -255,7 +255,7 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); if (positiveQuery != null) { visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery); diff --git a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java index e6c321e7bd40e..b2764d29da80a 100644 --- a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java @@ -186,7 +186,7 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.FILTER).accept(filterBuilder); } diff --git a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java index 6f5b3bcf75a9b..bd8ec62f6c43e 100644 --- a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java @@ -249,7 +249,7 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); if (queries.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); diff --git a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java index 576d86b818b1b..1162689a54689 100644 --- a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java @@ -210,7 +210,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(queryBuilder); } diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java index ca97227fc0be5..090f74c5be7fe 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java @@ -100,7 +100,7 @@ default QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOExc * Recurse through the QueryBuilder tree, visiting any child QueryBuilder. * @param visitor a query builder visitor to be called by each query builder in the tree. */ - default void visit(QueryBuilderVisitor visitor) throws IOException { + default void visit(QueryBuilderVisitor visitor) { visitor.accept(this); }; diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java index 97eb21cd9504b..af5a125f9dd95 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java @@ -10,8 +10,6 @@ import org.apache.lucene.search.BooleanClause; -import java.io.IOException; - /** * QueryBuilderVisitor is an interface to define Visitor Object to be traversed in QueryBuilder tree. */ @@ -21,7 +19,7 @@ public interface QueryBuilderVisitor { * Accept method is called when the visitor accepts the queryBuilder object to be traversed in the query tree. * @param qb is a queryBuilder object which is accepeted by the visitor. */ - void accept(QueryBuilder qb) throws IOException; + void accept(QueryBuilder qb); /** * Fetches the child sub visitor from the main QueryBuilderVisitor Object. diff --git a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java index 724fbf908ccfc..32a19ea3e9b50 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java @@ -191,7 +191,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); diff --git a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java index b79ae44e94d59..bcbc64ddf386d 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java @@ -189,7 +189,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(matchBuilder); } diff --git a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java index b69e2e9e7c518..96d03c91964e3 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java @@ -216,7 +216,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); if (multiTermQueryBuilder != null) { visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(multiTermQueryBuilder); diff --git a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java index 7d76608b7ba50..30a1c29c29126 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java @@ -301,7 +301,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); if (this.clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); diff --git a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java index 68bf80af3299e..59ec5b9d77fc8 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java @@ -287,7 +287,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); if (include != null) { visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(include); diff --git a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java index 543d5220109b1..fae1e318c66bd 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java @@ -191,7 +191,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); if (clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); diff --git a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java index 3bbedcec41950..4d5a6dde61a70 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java @@ -200,7 +200,7 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) throws IOException { + public void visit(QueryBuilderVisitor visitor) { visitor.accept(this); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); diff --git a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java index a84921b48c9e1..c47ace6f1697b 100644 --- a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java @@ -463,7 +463,8 @@ public void testVisit() { boolQueryBuilder.must(new BoolQueryBuilder()); boolQueryBuilder.mustNot(new BoolQueryBuilder()); boolQueryBuilder.filter(new BoolQueryBuilder()); - IOException e = expectThrows(IOException.class, () -> boolQueryBuilder.visit(QueryBuilderVisitor.NO_OP_VISITOR)); - assertEquals("Boosting Query Builder Traversed", e.getMessage()); + List visitedQueries = new ArrayList<>(); + boolQueryBuilder.visit(createTestVisitor(visitedQueries)); + assertEquals(5, visitedQueries.size()); } } diff --git a/server/src/test/java/org/opensearch/index/query/BoostingQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/BoostingQueryBuilderTests.java index 935fe28b6aaf0..66a02a02d4e5b 100644 --- a/server/src/test/java/org/opensearch/index/query/BoostingQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/BoostingQueryBuilderTests.java @@ -38,6 +38,8 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.nullValue; @@ -160,7 +162,9 @@ public void testVisit() { new TermQueryBuilder(KEYWORD_FIELD_NAME, "other_value") ); - IOException e = expectThrows(IOException.class, () -> builder.visit(createTestVisitor())); - assertEquals("Boosting Query Builder Traversed", e.getMessage()); + List visitedQueries = new ArrayList<>(); + builder.visit(createTestVisitor(visitedQueries)); + + assertEquals(3, visitedQueries.size()); } } diff --git a/server/src/test/java/org/opensearch/index/query/DisMaxQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/DisMaxQueryBuilderTests.java index dc029e8104b2a..cb0df38de5c02 100644 --- a/server/src/test/java/org/opensearch/index/query/DisMaxQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/DisMaxQueryBuilderTests.java @@ -41,6 +41,7 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.List; @@ -161,7 +162,9 @@ public void testVisit() { DisMaxQueryBuilder dismax = new DisMaxQueryBuilder(); dismax.add(new WrapperQueryBuilder(new WrapperQueryBuilder(new MatchAllQueryBuilder().toString()).toString())); - IOException e = expectThrows(IOException.class, () -> dismax.visit(createTestVisitor())); - assertEquals("DisMax Query Builder Traversed", e.getMessage()); + List visitedQueries = new ArrayList<>(); + dismax.visit(createTestVisitor(visitedQueries)); + + assertEquals(2, visitedQueries.size()); } } diff --git a/server/src/test/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilderTests.java index 3d3447365e963..db0a7bf1795ff 100644 --- a/server/src/test/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilderTests.java @@ -41,6 +41,8 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import static org.opensearch.index.query.FieldMaskingSpanQueryBuilder.SPAN_FIELD_MASKING_FIELD; import static org.hamcrest.CoreMatchers.equalTo; @@ -149,7 +151,8 @@ public void testDeprecatedName() throws IOException { } public void testVisit() { - IOException e = expectThrows(IOException.class, () -> doCreateTestQueryBuilder().visit(createTestVisitor())); - assertEquals("Field Masking Query Builder Traversed", e.getMessage()); + List visitedQueries = new ArrayList<>(); + doCreateTestQueryBuilder().visit(createTestVisitor(visitedQueries)); + assertEquals(2, visitedQueries.size()); } } diff --git a/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java b/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java new file mode 100644 index 0000000000000..4bca53fbe23bc --- /dev/null +++ b/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java @@ -0,0 +1,29 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.query; + +import org.opensearch.test.AbstractBuilderTestCase; + +import java.util.ArrayList; +import java.util.List; + +public class QueryBuilderVisitorTests extends AbstractBuilderTestCase { + + public void testNoOpsVisitor() { + BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder(); + + List visitedQueries = new ArrayList<>(); + boolQueryBuilder.visit(createTestVisitor(visitedQueries)); + assertEquals(0, visitedQueries.size()); + } + + protected static QueryBuilderVisitor createTestVisitor(List visitedQueries) { + return QueryBuilderVisitor.NO_OP_VISITOR; + } +} diff --git a/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java b/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java index 4c8963d6567eb..7a8cf4963c4f1 100644 --- a/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java @@ -70,10 +70,6 @@ import org.opensearch.index.fielddata.IndexFieldDataService; import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.index.mapper.MapperService; -import org.opensearch.index.query.BoolQueryBuilder; -import org.opensearch.index.query.BoostingQueryBuilder; -import org.opensearch.index.query.DisMaxQueryBuilder; -import org.opensearch.index.query.FieldMaskingSpanQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.QueryBuilderVisitor; import org.opensearch.index.query.QueryShardContext; @@ -323,24 +319,16 @@ protected static QueryShardContext createShardContext() { return createShardContext(null); } - protected static QueryBuilderVisitor createTestVisitor() { + protected static QueryBuilderVisitor createTestVisitor(List visitedQueries) { return new QueryBuilderVisitor() { @Override - public void accept(QueryBuilder qb) throws IOException { - if (qb instanceof BoolQueryBuilder) { - throw new IOException("Bool Query Builder Traversed"); - } else if (qb instanceof BoostingQueryBuilder) { - throw new IOException("Boosting Query Builder Traversed"); - } else if (qb instanceof DisMaxQueryBuilder) { - throw new IOException("DisMax Query Builder Traversed"); - } else if (qb instanceof FieldMaskingSpanQueryBuilder) { - throw new IOException("Field Masking Query Builder Traversed"); - } + public void accept(QueryBuilder qb) { + visitedQueries.add(qb); } @Override public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { - return NO_OP_VISITOR; + return this; } }; } From 0725d5c04b83fa66dc3d212c87578b81fe9d1b6f Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Wed, 20 Sep 2023 14:05:49 -0700 Subject: [PATCH 7/9] Adding bool query builder test cases at clause level Signed-off-by: Varun Jain --- .../index/query/BoolQueryBuilderTests.java | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java index c47ace6f1697b..9c4ea797377c5 100644 --- a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java @@ -47,10 +47,13 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; import static org.opensearch.index.query.QueryBuilders.boolQuery; import static org.opensearch.index.query.QueryBuilders.termQuery; @@ -459,12 +462,23 @@ public void testMustRewrite() throws IOException { public void testVisit() { BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder(); - boolQueryBuilder.should(new BoolQueryBuilder()); - boolQueryBuilder.must(new BoolQueryBuilder()); - boolQueryBuilder.mustNot(new BoolQueryBuilder()); - boolQueryBuilder.filter(new BoolQueryBuilder()); + boolQueryBuilder.should(new TermQueryBuilder(TEXT_FIELD_NAME, "should")); + boolQueryBuilder.must(new TermQueryBuilder(TEXT_FIELD_NAME, "must1")); + boolQueryBuilder.must(new TermQueryBuilder(TEXT_FIELD_NAME, "must2")); // Add a second one to confirm that they both get visited + boolQueryBuilder.mustNot(new TermQueryBuilder(TEXT_FIELD_NAME, "mustNot")); + boolQueryBuilder.filter(new TermQueryBuilder(TEXT_FIELD_NAME, "filter")); List visitedQueries = new ArrayList<>(); boolQueryBuilder.visit(createTestVisitor(visitedQueries)); assertEquals(5, visitedQueries.size()); + Set set = new HashSet<>(Arrays.asList("should", "must1", "must2", "mustNot", "filter")); + + for (QueryBuilder qb : visitedQueries) { + if (qb instanceof TermQueryBuilder) { + set.remove(((TermQueryBuilder) qb).value()); + } + } + + assertEquals(0, set.size()); + } } From 3b6d79ada9d25f8d59db529815c78127f75a76c9 Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Wed, 20 Sep 2023 15:50:41 -0700 Subject: [PATCH 8/9] Fixing test case Signed-off-by: Varun Jain --- .../java/org/opensearch/index/query/BoolQueryBuilderTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java index 9c4ea797377c5..d0f26f3026789 100644 --- a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java @@ -469,7 +469,7 @@ public void testVisit() { boolQueryBuilder.filter(new TermQueryBuilder(TEXT_FIELD_NAME, "filter")); List visitedQueries = new ArrayList<>(); boolQueryBuilder.visit(createTestVisitor(visitedQueries)); - assertEquals(5, visitedQueries.size()); + assertEquals(6, visitedQueries.size()); Set set = new HashSet<>(Arrays.asList("should", "must1", "must2", "mustNot", "filter")); for (QueryBuilder qb : visitedQueries) { From e9c81e17d3bf88b7f262660c221d0b95ec1e88c3 Mon Sep 17 00:00:00 2001 From: Varun Jain Date: Wed, 20 Sep 2023 18:16:09 -0700 Subject: [PATCH 9/9] Additional test cases Signed-off-by: Varun Jain --- .../index/query/ConstantScoreQueryBuilderTests.java | 10 ++++++++++ .../index/query/QueryBuilderVisitorTests.java | 6 +++++- .../index/query/SpanContainingQueryBuilderTests.java | 8 ++++++++ .../index/query/SpanFirstQueryBuilderTests.java | 8 ++++++++ .../index/query/SpanMultiTermQueryBuilderTests.java | 10 ++++++++++ .../index/query/SpanNearQueryBuilderTests.java | 10 ++++++++++ .../index/query/SpanNotQueryBuilderTests.java | 8 ++++++++ .../index/query/SpanOrQueryBuilderTests.java | 10 ++++++++++ .../index/query/SpanWithinQueryBuilderTests.java | 8 ++++++++ 9 files changed, 77 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/index/query/ConstantScoreQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/ConstantScoreQueryBuilderTests.java index 2bfe964ce7259..527413d2513d0 100644 --- a/server/src/test/java/org/opensearch/index/query/ConstantScoreQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/ConstantScoreQueryBuilderTests.java @@ -39,6 +39,8 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.nullValue; @@ -133,4 +135,12 @@ public void testMustRewrite() throws IOException { IllegalStateException e = expectThrows(IllegalStateException.class, () -> queryBuilder.toQuery(context)); assertEquals("Rewrite first", e.getMessage()); } + + public void testVisit() { + ConstantScoreQueryBuilder queryBuilder = new ConstantScoreQueryBuilder(new TermQueryBuilder("unmapped_field", "foo")); + List visitorQueries = new ArrayList<>(); + queryBuilder.visit(createTestVisitor(visitorQueries)); + + assertEquals(2, visitorQueries.size()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java b/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java index 4bca53fbe23bc..7849d3985ca59 100644 --- a/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java +++ b/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java @@ -8,6 +8,7 @@ package org.opensearch.index.query; +import org.apache.lucene.search.BooleanClause; import org.opensearch.test.AbstractBuilderTestCase; import java.util.ArrayList; @@ -19,8 +20,11 @@ public void testNoOpsVisitor() { BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder(); List visitedQueries = new ArrayList<>(); - boolQueryBuilder.visit(createTestVisitor(visitedQueries)); + QueryBuilderVisitor qbv = createTestVisitor(visitedQueries); + boolQueryBuilder.visit(qbv); + QueryBuilderVisitor subQbv = qbv.getChildVisitor(BooleanClause.Occur.MUST_NOT); assertEquals(0, visitedQueries.size()); + assertEquals(qbv, subQbv); } protected static QueryBuilderVisitor createTestVisitor(List visitedQueries) { diff --git a/server/src/test/java/org/opensearch/index/query/SpanContainingQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/SpanContainingQueryBuilderTests.java index fff4369e155b3..2a5441b0ea932 100644 --- a/server/src/test/java/org/opensearch/index/query/SpanContainingQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/SpanContainingQueryBuilderTests.java @@ -38,6 +38,8 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; @@ -192,4 +194,10 @@ public void testFromJsonWithNonDefaultBoostInLittleQuery() { equalTo("span_containing [little] as a nested span clause can't have non-default boost value [2.0]") ); } + + public void testVisit() { + List visitorQueries = new ArrayList<>(); + doCreateTestQueryBuilder().visit(createTestVisitor(visitorQueries)); + assertEquals(3, visitorQueries.size()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/SpanFirstQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/SpanFirstQueryBuilderTests.java index 781d2defcfd37..82bb77726b037 100644 --- a/server/src/test/java/org/opensearch/index/query/SpanFirstQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/SpanFirstQueryBuilderTests.java @@ -40,6 +40,8 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import static org.opensearch.index.query.QueryBuilders.spanTermQuery; import static org.hamcrest.CoreMatchers.equalTo; @@ -128,4 +130,10 @@ public void testFromJsonWithNonDefaultBoostInMatchQuery() { Exception exception = expectThrows(ParsingException.class, () -> parseQuery(json)); assertThat(exception.getMessage(), equalTo("span_first [match] as a nested span clause can't have non-default boost value [2.0]")); } + + public void testVisit() { + List visitorQueries = new ArrayList<>(); + doCreateTestQueryBuilder().visit(createTestVisitor(visitorQueries)); + assertEquals(2, visitorQueries.size()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/SpanMultiTermQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/SpanMultiTermQueryBuilderTests.java index e20d56c6aff2a..b4abff118802e 100644 --- a/server/src/test/java/org/opensearch/index/query/SpanMultiTermQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/SpanMultiTermQueryBuilderTests.java @@ -60,6 +60,8 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import static java.util.Collections.singleton; import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; @@ -306,4 +308,12 @@ public void testTopNMultiTermsRewriteInsideSpan() throws Exception { } } + + public void testVisit() { + MultiTermQueryBuilder multiTermQueryBuilder = new PrefixQueryBuilderTests().createTestQueryBuilder(); + SpanMultiTermQueryBuilder spanMultiTermQueryBuilder = new SpanMultiTermQueryBuilder(multiTermQueryBuilder); + List visitorQueries = new ArrayList<>(); + spanMultiTermQueryBuilder.visit(createTestVisitor(visitorQueries)); + assertEquals(2, visitorQueries.size()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/SpanNearQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/SpanNearQueryBuilderTests.java index 8c9130d4b7bbd..c97b541da2199 100644 --- a/server/src/test/java/org/opensearch/index/query/SpanNearQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/SpanNearQueryBuilderTests.java @@ -41,7 +41,9 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; import java.util.Iterator; +import java.util.List; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.either; @@ -231,4 +233,12 @@ public void testFromJsonWithNonDefaultBoostInInnerQuery() { Exception exception = expectThrows(ParsingException.class, () -> parseQuery(json)); assertThat(exception.getMessage(), equalTo("span_near [clauses] as a nested span clause can't have non-default boost value [2.0]")); } + + public void testVisit() { + SpanTermQueryBuilder[] spanTermQueries = new SpanTermQueryBuilderTests().createSpanTermQueryBuilders(1); + SpanNearQueryBuilder spanNearQueryBuilder = new SpanNearQueryBuilder(spanTermQueries[0], 1); + List visitorQueries = new ArrayList<>(); + spanNearQueryBuilder.visit(createTestVisitor(visitorQueries)); + assertEquals(2, visitorQueries.size()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/SpanNotQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/SpanNotQueryBuilderTests.java index b85c7cf81b0e1..ff42b271ffca5 100644 --- a/server/src/test/java/org/opensearch/index/query/SpanNotQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/SpanNotQueryBuilderTests.java @@ -40,6 +40,8 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import static org.opensearch.index.query.QueryBuilders.spanNearQuery; import static org.opensearch.index.query.QueryBuilders.spanTermQuery; @@ -316,4 +318,10 @@ public void testFromJsonWithNonDefaultBoostInExcludeQuery() { Exception exception = expectThrows(ParsingException.class, () -> parseQuery(json)); assertThat(exception.getMessage(), equalTo("span_not [exclude] as a nested span clause can't have non-default boost value [2.0]")); } + + public void testVisit() { + List visitedQueries = new ArrayList<>(); + doCreateTestQueryBuilder().visit(createTestVisitor(visitedQueries)); + assertEquals(3, visitedQueries.size()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/SpanOrQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/SpanOrQueryBuilderTests.java index 45323b5df74df..eb4b8fd486cb0 100644 --- a/server/src/test/java/org/opensearch/index/query/SpanOrQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/SpanOrQueryBuilderTests.java @@ -39,7 +39,9 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; import java.util.Iterator; +import java.util.List; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; @@ -137,4 +139,12 @@ public void testFromJsonWithNonDefaultBoostInInnerQuery() { Exception exception = expectThrows(ParsingException.class, () -> parseQuery(json)); assertThat(exception.getMessage(), equalTo("span_or [clauses] as a nested span clause can't have non-default boost value [2.0]")); } + + public void testVisit() { + SpanTermQueryBuilder spanTermQueryBuilder = new SpanTermQueryBuilder("demo", "demo"); + SpanOrQueryBuilder spanOrQueryBuilder = new SpanOrQueryBuilder(spanTermQueryBuilder); + List visitedQueries = new ArrayList<>(); + spanOrQueryBuilder.visit(createTestVisitor(visitedQueries)); + assertEquals(2, visitedQueries.size()); + } } diff --git a/server/src/test/java/org/opensearch/index/query/SpanWithinQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/SpanWithinQueryBuilderTests.java index e8b6a21254ff8..74b955e872d51 100644 --- a/server/src/test/java/org/opensearch/index/query/SpanWithinQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/SpanWithinQueryBuilderTests.java @@ -38,6 +38,8 @@ import org.opensearch.test.AbstractQueryTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; @@ -189,4 +191,10 @@ public void testFromJsonWithNonDefaultBoostInLittleQuery() { equalTo("span_within [little] as a nested span clause can't have non-default boost value [2.0]") ); } + + public void testVisit() { + List visitedQueries = new ArrayList<>(); + doCreateTestQueryBuilder().visit(createTestVisitor(visitedQueries)); + assertEquals(3, visitedQueries.size()); + } }