From 1097d9f2fa5283dfa412bd008459a163dfddea29 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Thu, 30 Jun 2022 19:56:31 -0700 Subject: [PATCH 01/24] Add support for highlight to parser and AstExpressionBuilder Signed-off-by: MaxKsyunz --- .../sql/ast/expression/HighlightFunction.java | 7 +++++++ sql/src/main/antlr/OpenSearchSQLLexer.g4 | 2 +- sql/src/main/antlr/OpenSearchSQLParser.g4 | 5 +++++ .../sql/sql/parser/AstExpressionBuilder.java | 7 +++++++ .../org/opensearch/sql/sql/antlr/HighlightTest.java | 10 ++++++++++ 5 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java create mode 100644 sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java new file mode 100644 index 0000000000..2ed5e8412c --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java @@ -0,0 +1,7 @@ +package org.opensearch.sql.ast.expression; + +public class HighlightFunction extends UnresolvedExpression { + public HighlightFunction(String field) { + + } +} diff --git a/sql/src/main/antlr/OpenSearchSQLLexer.g4 b/sql/src/main/antlr/OpenSearchSQLLexer.g4 index 8b74e31aac..8c4a092e62 100644 --- a/sql/src/main/antlr/OpenSearchSQLLexer.g4 +++ b/sql/src/main/antlr/OpenSearchSQLLexer.g4 @@ -346,7 +346,7 @@ TIE_BREAKER: 'TIE_BREAKER'; TIME_ZONE: 'TIME_ZONE'; TYPE: 'TYPE'; ZERO_TERMS_QUERY: 'ZERO_TERMS_QUERY'; - +HIGHLIGHT: 'HIGHLIGHT'; // RELEVANCE FUNCTIONS MATCH_BOOL_PREFIX: 'MATCH_BOOL_PREFIX'; // Operators diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index d75893b696..a9316b55a4 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -300,6 +300,11 @@ functionCall | aggregateFunction #aggregateFunctionCall | aggregateFunction (orderByClause)? filterClause #filteredAggregationFunctionCall | relevanceFunction #relevanceFunctionCall + | highlightFunction #highlightFunctionCall + ; + +highlightFunction + : HIGHLIGHT LR_BRACKET relevanceField RR_BRACKET ; scalarFunctionName diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index 3c686b5e8e..78523d289e 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -63,6 +63,7 @@ import org.opensearch.sql.ast.expression.Cast; import org.opensearch.sql.ast.expression.DataType; import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.HighlightFunction; import org.opensearch.sql.ast.expression.Interval; import org.opensearch.sql.ast.expression.IntervalUnit; import org.opensearch.sql.ast.expression.Literal; @@ -130,6 +131,12 @@ public UnresolvedExpression visitScalarFunctionCall(ScalarFunctionCallContext ct return visitFunction(ctx.scalarFunctionName().getText(), ctx.functionArgs()); } + @Override + public UnresolvedExpression visitHighlightFunctionCall( + OpenSearchSQLParser.HighlightFunctionCallContext ctx) { + return new HighlightFunction(ctx.highlightFunction().relevanceField().getText()); + } + @Override public UnresolvedExpression visitTableFilter(TableFilterContext ctx) { return new Function( diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java new file mode 100644 index 0000000000..558cb58e37 --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java @@ -0,0 +1,10 @@ +package org.opensearch.sql.sql.antlr; + +import org.junit.jupiter.api.Test; + +public class HighlightTest extends SQLParserTest { + @Test + void single_field() { + acceptQuery("SELECT HIGHLIGHT(Tags) FROM Index WHERE MATCH(Tags, 'Time')"); + } +} From 1a453e18e763d68826e2e619ea1d8867bb57134d Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Mon, 4 Jul 2022 16:03:43 -0700 Subject: [PATCH 02/24] Add unit test for highlight in AstExpressionBuilder Signed-off-by: MaxKsyunz --- .../src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java | 5 +++++ .../sql/sql/parser/AstExpressionBuilderTest.java | 9 +++++++++ 2 files changed, 14 insertions(+) diff --git a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java index 4056347d44..285948ee04 100644 --- a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java +++ b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java @@ -22,6 +22,7 @@ import org.opensearch.sql.ast.expression.EqualTo; import org.opensearch.sql.ast.expression.Field; import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.HighlightFunction; import org.opensearch.sql.ast.expression.In; import org.opensearch.sql.ast.expression.Interval; import org.opensearch.sql.ast.expression.Let; @@ -261,6 +262,10 @@ public When when(UnresolvedExpression condition, UnresolvedExpression result) { return new When(condition, result); } + public UnresolvedExpression highlight(String fieldName) { + return new HighlightFunction(fieldName); + } + public UnresolvedExpression window(UnresolvedExpression function, List partitionByList, List> sortList) { diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 9ce3dd6714..78f9a26981 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -15,6 +15,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.doubleLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.floatLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.function; +import static org.opensearch.sql.ast.dsl.AstDSL.highlight; import static org.opensearch.sql.ast.dsl.AstDSL.intLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.intervalLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.longLiteral; @@ -308,6 +309,14 @@ public void canBuildWindowFunctionWithNullOrderSpecified() { buildExprAst("DENSE_RANK() OVER (ORDER BY age ASC NULLS LAST)")); } + @Test + public void canBuildHighlighFunction() { + assertEquals( + highlight("fieldA"), + buildExprAst("highlight(fieldA)") + ); + } + @Test public void canBuildWindowFunctionWithoutOrderBy() { assertEquals( From 0d5c87bff4d59db6530334ad3adaf4e47d7068fa Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Mon, 4 Jul 2022 23:50:18 -0700 Subject: [PATCH 03/24] Add unit test for highlight in AstBuilderTest Signed-off-by: MaxKsyunz --- .../org/opensearch/sql/sql/parser/AstBuilderTest.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java index d576389595..72542aaf9e 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java @@ -18,6 +18,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.field; import static org.opensearch.sql.ast.dsl.AstDSL.filter; import static org.opensearch.sql.ast.dsl.AstDSL.function; +import static org.opensearch.sql.ast.dsl.AstDSL.highlight; import static org.opensearch.sql.ast.dsl.AstDSL.intLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.limit; import static org.opensearch.sql.ast.dsl.AstDSL.project; @@ -667,6 +668,15 @@ public void can_build_limit_clause_with_offset() { buildAST("SELECT name FROM test LIMIT 5, 10")); } + @Test + public void can_build_highlight() { + assertEquals( + project(relation("test"), + alias("highlight(fieldA)", highlight("fieldA"))), + buildAST("SELECT highlight(fieldA) FROM test") + ); + } + private UnresolvedPlan buildAST(String query) { ParseTree parseTree = parser.parse(query); return parseTree.accept(new AstBuilder(query)); From 26d0b7e96db1c440d732bc3d40d79d6e485ae2f7 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Tue, 5 Jul 2022 11:00:19 -0700 Subject: [PATCH 04/24] Support highlight as an Unresolved expression. Signed-off-by: MaxKsyunz --- .../java/org/opensearch/sql/ast/AbstractNodeVisitor.java | 5 +++++ .../opensearch/sql/sql/parser/AstExpressionBuilderTest.java | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java b/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java index 37163821e3..17321bc473 100644 --- a/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java @@ -18,6 +18,7 @@ import org.opensearch.sql.ast.expression.EqualTo; import org.opensearch.sql.ast.expression.Field; import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.HighlightFunction; import org.opensearch.sql.ast.expression.In; import org.opensearch.sql.ast.expression.Interval; import org.opensearch.sql.ast.expression.Let; @@ -254,4 +255,8 @@ public T visitKmeans(Kmeans node, C context) { public T visitAD(AD node, C context) { return visitChildren(node, context); } + + public T visitHighlight(HighlightFunction node, C context) { + return visitChildren(node, context); + } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 78f9a26981..fa350b5f79 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -312,8 +312,8 @@ public void canBuildWindowFunctionWithNullOrderSpecified() { @Test public void canBuildHighlighFunction() { assertEquals( - highlight("fieldA"), - buildExprAst("highlight(fieldA)") + highlight("fieldA"), + buildExprAst("highlight(fieldA)") ); } From 3f10b8b50acc58a80bbf3d0475837dd9bc19950c Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Tue, 5 Jul 2022 11:00:59 -0700 Subject: [PATCH 05/24] Represent highlight as UnresolvedExpression. Signed-off-by: MaxKsyunz --- .../sql/ast/expression/HighlightFunction.java | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java index 2ed5e8412c..4bf0da2374 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java @@ -1,7 +1,24 @@ package org.opensearch.sql.ast.expression; +import lombok.AllArgsConstructor; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import lombok.ToString; +import org.opensearch.sql.ast.AbstractNodeVisitor; + +@AllArgsConstructor +@EqualsAndHashCode(callSuper = false) +@Getter +@RequiredArgsConstructor +@ToString public class HighlightFunction extends UnresolvedExpression { - public HighlightFunction(String field) { + @Getter + private final String highlightField; + + @Override + public T accept(AbstractNodeVisitor nodeVisitor, C context) { + return nodeVisitor.visitHighlight(this, context); } } From 543d0d739dbe18a54a66717a6aefaa6ac251f3bc Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Tue, 5 Jul 2022 11:01:27 -0700 Subject: [PATCH 06/24] Support highlight in Analyzer. Signed-off-by: MaxKsyunz --- .../sql/analysis/ExpressionAnalyzer.java | 10 +++-- .../sql/analysis/HighlightExpression.java | 38 +++++++++++++++++++ .../sql/expression/ExpressionNodeVisitor.java | 4 ++ .../opensearch/sql/analysis/AnalyzerTest.java | 13 +++++++ .../sql/analysis/ExpressionAnalyzerTest.java | 7 +++- 5 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java index 91b964d074..ce1d4be614 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java @@ -11,9 +11,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.List; -import java.util.Map; import java.util.Optional; import java.util.stream.Collectors; import lombok.Getter; @@ -29,6 +27,7 @@ import org.opensearch.sql.ast.expression.EqualTo; import org.opensearch.sql.ast.expression.Field; import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.HighlightFunction; import org.opensearch.sql.ast.expression.In; import org.opensearch.sql.ast.expression.Interval; import org.opensearch.sql.ast.expression.Literal; @@ -44,7 +43,6 @@ import org.opensearch.sql.ast.expression.WindowFunction; import org.opensearch.sql.ast.expression.Xor; import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.exception.SemanticCheckException; @@ -191,6 +189,12 @@ public Expression visitWindowFunction(WindowFunction node, AnalysisContext conte return expr; } + @Override + public Expression visitHighlight(HighlightFunction node, AnalysisContext context) { + + return new HighlightExpression(node.getHighlightField()); + } + @Override public Expression visitIn(In node, AnalysisContext context) { return visitIn(node.getField(), node.getValueList(), context); diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java new file mode 100644 index 0000000000..e8e840fa12 --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java @@ -0,0 +1,38 @@ +package org.opensearch.sql.analysis; + +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import org.opensearch.sql.data.model.ExprValue; +import org.opensearch.sql.data.type.ExprCoreType; +import org.opensearch.sql.data.type.ExprType; +import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.expression.env.Environment; + +@EqualsAndHashCode(callSuper = false) +@Getter +@ToString +public class HighlightExpression implements Expression { + String highlightField; + + public HighlightExpression(String field) { + highlightField = field; + } + + @Override + public ExprValue valueOf(Environment valueEnv) { + throw new SemanticCheckException("valeOf highlight is not supported"); + } + + @Override + public ExprType type() { + return ExprCoreType.STRING; + } + + @Override + public T accept(org.opensearch.sql.expression.ExpressionNodeVisitor visitor, + C context) { + return visitor.visitHighlight(this, context); + } +} diff --git a/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java b/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java index b05b0924a8..69a7482452 100644 --- a/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java @@ -6,6 +6,7 @@ package org.opensearch.sql.expression; +import org.opensearch.sql.analysis.HighlightExpression; import org.opensearch.sql.expression.aggregation.Aggregator; import org.opensearch.sql.expression.aggregation.NamedAggregator; import org.opensearch.sql.expression.conditional.cases.CaseClause; @@ -55,6 +56,9 @@ public T visitNamed(NamedExpression node, C context) { return node.getDelegated().accept(this, context); } + public T visitHighlight(HighlightExpression node, C context) { + return visitNode(node, context); + } public T visitReference(ReferenceExpression node, C context) { return visitNode(node, context); } diff --git a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java index 797c5ab11a..110e48baaf 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java @@ -45,6 +45,7 @@ import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.expression.Argument; import org.opensearch.sql.ast.expression.DataType; +import org.opensearch.sql.ast.expression.HighlightFunction; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.SpanUnit; import org.opensearch.sql.ast.tree.AD; @@ -231,6 +232,18 @@ public void project_source() { AstDSL.alias("double_value", AstDSL.field("double_value")))); } + @Test + public void project_highlight() { + assertAnalyzeEqual( + LogicalPlanDSL.project(LogicalPlanDSL.relation("schema"), + DSL.named("highlight(fieldA)", new HighlightExpression("fieldA"))), + AstDSL.projectWithArg( + AstDSL.relation("schema"), + AstDSL.defaultFieldsArgs(), + AstDSL.alias("highlight(fieldA)", new HighlightFunction("fieldA")) + ) + ); + } @Test public void remove_source() { assertAnalyzeEqual( diff --git a/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java index dd0e199cf4..73aabcc3eb 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java @@ -10,7 +10,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.opensearch.sql.ast.dsl.AstDSL.field; -import static org.opensearch.sql.ast.dsl.AstDSL.floatLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.function; import static org.opensearch.sql.ast.dsl.AstDSL.intLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; @@ -35,6 +34,7 @@ import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.expression.AllFields; import org.opensearch.sql.ast.expression.DataType; +import org.opensearch.sql.ast.expression.HighlightFunction; import org.opensearch.sql.ast.expression.RelevanceFieldList; import org.opensearch.sql.ast.expression.SpanUnit; import org.opensearch.sql.ast.expression.UnresolvedExpression; @@ -536,6 +536,11 @@ public void match_phrase_prefix_all_params() { ); } + @Test + void highlight() { + assertAnalyzeEqual(new HighlightExpression("fieldA"), new HighlightFunction("fieldA")); + } + protected Expression analyze(UnresolvedExpression unresolvedExpression) { return expressionAnalyzer.analyze(unresolvedExpression, analysisContext); } From f47ffe7347405463b79b4c9e4564da203b38e1c4 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Wed, 6 Jul 2022 00:46:13 -0700 Subject: [PATCH 07/24] Treat highlight as a proper function in AST In particular, highlightField is an expression now. Signed-off-by: MaxKsyunz --- .../main/java/org/opensearch/sql/ast/dsl/AstDSL.java | 2 +- .../sql/ast/expression/HighlightFunction.java | 12 ++++++++---- .../sql/expression/function/BuiltinFunctionName.java | 1 + .../sql/sql/parser/AstExpressionBuilder.java | 2 +- .../opensearch/sql/sql/parser/AstBuilderTest.java | 7 ++++--- .../sql/sql/parser/AstExpressionBuilderTest.java | 2 +- 6 files changed, 16 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java index 285948ee04..510482c645 100644 --- a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java +++ b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java @@ -262,7 +262,7 @@ public When when(UnresolvedExpression condition, UnresolvedExpression result) { return new When(condition, result); } - public UnresolvedExpression highlight(String fieldName) { + public UnresolvedExpression highlight(UnresolvedExpression fieldName) { return new HighlightFunction(fieldName); } diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java index 4bf0da2374..71a4bfe4ec 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java @@ -1,5 +1,7 @@ package org.opensearch.sql.ast.expression; +import com.google.common.collect.ImmutableList; +import java.util.List; import lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; import lombok.Getter; @@ -10,15 +12,17 @@ @AllArgsConstructor @EqualsAndHashCode(callSuper = false) @Getter -@RequiredArgsConstructor @ToString public class HighlightFunction extends UnresolvedExpression { - @Getter - private final String highlightField; - + private final UnresolvedExpression highlightField; @Override public T accept(AbstractNodeVisitor nodeVisitor, C context) { return nodeVisitor.visitHighlight(this, context); } + + @Override + public List getChild() { + return List.of(highlightField); + } } diff --git a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java index 5eb9e715b8..cd34328453 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java @@ -192,6 +192,7 @@ public enum BuiltinFunctionName { MATCHPHRASE(FunctionName.of("matchphrase")), QUERY_STRING(FunctionName.of("query_string")), MATCH_BOOL_PREFIX(FunctionName.of("match_bool_prefix")), + HIGHLIGHT(FunctionName.of("highlight")), MATCH_PHRASE_PREFIX(FunctionName.of("match_phrase_prefix")), /** * Legacy Relevance Function. diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index 78523d289e..453162e335 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -134,7 +134,7 @@ public UnresolvedExpression visitScalarFunctionCall(ScalarFunctionCallContext ct @Override public UnresolvedExpression visitHighlightFunctionCall( OpenSearchSQLParser.HighlightFunctionCallContext ctx) { - return new HighlightFunction(ctx.highlightFunction().relevanceField().getText()); + return new HighlightFunction(visit(ctx.highlightFunction().relevanceField())); } @Override diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java index 72542aaf9e..a2a27bb2df 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java @@ -34,6 +34,7 @@ import com.google.common.collect.ImmutableList; import org.antlr.v4.runtime.tree.ParseTree; import org.junit.jupiter.api.Test; +import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.expression.AllFields; import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.common.antlr.SyntaxCheckException; @@ -671,9 +672,9 @@ public void can_build_limit_clause_with_offset() { @Test public void can_build_highlight() { assertEquals( - project(relation("test"), - alias("highlight(fieldA)", highlight("fieldA"))), - buildAST("SELECT highlight(fieldA) FROM test") + project(relation("test"), + alias("highlight(fieldA)", highlight(AstDSL.stringLiteral("fieldA")))), + buildAST("SELECT highlight(fieldA) FROM test") ); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index fa350b5f79..8cb782356b 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -312,7 +312,7 @@ public void canBuildWindowFunctionWithNullOrderSpecified() { @Test public void canBuildHighlighFunction() { assertEquals( - highlight("fieldA"), + highlight(AstDSL.stringLiteral("fieldA")), buildExprAst("highlight(fieldA)") ); } From 5fdb939b55ccbcb9b9f38f1f8be069ba358c43c2 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Wed, 6 Jul 2022 00:48:44 -0700 Subject: [PATCH 08/24] Add support for highlight in Analyzer HighlightFunction is converted to LogicalHighlight logical plan. Signed-off-by: MaxKsyunz --- .../org/opensearch/sql/analysis/Analyzer.java | 5 +++ .../sql/analysis/ExpressionAnalyzer.java | 4 +-- .../sql/analysis/HighlightAnalyzer.java | 33 +++++++++++++++++++ .../sql/analysis/HighlightExpression.java | 24 +++++++++----- .../sql/expression/ExpressionNodeVisitor.java | 1 + .../sql/planner/logical/LogicalHighlight.java | 24 ++++++++++++++ .../sql/planner/logical/LogicalPlanDSL.java | 4 +++ .../logical/LogicalPlanNodeVisitor.java | 4 +++ .../opensearch/sql/analysis/AnalyzerTest.java | 15 ++++++--- .../sql/analysis/ExpressionAnalyzerTest.java | 4 ++- .../logical/LogicalPlanNodeVisitorTest.java | 8 ++++- 11 files changed, 108 insertions(+), 18 deletions(-) create mode 100644 core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java create mode 100644 core/src/main/java/org/opensearch/sql/planner/logical/LogicalHighlight.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java index 9054f80e93..4ac2a3a797 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java @@ -292,6 +292,11 @@ public LogicalPlan visitProject(Project node, AnalysisContext context) { child = windowAnalyzer.analyze(expr, context); } + for (UnresolvedExpression expr : node.getProjectList()) { + HighlightAnalyzer highlightAnalyzer = new HighlightAnalyzer(expressionAnalyzer, child); + child = highlightAnalyzer.analyze(expr, context); + } + List namedExpressions = selectExpressionAnalyzer.analyze(node.getProjectList(), context, new ExpressionReferenceOptimizer(expressionAnalyzer.getRepository(), child)); diff --git a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java index ce1d4be614..f97b0803a9 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java @@ -191,8 +191,8 @@ public Expression visitWindowFunction(WindowFunction node, AnalysisContext conte @Override public Expression visitHighlight(HighlightFunction node, AnalysisContext context) { - - return new HighlightExpression(node.getHighlightField()); + Expression expr = node.getHighlightField().accept(this, context); + return new HighlightExpression(expr); } @Override diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java new file mode 100644 index 0000000000..2005046e4a --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java @@ -0,0 +1,33 @@ +package org.opensearch.sql.analysis; + +import lombok.RequiredArgsConstructor; +import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.HighlightFunction; +import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.ast.expression.WindowFunction; +import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.planner.logical.LogicalHighlight; +import org.opensearch.sql.planner.logical.LogicalPlan; + +@RequiredArgsConstructor +public class HighlightAnalyzer extends AbstractNodeVisitor { + private final ExpressionAnalyzer expressionAnalyzer; + private final LogicalPlan child; + + public LogicalPlan analyze(UnresolvedExpression projectItem, AnalysisContext context) { + LogicalPlan highlight = projectItem.accept(this, context); + return (highlight == null) ? child : highlight; + } + + @Override + public LogicalPlan visitAlias(Alias node, AnalysisContext context) { + if (!(node.getDelegated() instanceof HighlightFunction)) { + return null; + } + + HighlightFunction unresolved = (HighlightFunction) node.getDelegated(); + Expression field = expressionAnalyzer.analyze(unresolved.getHighlightField(), context); + return new LogicalHighlight(child, field); + } +} diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java index e8e840fa12..96f9d70e88 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java @@ -1,23 +1,29 @@ package org.opensearch.sql.analysis; +import java.util.Collections; +import java.util.List; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.ToString; import org.opensearch.sql.data.model.ExprValue; +import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.expression.FunctionExpression; import org.opensearch.sql.expression.env.Environment; +import org.opensearch.sql.expression.function.BuiltinFunctionName; @EqualsAndHashCode(callSuper = false) @Getter @ToString -public class HighlightExpression implements Expression { - String highlightField; +public class HighlightExpression extends FunctionExpression { + Expression highlightField; - public HighlightExpression(String field) { - highlightField = field; + public HighlightExpression(Expression highlightField) { + super(BuiltinFunctionName.HIGHLIGHT.getName(), List.of(highlightField)); + this.highlightField = highlightField; } @Override @@ -30,9 +36,9 @@ public ExprType type() { return ExprCoreType.STRING; } - @Override - public T accept(org.opensearch.sql.expression.ExpressionNodeVisitor visitor, - C context) { - return visitor.visitHighlight(this, context); - } + // @Override + // public T accept(org.opensearch.sql.expression.ExpressionNodeVisitor visitor, + // C context) { + // return visitor.visitHighlight(this, context); + // } } diff --git a/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java b/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java index 69a7482452..45dcdde012 100644 --- a/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java @@ -59,6 +59,7 @@ public T visitNamed(NamedExpression node, C context) { public T visitHighlight(HighlightExpression node, C context) { return visitNode(node, context); } + public T visitReference(ReferenceExpression node, C context) { return visitNode(node, context); } diff --git a/core/src/main/java/org/opensearch/sql/planner/logical/LogicalHighlight.java b/core/src/main/java/org/opensearch/sql/planner/logical/LogicalHighlight.java new file mode 100644 index 0000000000..a7905d6171 --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/planner/logical/LogicalHighlight.java @@ -0,0 +1,24 @@ +package org.opensearch.sql.planner.logical; + +import java.util.Collections; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import org.opensearch.sql.expression.Expression; + +@EqualsAndHashCode(callSuper = true) +@Getter +@ToString +public class LogicalHighlight extends LogicalPlan { + private final Expression highlightField; + + public LogicalHighlight(LogicalPlan childPlan, Expression field) { + super(Collections.singletonList(childPlan)); + highlightField = field; + } + + @Override + public R accept(LogicalPlanNodeVisitor visitor, C context) { + return visitor.visitHighlight(this, context); + } +} diff --git a/core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanDSL.java b/core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanDSL.java index d5fef43c0d..4185d55c55 100644 --- a/core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanDSL.java +++ b/core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanDSL.java @@ -62,6 +62,10 @@ public LogicalPlan window(LogicalPlan input, return new LogicalWindow(input, windowFunction, windowDefinition); } + public LogicalPlan highlight(LogicalPlan input, Expression field) { + return new LogicalHighlight(input, field); + } + public static LogicalPlan remove(LogicalPlan input, ReferenceExpression... fields) { return new LogicalRemove(input, ImmutableSet.copyOf(fields)); } diff --git a/core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitor.java b/core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitor.java index 5163e44edb..df23b9cd20 100644 --- a/core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitor.java @@ -26,6 +26,10 @@ public R visitFilter(LogicalFilter plan, C context) { return visitNode(plan, context); } + public R visitHighlight(LogicalHighlight plan, C context) { + return visitNode(plan, context); + } + public R visitAggregation(LogicalAggregation plan, C context) { return visitNode(plan, context); } diff --git a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java index 110e48baaf..28598e35a6 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java @@ -22,6 +22,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; import static org.opensearch.sql.ast.dsl.AstDSL.relation; import static org.opensearch.sql.ast.dsl.AstDSL.span; +import static org.opensearch.sql.ast.dsl.AstDSL.stringLiteral; import static org.opensearch.sql.ast.tree.Sort.NullOrder; import static org.opensearch.sql.ast.tree.Sort.SortOption; import static org.opensearch.sql.ast.tree.Sort.SortOption.DEFAULT_ASC; @@ -235,15 +236,19 @@ public void project_source() { @Test public void project_highlight() { assertAnalyzeEqual( - LogicalPlanDSL.project(LogicalPlanDSL.relation("schema"), - DSL.named("highlight(fieldA)", new HighlightExpression("fieldA"))), + LogicalPlanDSL.project( + LogicalPlanDSL.highlight(LogicalPlanDSL.relation("schema"), + DSL.literal("fieldA")), + DSL.named("highlight(fieldA)", new HighlightExpression(DSL.literal("fieldA"))) + ), AstDSL.projectWithArg( AstDSL.relation("schema"), AstDSL.defaultFieldsArgs(), - AstDSL.alias("highlight(fieldA)", new HighlightFunction("fieldA")) + AstDSL.alias("highlight(fieldA)", new HighlightFunction(AstDSL.stringLiteral("fieldA"))) ) ); } + @Test public void remove_source() { assertAnalyzeEqual( @@ -287,7 +292,7 @@ public void project_values() { AstDSL.project( AstDSL.values(ImmutableList.of(AstDSL.intLiteral(123))), AstDSL.alias("123", AstDSL.intLiteral(123)), - AstDSL.alias("hello", AstDSL.stringLiteral("hello")), + AstDSL.alias("hello", stringLiteral("hello")), AstDSL.alias("false", AstDSL.booleanLiteral(false)) ) ); @@ -707,7 +712,7 @@ public void parse_relation() { AstDSL.parse( AstDSL.relation("schema"), AstDSL.field("string_value"), - AstDSL.stringLiteral("(?.*)")), + stringLiteral("(?.*)")), AstDSL.alias("string_value", qualifiedName("string_value")) )); } diff --git a/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java index 73aabcc3eb..668af4263d 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java @@ -45,6 +45,7 @@ import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.DSL; import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.expression.LiteralExpression; import org.opensearch.sql.expression.config.ExpressionConfig; import org.opensearch.sql.expression.window.aggregation.AggregateWindowFunction; import org.springframework.context.annotation.Configuration; @@ -538,7 +539,8 @@ public void match_phrase_prefix_all_params() { @Test void highlight() { - assertAnalyzeEqual(new HighlightExpression("fieldA"), new HighlightFunction("fieldA")); + assertAnalyzeEqual(new HighlightExpression(DSL.literal("fieldA")), + new HighlightFunction(stringLiteral("fieldA"))); } protected Expression analyze(UnresolvedExpression unresolvedExpression) { diff --git a/core/src/test/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java b/core/src/test/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java index a2455a9a3d..e899351d4f 100644 --- a/core/src/test/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java @@ -19,13 +19,14 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.expression.DataType; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.tree.RareTopN.CommandType; import org.opensearch.sql.ast.tree.Sort.SortOption; +import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.expression.DSL; import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.expression.LiteralExpression; import org.opensearch.sql.expression.ReferenceExpression; import org.opensearch.sql.expression.aggregation.Aggregator; import org.opensearch.sql.expression.window.WindowDefinition; @@ -113,6 +114,11 @@ public void testAbstractPlanNodeVisitorShouldReturnNull() { assertNull(rareTopN.accept(new LogicalPlanNodeVisitor() { }, null)); + LogicalPlan highlight = new LogicalHighlight(filter, + new LiteralExpression(ExprValueUtils.stringValue("fieldA"))); + assertNull(highlight.accept(new LogicalPlanNodeVisitor() { + }, null)); + LogicalPlan mlCommons = new LogicalMLCommons(LogicalPlanDSL.relation("schema"), "kmeans", ImmutableMap.builder() From 5c8db0ac3c93e07b43a6d8f08db9e4c9f7ad7ab4 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Wed, 6 Jul 2022 00:49:13 -0700 Subject: [PATCH 09/24] Add a simple IT test for highlight. Signed-off-by: MaxKsyunz --- .../sql/sql/HighlightFunctionIT.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java new file mode 100644 index 0000000000..8e3d52e927 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java @@ -0,0 +1,22 @@ +package org.opensearch.sql.sql; + +import org.json.JSONObject; +import org.junit.Test; +import org.opensearch.sql.legacy.SQLIntegTestCase; +import org.opensearch.sql.legacy.TestsConstants; + +public class HighlightFunctionIT extends SQLIntegTestCase { + + @Override + protected void init() throws Exception { + loadIndex(Index.BEER); + } + + @Test + public void defaultParameters() { + String query = "SELECT Tags, highlight('Tags') from %s WHERE match(Tags, 'yeast') limit 1"; +// String query = "SELECT Tags from %s WHERE match(Tags, 'yeast') limit 1"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); + assertEquals(1, response.getInt("total")); + } +} From ac9f0807576f2bbd02337f4eb6806f6a5b64b6d5 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Wed, 6 Jul 2022 00:49:58 -0700 Subject: [PATCH 10/24] Register highlight function in the BuiltInFunctionRepository Signed-off-by: MaxKsyunz --- .../sql/expression/function/OpenSearchFunctions.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java index dff3ec1f20..d330f35542 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java @@ -8,7 +8,6 @@ import static org.opensearch.sql.data.type.ExprCoreType.STRING; import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.util.ArrayList; import java.util.Collections; @@ -16,6 +15,7 @@ import java.util.Map; import java.util.stream.Collectors; import lombok.experimental.UtilityClass; +import org.opensearch.sql.analysis.HighlightExpression; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; @@ -50,6 +50,14 @@ public void register(BuiltinFunctionRepository repository) { repository.register(match_phrase(BuiltinFunctionName.MATCH_PHRASE)); repository.register(match_phrase(BuiltinFunctionName.MATCHPHRASE)); repository.register(match_phrase_prefix()); + repository.register(highlight()); + } + + private static FunctionResolver highlight() { + FunctionName functionName = BuiltinFunctionName.HIGHLIGHT.getName(); + FunctionSignature functionSignature = new FunctionSignature(functionName, List.of(STRING)); + FunctionBuilder functionBuilder = arguments -> new HighlightExpression(arguments.get(0)); + return new FunctionResolver(functionName, ImmutableMap.of(functionSignature, functionBuilder)); } private static FunctionResolver match_bool_prefix() { From b526132cf252e52216fb2010caba2156ecba039e Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Wed, 6 Jul 2022 00:51:45 -0700 Subject: [PATCH 11/24] Partial support for highlight in physical plan. Signed-off-by: MaxKsyunz --- .../opensearch/sql/planner/DefaultImplementor.java | 1 + .../planner/logical/OpenSearchLogicalIndexScan.java | 11 ++++++++++- .../planner/logical/rule/MergeLimitAndIndexScan.java | 3 ++- .../sql/opensearch/storage/OpenSearchIndex.java | 4 ++++ .../sql/opensearch/storage/OpenSearchIndexScan.java | 11 +++++++++++ .../logical/OpenSearchLogicalIndexScanTest.java | 12 ++++++++++++ 6 files changed, 40 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java b/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java index 9f2c2c5fa8..5f1f76dc04 100644 --- a/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java +++ b/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java @@ -10,6 +10,7 @@ import org.opensearch.sql.planner.logical.LogicalDedupe; import org.opensearch.sql.planner.logical.LogicalEval; import org.opensearch.sql.planner.logical.LogicalFilter; +import org.opensearch.sql.planner.logical.LogicalHighlight; import org.opensearch.sql.planner.logical.LogicalLimit; import org.opensearch.sql.planner.logical.LogicalPlan; import org.opensearch.sql.planner.logical.LogicalPlanNodeVisitor; diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScan.java index d182b5f84d..8b23daad1c 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScan.java @@ -58,6 +58,10 @@ public class OpenSearchLogicalIndexScan extends LogicalPlan { @Setter private Integer limit; + + @Setter + private String highlightField; + /** * ElasticsearchLogicalIndexScan Constructor. */ @@ -67,7 +71,7 @@ public OpenSearchLogicalIndexScan( Expression filter, Set projectList, List> sortList, - Integer limit, Integer offset) { + Integer limit, Integer offset, String highlightField) { super(ImmutableList.of()); this.relationName = relationName; this.filter = filter; @@ -75,6 +79,7 @@ public OpenSearchLogicalIndexScan( this.sortList = sortList; this.limit = limit; this.offset = offset; + this.highlightField = highlightField; } @Override @@ -86,6 +91,10 @@ public boolean hasLimit() { return limit != null; } + public boolean hasHighlight() { + return highlightField != null; + } + /** * Test has projects or not. * diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeLimitAndIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeLimitAndIndexScan.java index 9d880bb4dc..fb8aba2c90 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeLimitAndIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeLimitAndIndexScan.java @@ -45,7 +45,8 @@ public LogicalPlan apply(LogicalLimit plan, Captures captures) { builder.relationName(indexScan.getRelationName()) .filter(indexScan.getFilter()) .offset(plan.getOffset()) - .limit(plan.getLimit()); + .limit(plan.getLimit()) + .highlightField(indexScan.getHighlightField()); if (indexScan.getSortList() != null) { builder.sortList(indexScan.getSortList()); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java index 49301cbf53..e245136791 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java @@ -145,6 +145,10 @@ public PhysicalPlan visitIndexScan(OpenSearchLogicalIndexScan node, if (node.hasProjects()) { context.pushDownProjects(node.getProjectList()); } + + if (node.hasHighlight()) { + context.pushDownHighlight(node.getHighlightField()); + } return indexScan; } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java index c35a5ba9db..a896764c92 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java @@ -25,6 +25,7 @@ import org.opensearch.index.query.QueryBuilders; import org.opensearch.search.aggregations.AggregationBuilder; import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.search.fetch.subphase.highlight.HighlightBuilder; import org.opensearch.search.sort.SortBuilder; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.data.model.ExprValue; @@ -158,6 +159,16 @@ public void pushDownLimit(Integer limit, Integer offset) { sourceBuilder.from(offset).size(limit); } + /** + * Add highlight to DSL requests. + * @param field name of the field to highlight + */ + public void pushDownHighlight(String field) { + SearchSourceBuilder sourceBuilder = request.getSourceBuilder(); + HighlightBuilder highlightBuilder = new HighlightBuilder().field(field); + sourceBuilder.highlighter(highlightBuilder); + } + /** * Push down project list to DSL requets. */ diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java index 2e10f33787..52e76a602b 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java @@ -7,6 +7,7 @@ package org.opensearch.sql.opensearch.planner.logical; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.ImmutableSet; import org.junit.jupiter.api.Test; @@ -21,4 +22,15 @@ void has_projects() { assertFalse(OpenSearchLogicalIndexScan.builder().build().hasProjects()); } + + @Test + void has_highlight() { + assertTrue( + OpenSearchLogicalIndexScan.builder().highlightField("fieldA").build().hasHighlight()); + } + + @Test + void no_highlight_by_default() { + assertFalse(OpenSearchLogicalIndexScan.builder().build().hasHighlight()); + } } From 807c475c838ea3d55d7a1173afab817ccfb64fda Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Wed, 6 Jul 2022 16:08:34 -0700 Subject: [PATCH 12/24] Add HighlightOperator. Signed-off-by: MaxKsyunz --- .../planner/physical/HighlightOperator.java | 40 +++++++++++++++++++ .../sql/planner/physical/PhysicalPlanDSL.java | 1 - .../physical/PhysicalPlanNodeVisitor.java | 4 +- .../opensearch/storage/OpenSearchIndex.java | 9 +++++ 4 files changed, 52 insertions(+), 2 deletions(-) create mode 100644 core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java new file mode 100644 index 0000000000..d1afb48a47 --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java @@ -0,0 +1,40 @@ +package org.opensearch.sql.planner.physical; + +import java.util.Collections; +import java.util.List; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import org.opensearch.sql.data.model.ExprValue; +import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.planner.physical.PhysicalPlan; +import org.opensearch.sql.planner.physical.PhysicalPlanNodeVisitor; + +@RequiredArgsConstructor +@EqualsAndHashCode(callSuper = false) +@Getter +public class HighlightOperator extends PhysicalPlan { + private final PhysicalPlan input; + private final Expression highlightField; + + @Override + public R accept(PhysicalPlanNodeVisitor visitor, C context) { + return visitor.visitHighlight(this, context); + } + + @Override + public List getChild() { + return Collections.singletonList(input); + } + + @Override + public boolean hasNext() { + return input.hasNext(); + } + + @Override + public ExprValue next() { + ExprValue inputValue = input.next(); + return inputValue; + } +} diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java index 938a4c532c..e6e59990c8 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java @@ -105,5 +105,4 @@ public ValuesOperator values(List... values) { public static LimitOperator limit(PhysicalPlan input, Integer limit, Integer offset) { return new LimitOperator(input, limit, offset); } - } diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java index 87582df3bb..6a81d66df6 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java @@ -80,5 +80,7 @@ public R visitAD(PhysicalPlan node, C context) { return visitNode(node, context); } - + public R visitHighlight(HighlightOperator highlightOperator, C context) { + return visitNode(highlightOperator, context); + } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java index e245136791..4cf776c677 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java @@ -23,6 +23,7 @@ import org.opensearch.sql.opensearch.planner.logical.OpenSearchLogicalIndexScan; import org.opensearch.sql.opensearch.planner.logical.OpenSearchLogicalPlanOptimizerFactory; import org.opensearch.sql.opensearch.planner.physical.ADOperator; +import org.opensearch.sql.planner.physical.HighlightOperator; import org.opensearch.sql.opensearch.planner.physical.MLCommonsOperator; import org.opensearch.sql.opensearch.request.OpenSearchRequest; import org.opensearch.sql.opensearch.request.system.OpenSearchDescribeIndexRequest; @@ -33,6 +34,7 @@ import org.opensearch.sql.opensearch.storage.serialization.DefaultExpressionSerializer; import org.opensearch.sql.planner.DefaultImplementor; import org.opensearch.sql.planner.logical.LogicalAD; +import org.opensearch.sql.planner.logical.LogicalHighlight; import org.opensearch.sql.planner.logical.LogicalMLCommons; import org.opensearch.sql.planner.logical.LogicalPlan; import org.opensearch.sql.planner.logical.LogicalRelation; @@ -120,6 +122,7 @@ public PhysicalPlan visitNode(LogicalPlan plan, OpenSearchIndexScan context) { } } + /** * Implement ElasticsearchLogicalIndexScan. */ @@ -149,6 +152,7 @@ public PhysicalPlan visitIndexScan(OpenSearchLogicalIndexScan node, if (node.hasHighlight()) { context.pushDownHighlight(node.getHighlightField()); } + return indexScan; } @@ -191,5 +195,10 @@ public PhysicalPlan visitAD(LogicalAD node, OpenSearchIndexScan context) { return new ADOperator(visitChild(node, context), node.getArguments(), client.getNodeClient()); } + + @Override + public PhysicalPlan visitHighlight(LogicalHighlight node, OpenSearchIndexScan context) { + return new HighlightOperator(visitChild(node, context), node.getHighlightField()); + } } } From 74b64920ddae1336eed4fc03ca64aacdaab03087 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Thu, 7 Jul 2022 02:21:48 -0700 Subject: [PATCH 13/24] Highlight alpha complete. Signed-off-by: MaxKsyunz --- .../sql/analysis/HighlightExpression.java | 7 ++++++- .../expression/function/OpenSearchFunctions.java | 1 + .../sql/planner/physical/HighlightOperator.java | 7 +++++++ .../protector/OpenSearchExecutionProtector.java | 9 +++++++++ .../logical/rule/MergeSortAndIndexScan.java | 1 + .../opensearch/response/OpenSearchResponse.java | 16 +++++++++++++++- .../sql/opensearch/storage/OpenSearchIndex.java | 1 + 7 files changed, 40 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java index 96f9d70e88..4fa532376b 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java @@ -1,5 +1,6 @@ package org.opensearch.sql.analysis; +import java.util.Arrays; import java.util.Collections; import java.util.List; import lombok.EqualsAndHashCode; @@ -10,8 +11,10 @@ import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.expression.DSL; import org.opensearch.sql.expression.Expression; import org.opensearch.sql.expression.FunctionExpression; +import org.opensearch.sql.expression.ReferenceExpression; import org.opensearch.sql.expression.env.Environment; import org.opensearch.sql.expression.function.BuiltinFunctionName; @@ -28,7 +31,9 @@ public HighlightExpression(Expression highlightField) { @Override public ExprValue valueOf(Environment valueEnv) { - throw new SemanticCheckException("valeOf highlight is not supported"); + // TODO Find the highlight data for the field highlightField, and return it. + String refName = "_highlight(" + highlightField.toString() + ")"; + return valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); } @Override diff --git a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java index d330f35542..1f857dfd53 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java @@ -56,6 +56,7 @@ public void register(BuiltinFunctionRepository repository) { private static FunctionResolver highlight() { FunctionName functionName = BuiltinFunctionName.HIGHLIGHT.getName(); FunctionSignature functionSignature = new FunctionSignature(functionName, List.of(STRING)); + // TODO change to pass the argument as a StringLiteral FunctionBuilder functionBuilder = arguments -> new HighlightExpression(arguments.get(0)); return new FunctionResolver(functionName, ImmutableMap.of(functionSignature, functionBuilder)); } diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java index d1afb48a47..0b0d4ca957 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java @@ -1,9 +1,11 @@ package org.opensearch.sql.planner.physical; +import com.google.common.collect.ImmutableMap; import java.util.Collections; import java.util.List; import lombok.EqualsAndHashCode; import lombok.Getter; +import lombok.NonNull; import lombok.RequiredArgsConstructor; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.expression.Expression; @@ -14,7 +16,9 @@ @EqualsAndHashCode(callSuper = false) @Getter public class HighlightOperator extends PhysicalPlan { + @NonNull private final PhysicalPlan input; + @NonNull private final Expression highlightField; @Override @@ -35,6 +39,9 @@ public boolean hasNext() { @Override public ExprValue next() { ExprValue inputValue = input.next(); + ImmutableMap.Builder mapBuilder = new ImmutableMap.Builder<>(); + inputValue.tupleValue().forEach(mapBuilder::put); + return inputValue; } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java index 45d2b12620..8253fe8c2f 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java @@ -14,6 +14,7 @@ import org.opensearch.sql.planner.physical.DedupeOperator; import org.opensearch.sql.planner.physical.EvalOperator; import org.opensearch.sql.planner.physical.FilterOperator; +import org.opensearch.sql.planner.physical.HighlightOperator; import org.opensearch.sql.planner.physical.LimitOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.planner.physical.ProjectOperator; @@ -150,6 +151,14 @@ public PhysicalPlan visitAD(PhysicalPlan node, Object context) { ); } + @Override + public PhysicalPlan visitHighlight(HighlightOperator node, Object context) { + return doProtect( + doProtect(new HighlightOperator(visitInput(node.getInput(), context), + node.getHighlightField())) + ); + } + PhysicalPlan visitInput(PhysicalPlan node, Object context) { if (null == node) { return node; diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexScan.java index 337f09308c..46324b9898 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexScan.java @@ -56,6 +56,7 @@ public LogicalPlan apply(LogicalSort sort, .relationName(indexScan.getRelationName()) .filter(indexScan.getFilter()) .sortList(mergeSortList(indexScan.getSortList(), sort.getSortList())) + .highlightField(indexScan.getHighlightField()) .build(); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java index 7dc77d7d29..0223a0acba 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java @@ -17,6 +17,7 @@ import org.opensearch.search.aggregations.Aggregations; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; +import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.opensearch.data.value.OpenSearchExprValueFactory; /** @@ -91,7 +92,20 @@ public Iterator iterator() { }).iterator(); } else { return Arrays.stream(hits.getHits()) - .map(hit -> (ExprValue) exprValueFactory.construct(hit.getSourceAsString())).iterator(); + .map(hit -> { + ExprValue docData = exprValueFactory.construct(hit.getSourceAsString()); + if (hit.getHighlightFields().isEmpty()) { + return docData; + } else { + ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); + builder.putAll(docData.tupleValue()); + for (var es : hit.getHighlightFields().entrySet()) { + String key = "_highlight(" + es.getKey() + ")"; + builder.put(key, ExprValueUtils.stringValue(es.getValue().toString())); + } + return ExprTupleValue.fromExprValueMap(builder.build()); + } + }).iterator(); } } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java index 4cf776c677..5b306cb9dc 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java @@ -198,6 +198,7 @@ public PhysicalPlan visitAD(LogicalAD node, OpenSearchIndexScan context) { @Override public PhysicalPlan visitHighlight(LogicalHighlight node, OpenSearchIndexScan context) { + context.pushDownHighlight(node.getHighlightField().toString()); return new HighlightOperator(visitChild(node, context), node.getHighlightField()); } } From ad7affca3644c5905db40ceeb64901f8a8b25b03 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Mon, 25 Jul 2022 09:13:25 -0700 Subject: [PATCH 14/24] Initial implementation to upporting highlight('*') Signed-off-by: forestmvey --- .../org/opensearch/sql/analysis/Analyzer.java | 1 + .../sql/analysis/HighlightAnalyzer.java | 1 + .../sql/analysis/HighlightExpression.java | 32 +++++++++++++++++-- .../sql/ast/expression/HighlightFunction.java | 6 ++-- .../function/OpenSearchFunctions.java | 1 + .../planner/physical/HighlightOperator.java | 10 +++--- .../sql/sql/HighlightFunctionIT.java | 1 - .../OpenSearchExecutionProtector.java | 5 ++- .../response/OpenSearchResponse.java | 7 ++-- .../storage/OpenSearchIndexScan.java | 3 +- sql/src/main/antlr/OpenSearchSQLLexer.g4 | 1 + sql/src/main/antlr/OpenSearchSQLParser.g4 | 1 + 12 files changed, 52 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java index 4ac2a3a797..dc12bdab73 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java @@ -295,6 +295,7 @@ public LogicalPlan visitProject(Project node, AnalysisContext context) { for (UnresolvedExpression expr : node.getProjectList()) { HighlightAnalyzer highlightAnalyzer = new HighlightAnalyzer(expressionAnalyzer, child); child = highlightAnalyzer.analyze(expr, context); + } List namedExpressions = diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java index 2005046e4a..f3ccce1ee2 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java @@ -3,6 +3,7 @@ import lombok.RequiredArgsConstructor; import org.opensearch.sql.ast.AbstractNodeVisitor; import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.AllFields; import org.opensearch.sql.ast.expression.HighlightFunction; import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.ast.expression.WindowFunction; diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java index 4fa532376b..f5ac8849c2 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java @@ -3,9 +3,13 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.ToString; +import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.data.type.ExprCoreType; @@ -17,6 +21,7 @@ import org.opensearch.sql.expression.ReferenceExpression; import org.opensearch.sql.expression.env.Environment; import org.opensearch.sql.expression.function.BuiltinFunctionName; +import org.opensearch.sql.common.utils.StringUtils; @EqualsAndHashCode(callSuper = false) @Getter @@ -31,9 +36,30 @@ public HighlightExpression(Expression highlightField) { @Override public ExprValue valueOf(Environment valueEnv) { - // TODO Find the highlight data for the field highlightField, and return it. - String refName = "_highlight(" + highlightField.toString() + ")"; - return valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); + String refName = "_highlight"; + if(!getHighlightField().toString().contains("*")) { + refName += "." + StringUtils.unquoteText(getHighlightField().toString()); + } + ExprValue temp = valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); + ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); + + if(temp.isMissing() || temp.type() == ExprCoreType.STRING) { + return temp; + } else if(temp.tupleValue().entrySet().size() > 1) { + var hlBuilder = ImmutableMap.builder(); + hlBuilder.putAll(temp.tupleValue()); + + for (var foo : temp.tupleValue().entrySet()) { + String blah = "highlight_" + foo.getKey(); + builder.put(blah, ExprValueUtils.stringValue(foo.getValue().toString())); + } + + return ExprTupleValue.fromExprValueMap(builder.build()); + } + return temp.tupleValue().get(StringUtils.unquoteText(this.highlightField.toString())); +// return ExprTupleValue.fromExprValueMap(builder.build()); +// String refName = "_highlight(" + highlightField.toString() + ")"; +// return valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); } @Override diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java index 71a4bfe4ec..0e6f5c660d 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java @@ -9,18 +9,20 @@ import lombok.ToString; import org.opensearch.sql.ast.AbstractNodeVisitor; -@AllArgsConstructor @EqualsAndHashCode(callSuper = false) @Getter @ToString public class HighlightFunction extends UnresolvedExpression { private final UnresolvedExpression highlightField; - @Override public T accept(AbstractNodeVisitor nodeVisitor, C context) { return nodeVisitor.visitHighlight(this, context); } + public HighlightFunction(UnresolvedExpression field) { + this.highlightField = field; + } + @Override public List getChild() { return List.of(highlightField); diff --git a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java index 1f857dfd53..f295bbf57c 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java @@ -23,6 +23,7 @@ import org.opensearch.sql.expression.FunctionExpression; import org.opensearch.sql.expression.NamedArgumentExpression; import org.opensearch.sql.expression.env.Environment; +import org.opensearch.sql.common.utils.StringUtils; @UtilityClass public class OpenSearchFunctions { diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java index 0b0d4ca957..2379ef5473 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java @@ -6,21 +6,21 @@ import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.NonNull; -import lombok.RequiredArgsConstructor; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.expression.Expression; -import org.opensearch.sql.planner.physical.PhysicalPlan; -import org.opensearch.sql.planner.physical.PhysicalPlanNodeVisitor; -@RequiredArgsConstructor @EqualsAndHashCode(callSuper = false) @Getter public class HighlightOperator extends PhysicalPlan { @NonNull private final PhysicalPlan input; - @NonNull private final Expression highlightField; + public HighlightOperator(PhysicalPlan input, Expression highlightField) { + this.input = input; + this.highlightField = highlightField; + } + @Override public R accept(PhysicalPlanNodeVisitor visitor, C context) { return visitor.visitHighlight(this, context); diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java index 8e3d52e927..db536b93bf 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java @@ -15,7 +15,6 @@ protected void init() throws Exception { @Test public void defaultParameters() { String query = "SELECT Tags, highlight('Tags') from %s WHERE match(Tags, 'yeast') limit 1"; -// String query = "SELECT Tags from %s WHERE match(Tags, 'yeast') limit 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); assertEquals(1, response.getInt("total")); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java index 8253fe8c2f..f36b7b97db 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java @@ -153,9 +153,8 @@ public PhysicalPlan visitAD(PhysicalPlan node, Object context) { @Override public PhysicalPlan visitHighlight(HighlightOperator node, Object context) { - return doProtect( - doProtect(new HighlightOperator(visitInput(node.getInput(), context), - node.getHighlightField())) + return doProtect(new HighlightOperator(visitInput(node.getInput(), context), + node.getHighlightField()) ); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java index 0223a0acba..0e551c422d 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java @@ -9,12 +9,14 @@ import com.google.common.collect.ImmutableMap; import java.util.Arrays; import java.util.Iterator; +import java.util.List; import java.util.Map; import lombok.EqualsAndHashCode; import lombok.ToString; import org.opensearch.action.search.SearchResponse; import org.opensearch.search.SearchHits; import org.opensearch.search.aggregations.Aggregations; +import org.opensearch.search.fetch.subphase.highlight.HighlightField; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; @@ -99,10 +101,11 @@ public Iterator iterator() { } else { ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); builder.putAll(docData.tupleValue()); + var hlBuilder = ImmutableMap.builder(); for (var es : hit.getHighlightFields().entrySet()) { - String key = "_highlight(" + es.getKey() + ")"; - builder.put(key, ExprValueUtils.stringValue(es.getValue().toString())); + hlBuilder.put(es.getKey(), ExprValueUtils.stringValue(es.getValue().toString())); } + builder.put("_highlight", ExprTupleValue.fromExprValueMap(hlBuilder.build())); return ExprTupleValue.fromExprValueMap(builder.build()); } }).iterator(); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java index a896764c92..9c79659a4a 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java @@ -28,6 +28,7 @@ import org.opensearch.search.fetch.subphase.highlight.HighlightBuilder; import org.opensearch.search.sort.SortBuilder; import org.opensearch.sql.common.setting.Settings; +import org.opensearch.sql.common.utils.StringUtils; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.expression.ReferenceExpression; @@ -165,7 +166,7 @@ public void pushDownLimit(Integer limit, Integer offset) { */ public void pushDownHighlight(String field) { SearchSourceBuilder sourceBuilder = request.getSourceBuilder(); - HighlightBuilder highlightBuilder = new HighlightBuilder().field(field); + HighlightBuilder highlightBuilder = new HighlightBuilder().field(StringUtils.unquoteText(field)); sourceBuilder.highlighter(highlightBuilder); } diff --git a/sql/src/main/antlr/OpenSearchSQLLexer.g4 b/sql/src/main/antlr/OpenSearchSQLLexer.g4 index 8c4a092e62..9fca2942cf 100644 --- a/sql/src/main/antlr/OpenSearchSQLLexer.g4 +++ b/sql/src/main/antlr/OpenSearchSQLLexer.g4 @@ -347,6 +347,7 @@ TIME_ZONE: 'TIME_ZONE'; TYPE: 'TYPE'; ZERO_TERMS_QUERY: 'ZERO_TERMS_QUERY'; HIGHLIGHT: 'HIGHLIGHT'; + // RELEVANCE FUNCTIONS MATCH_BOOL_PREFIX: 'MATCH_BOOL_PREFIX'; // Operators diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index a9316b55a4..dde02cfd3e 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -305,6 +305,7 @@ functionCall highlightFunction : HIGHLIGHT LR_BRACKET relevanceField RR_BRACKET + | HIGHLIGHT LR_BRACKET STAR RR_BRACKET ; scalarFunctionName From bb97dde07f79a58efc8902c7c838c015dccbec5e Mon Sep 17 00:00:00 2001 From: forestmvey Date: Mon, 25 Jul 2022 16:25:35 -0700 Subject: [PATCH 15/24] Add support for multiple highlight calls in select statement. Signed-off-by: forestmvey --- .../sql/analysis/HighlightExpression.java | 57 ++++++++++--------- .../sql/ast/expression/HighlightFunction.java | 13 ++--- .../storage/OpenSearchIndexScan.java | 9 ++- 3 files changed, 43 insertions(+), 36 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java index f5ac8849c2..ea3134fcaa 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java @@ -1,10 +1,12 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + package org.opensearch.sql.analysis; -import java.util.Arrays; -import java.util.Collections; import java.util.List; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import lombok.EqualsAndHashCode; import lombok.Getter; @@ -14,11 +16,9 @@ import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; -import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.DSL; import org.opensearch.sql.expression.Expression; import org.opensearch.sql.expression.FunctionExpression; -import org.opensearch.sql.expression.ReferenceExpression; import org.opensearch.sql.expression.env.Environment; import org.opensearch.sql.expression.function.BuiltinFunctionName; import org.opensearch.sql.common.utils.StringUtils; @@ -27,49 +27,52 @@ @Getter @ToString public class HighlightExpression extends FunctionExpression { - Expression highlightField; + private final Expression highlightField; + /** + * HighlightExpression Constructor. + * @param highlightField : Highlight field for expression. + */ public HighlightExpression(Expression highlightField) { super(BuiltinFunctionName.HIGHLIGHT.getName(), List.of(highlightField)); this.highlightField = highlightField; } + /** + * Return String or Map value matching highlight field. + * @param valueEnv : Dataset to parse value from. + * @return : String or Map value of highlight fields. + */ @Override public ExprValue valueOf(Environment valueEnv) { String refName = "_highlight"; if(!getHighlightField().toString().contains("*")) { refName += "." + StringUtils.unquoteText(getHighlightField().toString()); } - ExprValue temp = valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); + ExprValue retVal = valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); - if(temp.isMissing() || temp.type() == ExprCoreType.STRING) { - return temp; - } else if(temp.tupleValue().entrySet().size() > 1) { - var hlBuilder = ImmutableMap.builder(); - hlBuilder.putAll(temp.tupleValue()); + if(retVal.isMissing() || retVal.type() != ExprCoreType.STRUCT) { + return retVal; + } - for (var foo : temp.tupleValue().entrySet()) { - String blah = "highlight_" + foo.getKey(); - builder.put(blah, ExprValueUtils.stringValue(foo.getValue().toString())); - } + var hlBuilder = ImmutableMap.builder(); + hlBuilder.putAll(retVal.tupleValue()); - return ExprTupleValue.fromExprValueMap(builder.build()); + for (var entry : retVal.tupleValue().entrySet()) { + String entryKey = "highlight(" + getHighlightField() + ")." + entry.getKey(); + builder.put(entryKey, ExprValueUtils.stringValue(entry.getValue().toString())); } - return temp.tupleValue().get(StringUtils.unquoteText(this.highlightField.toString())); -// return ExprTupleValue.fromExprValueMap(builder.build()); -// String refName = "_highlight(" + highlightField.toString() + ")"; -// return valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); + + return ExprTupleValue.fromExprValueMap(builder.build()); } + /** + * Get type for HighlightExpression. + * @return : String type. + */ @Override public ExprType type() { return ExprCoreType.STRING; } - - // @Override - // public T accept(org.opensearch.sql.expression.ExpressionNodeVisitor visitor, - // C context) { - // return visitor.visitHighlight(this, context); - // } } diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java index 0e6f5c660d..f26cff0288 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java @@ -1,14 +1,18 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + package org.opensearch.sql.ast.expression; -import com.google.common.collect.ImmutableList; import java.util.List; import lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; import lombok.Getter; -import lombok.RequiredArgsConstructor; import lombok.ToString; import org.opensearch.sql.ast.AbstractNodeVisitor; +@AllArgsConstructor @EqualsAndHashCode(callSuper = false) @Getter @ToString @@ -18,11 +22,6 @@ public class HighlightFunction extends UnresolvedExpression { public T accept(AbstractNodeVisitor nodeVisitor, C context) { return nodeVisitor.visitHighlight(this, context); } - - public HighlightFunction(UnresolvedExpression field) { - this.highlightField = field; - } - @Override public List getChild() { return List.of(highlightField); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java index 9c79659a4a..2a3cc05f15 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java @@ -166,8 +166,13 @@ public void pushDownLimit(Integer limit, Integer offset) { */ public void pushDownHighlight(String field) { SearchSourceBuilder sourceBuilder = request.getSourceBuilder(); - HighlightBuilder highlightBuilder = new HighlightBuilder().field(StringUtils.unquoteText(field)); - sourceBuilder.highlighter(highlightBuilder); + if(sourceBuilder.highlighter() != null) { + sourceBuilder.highlighter().field(StringUtils.unquoteText(field)); + sourceBuilder.highlighter(sourceBuilder.highlighter()); + } else { + HighlightBuilder highlightBuilder = new HighlightBuilder().field(StringUtils.unquoteText(field)); + sourceBuilder.highlighter(highlightBuilder); + } } /** From 163b90976d37fb43d53b854d3c4927f252a2b763 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Tue, 26 Jul 2022 08:03:19 -0700 Subject: [PATCH 16/24] Removed OpenSearchLogicalIndexScan highlightFields and dependencies. Improved test coverage and fixing checkstyle errors. Signed-off-by: forestmvey --- .../sql/analysis/HighlightAnalyzer.java | 7 ++- .../sql/analysis/HighlightExpression.java | 9 ++-- .../sql/ast/expression/HighlightFunction.java | 2 + .../function/OpenSearchFunctions.java | 2 - .../sql/planner/DefaultImplementor.java | 1 - .../sql/planner/logical/LogicalHighlight.java | 5 ++ .../planner/physical/HighlightOperator.java | 5 ++ .../expression/ExpressionNodeVisitorTest.java | 6 +++ .../physical/HighlightOperatorTest.java | 40 ++++++++++++++ .../physical/PhysicalPlanNodeVisitorTest.java | 5 ++ .../sql/sql/HighlightFunctionIT.java | 53 ++++++++++++++++++- .../logical/OpenSearchLogicalIndexScan.java | 11 +--- .../logical/rule/MergeLimitAndIndexScan.java | 3 +- .../logical/rule/MergeSortAndIndexScan.java | 1 - .../opensearch/storage/OpenSearchIndex.java | 8 +-- .../storage/OpenSearchIndexScan.java | 5 +- .../OpenSearchLogicalIndexScanTest.java | 11 ---- sql/src/main/antlr/OpenSearchSQLParser.g4 | 1 - .../sql/sql/antlr/HighlightTest.java | 26 ++++++++- 19 files changed, 153 insertions(+), 48 deletions(-) create mode 100644 core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java index f3ccce1ee2..61ba7d66f5 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java @@ -1,12 +1,15 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + package org.opensearch.sql.analysis; import lombok.RequiredArgsConstructor; import org.opensearch.sql.ast.AbstractNodeVisitor; import org.opensearch.sql.ast.expression.Alias; -import org.opensearch.sql.ast.expression.AllFields; import org.opensearch.sql.ast.expression.HighlightFunction; import org.opensearch.sql.ast.expression.UnresolvedExpression; -import org.opensearch.sql.ast.expression.WindowFunction; import org.opensearch.sql.expression.Expression; import org.opensearch.sql.planner.logical.LogicalHighlight; import org.opensearch.sql.planner.logical.LogicalPlan; diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java index ea3134fcaa..9e100cdbba 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java @@ -5,12 +5,12 @@ package org.opensearch.sql.analysis; -import java.util.List; - import com.google.common.collect.ImmutableMap; +import java.util.List; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.ToString; +import org.opensearch.sql.common.utils.StringUtils; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; @@ -21,7 +21,6 @@ import org.opensearch.sql.expression.FunctionExpression; import org.opensearch.sql.expression.env.Environment; import org.opensearch.sql.expression.function.BuiltinFunctionName; -import org.opensearch.sql.common.utils.StringUtils; @EqualsAndHashCode(callSuper = false) @Getter @@ -46,13 +45,13 @@ public HighlightExpression(Expression highlightField) { @Override public ExprValue valueOf(Environment valueEnv) { String refName = "_highlight"; - if(!getHighlightField().toString().contains("*")) { + if (!getHighlightField().toString().contains("*")) { refName += "." + StringUtils.unquoteText(getHighlightField().toString()); } ExprValue retVal = valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); - if(retVal.isMissing() || retVal.type() != ExprCoreType.STRUCT) { + if (retVal.isMissing() || retVal.type() != ExprCoreType.STRUCT) { return retVal; } diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java index f26cff0288..eae1525aae 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java @@ -18,10 +18,12 @@ @ToString public class HighlightFunction extends UnresolvedExpression { private final UnresolvedExpression highlightField; + @Override public T accept(AbstractNodeVisitor nodeVisitor, C context) { return nodeVisitor.visitHighlight(this, context); } + @Override public List getChild() { return List.of(highlightField); diff --git a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java index f295bbf57c..d330f35542 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java @@ -23,7 +23,6 @@ import org.opensearch.sql.expression.FunctionExpression; import org.opensearch.sql.expression.NamedArgumentExpression; import org.opensearch.sql.expression.env.Environment; -import org.opensearch.sql.common.utils.StringUtils; @UtilityClass public class OpenSearchFunctions { @@ -57,7 +56,6 @@ public void register(BuiltinFunctionRepository repository) { private static FunctionResolver highlight() { FunctionName functionName = BuiltinFunctionName.HIGHLIGHT.getName(); FunctionSignature functionSignature = new FunctionSignature(functionName, List.of(STRING)); - // TODO change to pass the argument as a StringLiteral FunctionBuilder functionBuilder = arguments -> new HighlightExpression(arguments.get(0)); return new FunctionResolver(functionName, ImmutableMap.of(functionSignature, functionBuilder)); } diff --git a/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java b/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java index 5f1f76dc04..9f2c2c5fa8 100644 --- a/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java +++ b/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java @@ -10,7 +10,6 @@ import org.opensearch.sql.planner.logical.LogicalDedupe; import org.opensearch.sql.planner.logical.LogicalEval; import org.opensearch.sql.planner.logical.LogicalFilter; -import org.opensearch.sql.planner.logical.LogicalHighlight; import org.opensearch.sql.planner.logical.LogicalLimit; import org.opensearch.sql.planner.logical.LogicalPlan; import org.opensearch.sql.planner.logical.LogicalPlanNodeVisitor; diff --git a/core/src/main/java/org/opensearch/sql/planner/logical/LogicalHighlight.java b/core/src/main/java/org/opensearch/sql/planner/logical/LogicalHighlight.java index a7905d6171..986a545486 100644 --- a/core/src/main/java/org/opensearch/sql/planner/logical/LogicalHighlight.java +++ b/core/src/main/java/org/opensearch/sql/planner/logical/LogicalHighlight.java @@ -1,3 +1,8 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + package org.opensearch.sql.planner.logical; import java.util.Collections; diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java index 2379ef5473..5618f89a5c 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java @@ -1,3 +1,8 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + package org.opensearch.sql.planner.physical; import com.google.common.collect.ImmutableMap; diff --git a/core/src/test/java/org/opensearch/sql/expression/ExpressionNodeVisitorTest.java b/core/src/test/java/org/opensearch/sql/expression/ExpressionNodeVisitorTest.java index caf11064ae..3cebf4aaef 100644 --- a/core/src/test/java/org/opensearch/sql/expression/ExpressionNodeVisitorTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/ExpressionNodeVisitorTest.java @@ -20,6 +20,7 @@ import org.junit.jupiter.api.DisplayNameGeneration; import org.junit.jupiter.api.DisplayNameGenerator; import org.junit.jupiter.api.Test; +import org.opensearch.sql.analysis.HighlightExpression; import org.opensearch.sql.expression.aggregation.Aggregator; import org.opensearch.sql.expression.aggregation.AvgAggregator; import org.opensearch.sql.expression.conditional.cases.CaseClause; @@ -88,6 +89,11 @@ public Expression visitAggregator(Aggregator node, Object context) { return dsl.sum(visitArguments(node.getArguments(), context)); } + @Override + public Expression visitHighlight(HighlightExpression node, Object context) { + return node; + } + private Expression[] visitArguments(List arguments, Object context) { return arguments.stream() .map(arg -> arg.accept(this, context)) diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java new file mode 100644 index 0000000000..349820dada --- /dev/null +++ b/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java @@ -0,0 +1,40 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.planner.physical; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.opensearch.sql.data.type.ExprCoreType.STRING; + +import com.google.common.collect.ImmutableMap; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.data.model.ExprValue; +import org.opensearch.sql.data.model.ExprValueUtils; +import org.opensearch.sql.expression.DSL; + +public class HighlightOperatorTest extends PhysicalPlanTestBase { + + @Test + public void highlight_all_test() { + PhysicalPlan plan = new HighlightOperator(new TestScan(), DSL.ref("*", STRING)); + List result = execute(plan); + assertEquals(5, result.size()); + assertThat(result, containsInAnyOrder( + ExprValueUtils.tupleValue(ImmutableMap.of("ip", "209.160.24.63", + "action", "GET", "response", 200, "referer", "www.amazon.com")), + ExprValueUtils.tupleValue(ImmutableMap.of("ip", "209.160.24.63", + "action", "GET", "response", 404, "referer", "www.amazon.com")), + ExprValueUtils.tupleValue(ImmutableMap.of("ip", "112.111.162.4", + "action", "GET", "response", 200, "referer", "www.amazon.com")), + ExprValueUtils.tupleValue(ImmutableMap.of("ip", "74.125.19.106", + "action", "POST", "response", 200, "referer", "www.google.com")), + ExprValueUtils.tupleValue(ImmutableMap.of("ip", "74.125.19.106", + "action", "POST", "response", 500)) + )); + } +} diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java index cd561f3c09..4f38708f89 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java @@ -191,6 +191,11 @@ public String visitLimit(LimitOperator node, Integer tabs) { return name(node, "Limit->", tabs); } + @Override + public String visitHighlight(HighlightOperator node, Integer tabs) { + return name(node, "Highlight->", tabs); + } + private String name(PhysicalPlan node, String current, int tabs) { String child = node.getChild().get(0).accept(this, tabs + 1); StringBuilder sb = new StringBuilder(); diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java index db536b93bf..c5bcd24fad 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java @@ -1,3 +1,8 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + package org.opensearch.sql.sql; import org.json.JSONObject; @@ -5,6 +10,9 @@ import org.opensearch.sql.legacy.SQLIntegTestCase; import org.opensearch.sql.legacy.TestsConstants; +import static org.opensearch.sql.util.MatcherUtils.schema; +import static org.opensearch.sql.util.MatcherUtils.verifySchema; + public class HighlightFunctionIT extends SQLIntegTestCase { @Override @@ -13,9 +21,50 @@ protected void init() throws Exception { } @Test - public void defaultParameters() { - String query = "SELECT Tags, highlight('Tags') from %s WHERE match(Tags, 'yeast') limit 1"; + public void single_highlight_test() { + String query = "SELECT Tags, highlight('Tags') FROM %s WHERE match(Tags, 'yeast') LIMIT 1"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); + verifySchema(response, schema("Tags", null, "text"), schema("highlight('Tags')", null, "keyword")); + assertEquals(1, response.getInt("total")); + } + + @Test + public void accepts_unquoted_test() { + String query = "SELECT Tags, highlight(Tags) FROM %s WHERE match(Tags, 'yeast') LIMIT 1"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); + verifySchema(response, schema("Tags", null, "text"), schema("highlight(Tags)", null, "keyword")); + assertEquals(1, response.getInt("total")); + } + + @Test + public void multiple_highlight_test() { + String query = "SELECT highlight(Title), highlight(Body) FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); + verifySchema(response, schema("highlight(Title)", null, "keyword"), schema("highlight(Body)", null, "keyword")); + assertEquals(1, response.getInt("total")); + } + + @Test + public void wildcard_highlight_test() { + String query = "SELECT highlight('*itle') FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); + verifySchema(response, schema("highlight('*itle')", null, "keyword")); assertEquals(1, response.getInt("total")); } + + @Test + public void highlight_all_test() { + String query = "SELECT highlight('*') FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); + verifySchema(response, schema("highlight('*')", null, "keyword")); + assertEquals(1, response.getInt("total")); + } + + @Test + public void highlight_no_limit_test() { + String query = "SELECT highlight('*') FROM %s WHERE MULTI_MATCH([Title, Body], 'hops')"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); + verifySchema(response, schema("highlight('*')", null, "keyword")); + assertEquals(225, response.getInt("total")); + } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScan.java index 8b23daad1c..d182b5f84d 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScan.java @@ -58,10 +58,6 @@ public class OpenSearchLogicalIndexScan extends LogicalPlan { @Setter private Integer limit; - - @Setter - private String highlightField; - /** * ElasticsearchLogicalIndexScan Constructor. */ @@ -71,7 +67,7 @@ public OpenSearchLogicalIndexScan( Expression filter, Set projectList, List> sortList, - Integer limit, Integer offset, String highlightField) { + Integer limit, Integer offset) { super(ImmutableList.of()); this.relationName = relationName; this.filter = filter; @@ -79,7 +75,6 @@ public OpenSearchLogicalIndexScan( this.sortList = sortList; this.limit = limit; this.offset = offset; - this.highlightField = highlightField; } @Override @@ -91,10 +86,6 @@ public boolean hasLimit() { return limit != null; } - public boolean hasHighlight() { - return highlightField != null; - } - /** * Test has projects or not. * diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeLimitAndIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeLimitAndIndexScan.java index fb8aba2c90..9d880bb4dc 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeLimitAndIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeLimitAndIndexScan.java @@ -45,8 +45,7 @@ public LogicalPlan apply(LogicalLimit plan, Captures captures) { builder.relationName(indexScan.getRelationName()) .filter(indexScan.getFilter()) .offset(plan.getOffset()) - .limit(plan.getLimit()) - .highlightField(indexScan.getHighlightField()); + .limit(plan.getLimit()); if (indexScan.getSortList() != null) { builder.sortList(indexScan.getSortList()); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexScan.java index 46324b9898..337f09308c 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexScan.java @@ -56,7 +56,6 @@ public LogicalPlan apply(LogicalSort sort, .relationName(indexScan.getRelationName()) .filter(indexScan.getFilter()) .sortList(mergeSortList(indexScan.getSortList(), sort.getSortList())) - .highlightField(indexScan.getHighlightField()) .build(); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java index 5b306cb9dc..8d0f09ddd1 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java @@ -23,7 +23,6 @@ import org.opensearch.sql.opensearch.planner.logical.OpenSearchLogicalIndexScan; import org.opensearch.sql.opensearch.planner.logical.OpenSearchLogicalPlanOptimizerFactory; import org.opensearch.sql.opensearch.planner.physical.ADOperator; -import org.opensearch.sql.planner.physical.HighlightOperator; import org.opensearch.sql.opensearch.planner.physical.MLCommonsOperator; import org.opensearch.sql.opensearch.request.OpenSearchRequest; import org.opensearch.sql.opensearch.request.system.OpenSearchDescribeIndexRequest; @@ -38,6 +37,7 @@ import org.opensearch.sql.planner.logical.LogicalMLCommons; import org.opensearch.sql.planner.logical.LogicalPlan; import org.opensearch.sql.planner.logical.LogicalRelation; +import org.opensearch.sql.planner.physical.HighlightOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.storage.Table; @@ -122,7 +122,6 @@ public PhysicalPlan visitNode(LogicalPlan plan, OpenSearchIndexScan context) { } } - /** * Implement ElasticsearchLogicalIndexScan. */ @@ -148,11 +147,6 @@ public PhysicalPlan visitIndexScan(OpenSearchLogicalIndexScan node, if (node.hasProjects()) { context.pushDownProjects(node.getProjectList()); } - - if (node.hasHighlight()) { - context.pushDownHighlight(node.getHighlightField()); - } - return indexScan; } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java index 2a3cc05f15..c93b4ef6b5 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java @@ -166,11 +166,12 @@ public void pushDownLimit(Integer limit, Integer offset) { */ public void pushDownHighlight(String field) { SearchSourceBuilder sourceBuilder = request.getSourceBuilder(); - if(sourceBuilder.highlighter() != null) { + if (sourceBuilder.highlighter() != null) { sourceBuilder.highlighter().field(StringUtils.unquoteText(field)); sourceBuilder.highlighter(sourceBuilder.highlighter()); } else { - HighlightBuilder highlightBuilder = new HighlightBuilder().field(StringUtils.unquoteText(field)); + HighlightBuilder highlightBuilder = + new HighlightBuilder().field(StringUtils.unquoteText(field)); sourceBuilder.highlighter(highlightBuilder); } } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java index 52e76a602b..dc598b4413 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java @@ -22,15 +22,4 @@ void has_projects() { assertFalse(OpenSearchLogicalIndexScan.builder().build().hasProjects()); } - - @Test - void has_highlight() { - assertTrue( - OpenSearchLogicalIndexScan.builder().highlightField("fieldA").build().hasHighlight()); - } - - @Test - void no_highlight_by_default() { - assertFalse(OpenSearchLogicalIndexScan.builder().build().hasHighlight()); - } } diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index dde02cfd3e..a9316b55a4 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -305,7 +305,6 @@ functionCall highlightFunction : HIGHLIGHT LR_BRACKET relevanceField RR_BRACKET - | HIGHLIGHT LR_BRACKET STAR RR_BRACKET ; scalarFunctionName diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java index 558cb58e37..d88b965368 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java @@ -1,10 +1,32 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + package org.opensearch.sql.sql.antlr; import org.junit.jupiter.api.Test; -public class HighlightTest extends SQLParserTest { +public class HighlightTest extends SQLParserTest { @Test - void single_field() { + void single_field_test() { acceptQuery("SELECT HIGHLIGHT(Tags) FROM Index WHERE MATCH(Tags, 'Time')"); } + + @Test + void multiple_highlights_test() { + acceptQuery("SELECT HIGHLIGHT(Tags), HIGHLIGHT(Body) FROM Index " + + "WHERE MULTI_MATCH([Tags, Body], 'Time')"); + } + + @Test + void wildcard_test() { + acceptQuery("SELECT HIGHLIGHT('T*') FROM Index " + + "WHERE MULTI_MATCH([Tags, Body], 'Time')"); + } + + @Test + void highlight_all_test() { + acceptQuery("SELECT HIGHLIGHT('*') FROM Index WHERE MULTI_MATCH([Tags, Body], 'Time')"); + } } From b2a90d4735ed610f2ac78ac961b5142042e2d515 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Wed, 27 Jul 2022 06:50:57 -0700 Subject: [PATCH 17/24] Added HighlightExpressionTest Signed-off-by: forestmvey --- .../sql/analysis/HighlightExpression.java | 1 - .../expression/HighlightExpressionTest.java | 48 +++++++++++++++++++ .../OpenSearchLogicalIndexScanTest.java | 1 - 3 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java index 9e100cdbba..cdd0d1896f 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java @@ -57,7 +57,6 @@ public ExprValue valueOf(Environment valueEnv) { var hlBuilder = ImmutableMap.builder(); hlBuilder.putAll(retVal.tupleValue()); - for (var entry : retVal.tupleValue().entrySet()) { String entryKey = "highlight(" + getHighlightField() + ")." + entry.getKey(); builder.put(entryKey, ExprValueUtils.stringValue(entry.getValue().toString())); diff --git a/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java b/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java new file mode 100644 index 0000000000..1b00f768e6 --- /dev/null +++ b/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java @@ -0,0 +1,48 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.expression; + +import com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.analysis.HighlightExpression; +import org.opensearch.sql.data.model.ExprTupleValue; +import org.opensearch.sql.data.model.ExprValue; +import org.opensearch.sql.data.model.ExprValueUtils; +import org.opensearch.sql.expression.env.Environment; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.opensearch.sql.data.type.ExprCoreType.STRING; +import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; + +public class HighlightExpressionTest extends ExpressionTestBase { + + @Test + public void single_highlight_test() { + Environment tmp = ExprValueUtils.tupleValue( + ImmutableMap.of("_highlight.Title", "result value")).bindingTuples(); + HighlightExpression expr = new HighlightExpression(DSL.literal("Title")); + ExprValue resultVal = expr.valueOf(tmp); + + assertEquals(STRING, resultVal.type()); + assertEquals("result value", resultVal.stringValue()); + } + + @Test + public void highlight_all_test() { + ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); + var hlBuilder = ImmutableMap.builder(); + hlBuilder.put("Title", ExprValueUtils.stringValue("correct result value")); + hlBuilder.put("Body", ExprValueUtils.stringValue("incorrect result value")); + builder.put("_highlight", ExprTupleValue.fromExprValueMap(hlBuilder.build())); + + HighlightExpression expr = new HighlightExpression(DSL.literal("*")); + ExprValue resultVal = expr.valueOf(ExprTupleValue.fromExprValueMap(builder.build()).bindingTuples()); + assertEquals(STRUCT, resultVal.type()); + assertTrue(resultVal.tupleValue().containsValue(ExprValueUtils.stringValue("\"correct result value\""))); + assertTrue(resultVal.tupleValue().containsValue(ExprValueUtils.stringValue("\"correct result value\""))); + } +} diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java index dc598b4413..2e10f33787 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java @@ -7,7 +7,6 @@ package org.opensearch.sql.opensearch.planner.logical; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.ImmutableSet; import org.junit.jupiter.api.Test; From 2c04c49e0fd766bc533a3a90914e28da471d531c Mon Sep 17 00:00:00 2001 From: forestmvey Date: Thu, 28 Jul 2022 16:05:57 -0700 Subject: [PATCH 18/24] Added javadocs, minor PR revisions, and fixed jacoco errors by improving coverage for OpenSearchIndexScan Signed-off-by: forestmvey --- .../sql/analysis/ExpressionAnalyzer.java | 1 + .../sql/analysis/HighlightAnalyzer.java | 6 ++- .../sql/ast/expression/HighlightFunction.java | 3 ++ .../sql/expression/ExpressionNodeVisitor.java | 1 - .../HighlightExpression.java | 19 ++++++--- .../function/OpenSearchFunctions.java | 2 +- .../sql/planner/physical/PhysicalPlanDSL.java | 4 ++ .../opensearch/sql/analysis/AnalyzerTest.java | 5 ++- .../sql/analysis/ExpressionAnalyzerTest.java | 2 +- .../expression/ExpressionNodeVisitorTest.java | 7 +--- .../expression/HighlightExpressionTest.java | 40 +++++++++++++------ .../physical/PhysicalPlanNodeVisitorTest.java | 9 ++--- .../sql/sql/HighlightFunctionIT.java | 23 ++++++++--- .../OpenSearchExecutionProtectorTest.java | 12 +++++- .../response/OpenSearchResponseTest.java | 37 ++++++++++++++++- .../OpenSearchDefaultImplementorTest.java | 11 +++++ .../storage/OpenSearchIndexScanTest.java | 27 +++++++++++++ 17 files changed, 165 insertions(+), 44 deletions(-) rename core/src/main/java/org/opensearch/sql/{analysis => expression}/HighlightExpression.java (85%) diff --git a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java index f97b0803a9..670da5c85c 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java @@ -48,6 +48,7 @@ import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.DSL; import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.expression.HighlightExpression; import org.opensearch.sql.expression.LiteralExpression; import org.opensearch.sql.expression.NamedArgumentExpression; import org.opensearch.sql.expression.NamedExpression; diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java index 61ba7d66f5..06a601327c 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/HighlightAnalyzer.java @@ -14,8 +14,12 @@ import org.opensearch.sql.planner.logical.LogicalHighlight; import org.opensearch.sql.planner.logical.LogicalPlan; +/** + * Analyze the highlight in the {@link AnalysisContext} to construct the {@link + * LogicalPlan}. + */ @RequiredArgsConstructor -public class HighlightAnalyzer extends AbstractNodeVisitor { +public class HighlightAnalyzer extends AbstractNodeVisitor { private final ExpressionAnalyzer expressionAnalyzer; private final LogicalPlan child; diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java index eae1525aae..5f1bb652d9 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java @@ -12,6 +12,9 @@ import lombok.ToString; import org.opensearch.sql.ast.AbstractNodeVisitor; +/** + * Expression node of Highlight function. + */ @AllArgsConstructor @EqualsAndHashCode(callSuper = false) @Getter diff --git a/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java b/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java index 45dcdde012..d53371dd58 100644 --- a/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java @@ -6,7 +6,6 @@ package org.opensearch.sql.expression; -import org.opensearch.sql.analysis.HighlightExpression; import org.opensearch.sql.expression.aggregation.Aggregator; import org.opensearch.sql.expression.aggregation.NamedAggregator; import org.opensearch.sql.expression.conditional.cases.CaseClause; diff --git a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java similarity index 85% rename from core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java rename to core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java index cdd0d1896f..d623dd6393 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -package org.opensearch.sql.analysis; +package org.opensearch.sql.expression; import com.google.common.collect.ImmutableMap; import java.util.List; @@ -22,9 +22,10 @@ import org.opensearch.sql.expression.env.Environment; import org.opensearch.sql.expression.function.BuiltinFunctionName; -@EqualsAndHashCode(callSuper = false) +/** + * Highlight Expression. + */ @Getter -@ToString public class HighlightExpression extends FunctionExpression { private final Expression highlightField; @@ -49,14 +50,15 @@ public ExprValue valueOf(Environment valueEnv) { refName += "." + StringUtils.unquoteText(getHighlightField().toString()); } ExprValue retVal = valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); - ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); + // If only one highlight returned, or no highlights can be parsed. if (retVal.isMissing() || retVal.type() != ExprCoreType.STRUCT) { return retVal; } - var hlBuilder = ImmutableMap.builder(); - hlBuilder.putAll(retVal.tupleValue()); + var highlightMapBuilder = ImmutableMap.builder(); + highlightMapBuilder.putAll(retVal.tupleValue()); + ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); for (var entry : retVal.tupleValue().entrySet()) { String entryKey = "highlight(" + getHighlightField() + ")." + entry.getKey(); builder.put(entryKey, ExprValueUtils.stringValue(entry.getValue().toString())); @@ -73,4 +75,9 @@ public ExprValue valueOf(Environment valueEnv) { public ExprType type() { return ExprCoreType.STRING; } + + @Override + public T accept(ExpressionNodeVisitor visitor, C context) { + return visitor.visitHighlight(this, context); + } } diff --git a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java index d330f35542..f7d922cd9b 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java @@ -15,12 +15,12 @@ import java.util.Map; import java.util.stream.Collectors; import lombok.experimental.UtilityClass; -import org.opensearch.sql.analysis.HighlightExpression; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.expression.Expression; import org.opensearch.sql.expression.FunctionExpression; +import org.opensearch.sql.expression.HighlightExpression; import org.opensearch.sql.expression.NamedArgumentExpression; import org.opensearch.sql.expression.env.Environment; diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java index e6e59990c8..f5535f64d1 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java @@ -105,4 +105,8 @@ public ValuesOperator values(List... values) { public static LimitOperator limit(PhysicalPlan input, Integer limit, Integer offset) { return new LimitOperator(input, limit, offset); } + + public static HighlightOperator highlight(PhysicalPlan input, Expression highlightField) { + return new HighlightOperator(input, highlightField); + } } diff --git a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java index 28598e35a6..b9de96b30a 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java @@ -54,6 +54,7 @@ import org.opensearch.sql.ast.tree.RareTopN.CommandType; import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.DSL; +import org.opensearch.sql.expression.HighlightExpression; import org.opensearch.sql.expression.config.ExpressionConfig; import org.opensearch.sql.expression.window.WindowDefinition; import org.opensearch.sql.planner.logical.LogicalAD; @@ -292,7 +293,7 @@ public void project_values() { AstDSL.project( AstDSL.values(ImmutableList.of(AstDSL.intLiteral(123))), AstDSL.alias("123", AstDSL.intLiteral(123)), - AstDSL.alias("hello", stringLiteral("hello")), + AstDSL.alias("hello", AstDSL.stringLiteral("hello")), AstDSL.alias("false", AstDSL.booleanLiteral(false)) ) ); @@ -712,7 +713,7 @@ public void parse_relation() { AstDSL.parse( AstDSL.relation("schema"), AstDSL.field("string_value"), - stringLiteral("(?.*)")), + AstDSL.stringLiteral("(?.*)")), AstDSL.alias("string_value", qualifiedName("string_value")) )); } diff --git a/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java index 668af4263d..72db402552 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java @@ -45,7 +45,7 @@ import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.DSL; import org.opensearch.sql.expression.Expression; -import org.opensearch.sql.expression.LiteralExpression; +import org.opensearch.sql.expression.HighlightExpression; import org.opensearch.sql.expression.config.ExpressionConfig; import org.opensearch.sql.expression.window.aggregation.AggregateWindowFunction; import org.springframework.context.annotation.Configuration; diff --git a/core/src/test/java/org/opensearch/sql/expression/ExpressionNodeVisitorTest.java b/core/src/test/java/org/opensearch/sql/expression/ExpressionNodeVisitorTest.java index 3cebf4aaef..b0b2bc5b2b 100644 --- a/core/src/test/java/org/opensearch/sql/expression/ExpressionNodeVisitorTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/ExpressionNodeVisitorTest.java @@ -20,7 +20,6 @@ import org.junit.jupiter.api.DisplayNameGeneration; import org.junit.jupiter.api.DisplayNameGenerator; import org.junit.jupiter.api.Test; -import org.opensearch.sql.analysis.HighlightExpression; import org.opensearch.sql.expression.aggregation.Aggregator; import org.opensearch.sql.expression.aggregation.AvgAggregator; import org.opensearch.sql.expression.conditional.cases.CaseClause; @@ -35,6 +34,7 @@ class ExpressionNodeVisitorTest { @Test void should_return_null_by_default() { ExpressionNodeVisitor visitor = new ExpressionNodeVisitor(){}; + assertNull(new HighlightExpression(DSL.literal("Title")).accept(visitor, null)); assertNull(literal(10).accept(visitor, null)); assertNull(ref("name", STRING).accept(visitor, null)); assertNull(named("bool", literal(true)).accept(visitor, null)); @@ -89,11 +89,6 @@ public Expression visitAggregator(Aggregator node, Object context) { return dsl.sum(visitArguments(node.getArguments(), context)); } - @Override - public Expression visitHighlight(HighlightExpression node, Object context) { - return node; - } - private Expression[] visitArguments(List arguments, Object context) { return arguments.stream() .map(arg -> arg.accept(this, context)) diff --git a/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java b/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java index 1b00f768e6..a63e079418 100644 --- a/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java @@ -5,32 +5,40 @@ package org.opensearch.sql.expression; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; + import com.google.common.collect.ImmutableMap; import org.junit.jupiter.api.Test; -import org.opensearch.sql.analysis.HighlightExpression; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.expression.env.Environment; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.opensearch.sql.data.type.ExprCoreType.STRING; -import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; - public class HighlightExpressionTest extends ExpressionTestBase { @Test public void single_highlight_test() { - Environment tmp = ExprValueUtils.tupleValue( + Environment hlTuple = ExprValueUtils.tupleValue( ImmutableMap.of("_highlight.Title", "result value")).bindingTuples(); HighlightExpression expr = new HighlightExpression(DSL.literal("Title")); - ExprValue resultVal = expr.valueOf(tmp); + ExprValue resultVal = expr.valueOf(hlTuple); - assertEquals(STRING, resultVal.type()); + assertEquals(expr.type(), resultVal.type()); assertEquals("result value", resultVal.stringValue()); } + @Test + public void missing_highlight_test() { + Environment hlTuple = ExprValueUtils.tupleValue( + ImmutableMap.of("_highlight.Title", "result value")).bindingTuples(); + HighlightExpression expr = new HighlightExpression(DSL.literal("invalid")); + ExprValue resultVal = expr.valueOf(hlTuple); + + assertTrue(resultVal.isMissing()); + } + @Test public void highlight_all_test() { ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); @@ -39,10 +47,16 @@ public void highlight_all_test() { hlBuilder.put("Body", ExprValueUtils.stringValue("incorrect result value")); builder.put("_highlight", ExprTupleValue.fromExprValueMap(hlBuilder.build())); - HighlightExpression expr = new HighlightExpression(DSL.literal("*")); - ExprValue resultVal = expr.valueOf(ExprTupleValue.fromExprValueMap(builder.build()).bindingTuples()); + HighlightExpression hlExpr = new HighlightExpression(DSL.literal("*")); + ExprValue resultVal = hlExpr.valueOf( + ExprTupleValue.fromExprValueMap(builder.build()).bindingTuples()); assertEquals(STRUCT, resultVal.type()); - assertTrue(resultVal.tupleValue().containsValue(ExprValueUtils.stringValue("\"correct result value\""))); - assertTrue(resultVal.tupleValue().containsValue(ExprValueUtils.stringValue("\"correct result value\""))); + for (var field : resultVal.tupleValue().entrySet()) { + assertTrue(field.toString().contains(hlExpr.getHighlightField().toString())); + } + assertTrue(resultVal.tupleValue().containsValue( + ExprValueUtils.stringValue("\"correct result value\""))); + assertTrue(resultVal.tupleValue().containsValue( + ExprValueUtils.stringValue("\"correct result value\""))); } } diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java index 4f38708f89..e721444399 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java @@ -120,6 +120,10 @@ public void test_PhysicalPlanVisitor_should_return_null() { assertNull(dedupe.accept(new PhysicalPlanNodeVisitor() { }, null)); + PhysicalPlan highlight = PhysicalPlanDSL.highlight(plan, ref); + assertNull(highlight.accept(new PhysicalPlanNodeVisitor() { + }, null)); + PhysicalPlan values = PhysicalPlanDSL.values(emptyList()); assertNull(values.accept(new PhysicalPlanNodeVisitor() { }, null)); @@ -191,11 +195,6 @@ public String visitLimit(LimitOperator node, Integer tabs) { return name(node, "Limit->", tabs); } - @Override - public String visitHighlight(HighlightOperator node, Integer tabs) { - return name(node, "Highlight->", tabs); - } - private String name(PhysicalPlan node, String current, int tabs) { String child = node.getChild().get(0).accept(this, tabs + 1); StringBuilder sb = new StringBuilder(); diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java index c5bcd24fad..871285f454 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java @@ -5,14 +5,14 @@ package org.opensearch.sql.sql; +import static org.opensearch.sql.util.MatcherUtils.schema; +import static org.opensearch.sql.util.MatcherUtils.verifySchema; + import org.json.JSONObject; import org.junit.Test; import org.opensearch.sql.legacy.SQLIntegTestCase; import org.opensearch.sql.legacy.TestsConstants; -import static org.opensearch.sql.util.MatcherUtils.schema; -import static org.opensearch.sql.util.MatcherUtils.verifySchema; - public class HighlightFunctionIT extends SQLIntegTestCase { @Override @@ -52,6 +52,17 @@ public void wildcard_highlight_test() { assertEquals(1, response.getInt("total")); } + @Test + public void wildcard_multi_field_highlight_test() { + String query = "SELECT highlight('T*') FROM %s WHERE MULTI_MATCH([Title, Tags], 'hops') LIMIT 1"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); + verifySchema(response, schema("highlight('T*')", null, "keyword")); + var resultMap = response.getJSONArray("datarows").getJSONArray(0).getJSONObject(0); + assertEquals(1, response.getInt("total")); + assertTrue(resultMap.has("highlight(\"T*\").Title")); + assertTrue(resultMap.has("highlight(\"T*\").Tags")); + } + @Test public void highlight_all_test() { String query = "SELECT highlight('*') FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; @@ -62,9 +73,9 @@ public void highlight_all_test() { @Test public void highlight_no_limit_test() { - String query = "SELECT highlight('*') FROM %s WHERE MULTI_MATCH([Title, Body], 'hops')"; + String query = "SELECT highlight(Body) FROM %s WHERE MATCH(Body, 'hops')"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); - verifySchema(response, schema("highlight('*')", null, "keyword")); - assertEquals(225, response.getInt("total")); + verifySchema(response, schema("highlight(Body)", null, "keyword")); + assertEquals(2, response.getInt("total")); } } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java index 5bffa1cfa8..356036cb66 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java @@ -36,7 +36,6 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.opensearch.client.node.NodeClient; -import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.expression.DataType; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.tree.RareTopN.CommandType; @@ -59,6 +58,7 @@ import org.opensearch.sql.opensearch.planner.physical.MLCommonsOperator; import org.opensearch.sql.opensearch.setting.OpenSearchSettings; import org.opensearch.sql.opensearch.storage.OpenSearchIndexScan; +import org.opensearch.sql.planner.physical.HighlightOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.planner.physical.PhysicalPlanDSL; @@ -291,6 +291,16 @@ public void testVisitAD() { executionProtector.visitAD(adOperator, null)); } + @Test + public void testVisitHighlight() { + HighlightOperator hl = + new HighlightOperator( + values(emptyList()), DSL.ref("*", STRING)); + + assertEquals(executionProtector.doProtect(hl), + executionProtector.visitHighlight(hl, null)); + } + PhysicalPlan resourceMonitor(PhysicalPlan input) { return new ResourceMonitorPlan(input, resourceMonitor); } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java index fa25b4f408..ba244bacc6 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java @@ -17,18 +17,23 @@ import com.google.common.collect.ImmutableMap; import java.util.Arrays; +import java.util.Map; import org.apache.lucene.search.TotalHits; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.opensearch.action.search.SearchResponse; +import org.opensearch.common.bytes.BytesArray; +import org.opensearch.common.text.Text; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; import org.opensearch.search.aggregations.Aggregations; +import org.opensearch.search.fetch.subphase.highlight.HighlightField; import org.opensearch.sql.data.model.ExprIntegerValue; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; +import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.opensearch.data.value.OpenSearchExprValueFactory; import org.opensearch.sql.opensearch.response.agg.OpenSearchAggregationResponseParser; @@ -148,4 +153,34 @@ void aggregation_iterator() { i++; } } -} + + @Test + void highlight_iterator() { + SearchHit searchHit = new SearchHit(1); + searchHit.sourceRef( + new BytesArray("{\"name\":\"John\"}")); + Map highlightMap = Map.of("highlights", + new HighlightField("Title", new Text[] {new Text("field")})); + searchHit.highlightFields(Map.of("highlights", new HighlightField("Title", + new Text[] {new Text("field")}))); + ExprValue resultTuple = ExprValueUtils.tupleValue(searchHit.getSourceAsMap()); + + when(searchResponse.getHits()) + .thenReturn( + new SearchHits( + new SearchHit[]{searchHit1}, + new TotalHits(1L, TotalHits.Relation.EQUAL_TO), + 1.0F)); + + when(searchHit1.getHighlightFields()).thenReturn(highlightMap); + when(factory.construct(any())).thenReturn(resultTuple); + + for (ExprValue resultHit : new OpenSearchResponse(searchResponse, factory)) { + var expected = ExprValueUtils.stringValue( + searchHit.getHighlightFields().get("highlights").toString()); + var result = resultHit.tupleValue().get( + "_highlight").tupleValue().get("highlights"); + assertTrue(expected.equals(result)); + } + } +} \ No newline at end of file diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchDefaultImplementorTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchDefaultImplementorTest.java index 0770ea3938..b3bed007cd 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchDefaultImplementorTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchDefaultImplementorTest.java @@ -19,6 +19,7 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.opensearch.sql.opensearch.client.OpenSearchClient; import org.opensearch.sql.planner.logical.LogicalAD; +import org.opensearch.sql.planner.logical.LogicalHighlight; import org.opensearch.sql.planner.logical.LogicalMLCommons; import org.opensearch.sql.planner.logical.LogicalPlan; @@ -67,4 +68,14 @@ public void visitAD() { new OpenSearchIndex.OpenSearchDefaultImplementor(indexScan, client); assertNotNull(implementor.visitAD(node, indexScan)); } + + @Test + public void visitHighlight() { + LogicalHighlight node = Mockito.mock(LogicalHighlight.class, + Answers.RETURNS_DEEP_STUBS); + Mockito.when(node.getChild().get(0)).thenReturn(Mockito.mock(LogicalPlan.class)); + OpenSearchIndex.OpenSearchDefaultImplementor implementor = + new OpenSearchIndex.OpenSearchDefaultImplementor(indexScan, client); + assertNotNull(implementor.visitHighlight(node, indexScan)); + } } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScanTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScanTest.java index 429c639da9..41769914d9 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScanTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScanTest.java @@ -31,6 +31,7 @@ import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.QueryBuilders; import org.opensearch.search.SearchHit; +import org.opensearch.search.fetch.subphase.highlight.HighlightBuilder; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; @@ -110,6 +111,16 @@ void pushDownFilters() { .filter(QueryBuilders.rangeQuery("balance").gte(10000))); } + @Test + void pushDownHighlight() { + assertThat() + .pushDown(QueryBuilders.termQuery("name", "John")) + .pushDownHighlight("Title") + .pushDownHighlight("Body") + .shouldQueryHighlight(QueryBuilders.termQuery("name", "John"), + new HighlightBuilder().field("Title").field("Body")); + } + private PushDownAssertion assertThat() { return new PushDownAssertion(client, exprValueFactory, settings); } @@ -135,6 +146,22 @@ PushDownAssertion pushDown(QueryBuilder query) { return this; } + PushDownAssertion pushDownHighlight(String query) { + indexScan.pushDownHighlight(query); + return this; + } + + PushDownAssertion shouldQueryHighlight(QueryBuilder query, HighlightBuilder highlight) { + OpenSearchRequest request = new OpenSearchQueryRequest("test", 200, factory); + request.getSourceBuilder() + .query(query) + .highlighter(highlight) + .sort(DOC_FIELD_NAME, ASC); + when(client.search(request)).thenReturn(response); + indexScan.open(); + return this; + } + PushDownAssertion shouldQuery(QueryBuilder expected) { OpenSearchRequest request = new OpenSearchQueryRequest("test", 200, factory); request.getSourceBuilder() From 01708e3a0d135cf57ffa7d99504137314be4719f Mon Sep 17 00:00:00 2001 From: forestmvey Date: Fri, 29 Jul 2022 11:50:54 -0700 Subject: [PATCH 19/24] Code cleanup, adding parsing failure tests, and adding tests for highlight acceptance as a string literal as well as qualified name. Signed-off-by: forestmvey --- .../function/OpenSearchFunctions.java | 1 + .../physical/HighlightOperatorTest.java | 2 +- .../sql/sql/HighlightFunctionIT.java | 9 ++++--- .../response/OpenSearchResponseTest.java | 2 +- .../sql/sql/antlr/HighlightTest.java | 13 ++++++++++ .../sql/sql/parser/AstBuilderTest.java | 25 +++++++++++++------ 6 files changed, 40 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java index f7d922cd9b..c3e5cc5594 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java @@ -15,6 +15,7 @@ import java.util.Map; import java.util.stream.Collectors; import lombok.experimental.UtilityClass; +import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java index 349820dada..55745d51f4 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java @@ -20,7 +20,7 @@ public class HighlightOperatorTest extends PhysicalPlanTestBase { @Test - public void highlight_all_test() { + public void valid_highlight_operator_test() { PhysicalPlan plan = new HighlightOperator(new TestScan(), DSL.ref("*", STRING)); List result = execute(plan); assertEquals(5, result.size()); diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java index 871285f454..1c171d7bb1 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java @@ -24,7 +24,8 @@ protected void init() throws Exception { public void single_highlight_test() { String query = "SELECT Tags, highlight('Tags') FROM %s WHERE match(Tags, 'yeast') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); - verifySchema(response, schema("Tags", null, "text"), schema("highlight('Tags')", null, "keyword")); + verifySchema(response, schema("Tags", null, "text"), + schema("highlight('Tags')", null, "keyword")); assertEquals(1, response.getInt("total")); } @@ -32,7 +33,8 @@ public void single_highlight_test() { public void accepts_unquoted_test() { String query = "SELECT Tags, highlight(Tags) FROM %s WHERE match(Tags, 'yeast') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); - verifySchema(response, schema("Tags", null, "text"), schema("highlight(Tags)", null, "keyword")); + verifySchema(response, schema("Tags", null, "text"), + schema("highlight(Tags)", null, "keyword")); assertEquals(1, response.getInt("total")); } @@ -40,7 +42,8 @@ public void accepts_unquoted_test() { public void multiple_highlight_test() { String query = "SELECT highlight(Title), highlight(Body) FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); - verifySchema(response, schema("highlight(Title)", null, "keyword"), schema("highlight(Body)", null, "keyword")); + verifySchema(response, schema("highlight(Title)", null, "keyword"), + schema("highlight(Body)", null, "keyword")); assertEquals(1, response.getInt("total")); } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java index ba244bacc6..2ad41c7a63 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java @@ -183,4 +183,4 @@ void highlight_iterator() { assertTrue(expected.equals(result)); } } -} \ No newline at end of file +} diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java index d88b965368..6826a37c0b 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java @@ -27,6 +27,19 @@ void wildcard_test() { @Test void highlight_all_test() { + acceptQuery("SELECT HIGHLIGHT('*') FROM Index WHERE MULTI_MATCH([Tags, Body], 'Time')"); } + + @Test + void multiple_parameters_failure_test() { + rejectQuery("SELECT HIGHLIGHT(Tags1, Tags2) FROM Index " + + "WHERE MULTI_MATCH([Tags, Body], 'Time')"); + } + + @Test + void no_parameters_failure_test() { + rejectQuery("SELECT HIGHLIGHT() FROM Index " + + "WHERE MULTI_MATCH([Tags, Body], 'Time')"); + } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java index a2a27bb2df..9f2f8b5f92 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java @@ -616,12 +616,14 @@ public void describe_and_column_compatible_with_old_engine_syntax() { @Test public void can_build_alias_by_keywords() { + var expected = project( + relation("test"), + alias("avg_age", qualifiedName("avg_age"), "avg") + ); + var comp = buildAST("SELECT avg_age AS avg FROM test"); assertEquals( - project( - relation("test"), - alias("avg_age", qualifiedName("avg_age"), "avg") - ), - buildAST("SELECT avg_age AS avg FROM test") + expected, + comp ); } @@ -670,14 +672,23 @@ public void can_build_limit_clause_with_offset() { } @Test - public void can_build_highlight() { + public void can_build_qualified_name_highlight() { assertEquals( project(relation("test"), - alias("highlight(fieldA)", highlight(AstDSL.stringLiteral("fieldA")))), + alias("highlight(fieldA)", highlight(AstDSL.qualifiedName("fieldA")))), buildAST("SELECT highlight(fieldA) FROM test") ); } + @Test + public void can_build_string_literal_highlight() { + assertEquals( + project(relation("test"), + alias("highlight(\"fieldA\")", highlight(AstDSL.stringLiteral("fieldA")))), + buildAST("SELECT highlight(\"fieldA\") FROM test") + ); + } + private UnresolvedPlan buildAST(String query) { ParseTree parseTree = parser.parse(query); return parseTree.accept(new AstBuilder(query)); From 6b96ac04e559160611cd9269fbf08dae02dd372a Mon Sep 17 00:00:00 2001 From: forestmvey Date: Fri, 29 Jul 2022 16:29:39 -0700 Subject: [PATCH 20/24] Removing HighlightOperator functionality and unnecessary visitHighlight call in PhysicalPlanNodeVisitor.. Signed-off-by: forestmvey --- .../sql/expression/HighlightExpression.java | 5 -- .../planner/physical/HighlightOperator.java | 52 ------------------- .../sql/planner/physical/PhysicalPlanDSL.java | 4 -- .../physical/PhysicalPlanNodeVisitor.java | 4 -- .../analysis/NamedExpressionAnalyzerTest.java | 16 ++++++ .../physical/HighlightOperatorTest.java | 40 -------------- .../physical/PhysicalPlanNodeVisitorTest.java | 4 -- .../OpenSearchExecutionProtector.java | 8 --- .../opensearch/storage/OpenSearchIndex.java | 3 +- .../OpenSearchExecutionProtectorTest.java | 11 ---- .../OpenSearchDefaultImplementorTest.java | 6 ++- .../sql/sql/parser/AstBuilderTest.java | 12 ++--- .../sql/parser/AstExpressionBuilderTest.java | 10 +++- 13 files changed, 36 insertions(+), 139 deletions(-) delete mode 100644 core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java delete mode 100644 core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java diff --git a/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java index d623dd6393..240ec1ea8e 100644 --- a/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java @@ -7,18 +7,13 @@ import com.google.common.collect.ImmutableMap; import java.util.List; -import lombok.EqualsAndHashCode; import lombok.Getter; -import lombok.ToString; import org.opensearch.sql.common.utils.StringUtils; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; -import org.opensearch.sql.expression.DSL; -import org.opensearch.sql.expression.Expression; -import org.opensearch.sql.expression.FunctionExpression; import org.opensearch.sql.expression.env.Environment; import org.opensearch.sql.expression.function.BuiltinFunctionName; diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java deleted file mode 100644 index 5618f89a5c..0000000000 --- a/core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java +++ /dev/null @@ -1,52 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.planner.physical; - -import com.google.common.collect.ImmutableMap; -import java.util.Collections; -import java.util.List; -import lombok.EqualsAndHashCode; -import lombok.Getter; -import lombok.NonNull; -import org.opensearch.sql.data.model.ExprValue; -import org.opensearch.sql.expression.Expression; - -@EqualsAndHashCode(callSuper = false) -@Getter -public class HighlightOperator extends PhysicalPlan { - @NonNull - private final PhysicalPlan input; - private final Expression highlightField; - - public HighlightOperator(PhysicalPlan input, Expression highlightField) { - this.input = input; - this.highlightField = highlightField; - } - - @Override - public R accept(PhysicalPlanNodeVisitor visitor, C context) { - return visitor.visitHighlight(this, context); - } - - @Override - public List getChild() { - return Collections.singletonList(input); - } - - @Override - public boolean hasNext() { - return input.hasNext(); - } - - @Override - public ExprValue next() { - ExprValue inputValue = input.next(); - ImmutableMap.Builder mapBuilder = new ImmutableMap.Builder<>(); - inputValue.tupleValue().forEach(mapBuilder::put); - - return inputValue; - } -} diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java index f5535f64d1..e6e59990c8 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanDSL.java @@ -105,8 +105,4 @@ public ValuesOperator values(List... values) { public static LimitOperator limit(PhysicalPlan input, Integer limit, Integer offset) { return new LimitOperator(input, limit, offset); } - - public static HighlightOperator highlight(PhysicalPlan input, Expression highlightField) { - return new HighlightOperator(input, highlightField); - } } diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java index 6a81d66df6..646aae8220 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java @@ -79,8 +79,4 @@ public R visitMLCommons(PhysicalPlan node, C context) { public R visitAD(PhysicalPlan node, C context) { return visitNode(node, context); } - - public R visitHighlight(HighlightOperator highlightOperator, C context) { - return visitNode(highlightOperator, context); - } } diff --git a/core/src/test/java/org/opensearch/sql/analysis/NamedExpressionAnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/NamedExpressionAnalyzerTest.java index 738e81bfd6..2293d125aa 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/NamedExpressionAnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/NamedExpressionAnalyzerTest.java @@ -12,6 +12,11 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.HighlightFunction; +import org.opensearch.sql.ast.expression.QualifiedName; +import org.opensearch.sql.expression.DSL; +import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.expression.LiteralExpression; import org.opensearch.sql.expression.NamedExpression; import org.opensearch.sql.expression.config.ExpressionConfig; import org.springframework.context.annotation.Configuration; @@ -32,4 +37,15 @@ void visit_named_select_item() { NamedExpression analyze = analyzer.analyze(alias, analysisContext); assertEquals("integer_value", analyze.getNameOrAlias()); } + + @Test + void visit_highlight() { + Alias alias = AstDSL.alias("highlight(fieldA)", + new HighlightFunction(AstDSL.stringLiteral("fieldA"))); + NamedExpressionAnalyzer analyzer = + new NamedExpressionAnalyzer(expressionAnalyzer); + + NamedExpression analyze = analyzer.analyze(alias, analysisContext); + assertEquals("highlight(fieldA)", analyze.getNameOrAlias()); + } } diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java deleted file mode 100644 index 55745d51f4..0000000000 --- a/core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.planner.physical; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.opensearch.sql.data.type.ExprCoreType.STRING; - -import com.google.common.collect.ImmutableMap; -import java.util.List; -import org.junit.jupiter.api.Test; -import org.opensearch.sql.data.model.ExprValue; -import org.opensearch.sql.data.model.ExprValueUtils; -import org.opensearch.sql.expression.DSL; - -public class HighlightOperatorTest extends PhysicalPlanTestBase { - - @Test - public void valid_highlight_operator_test() { - PhysicalPlan plan = new HighlightOperator(new TestScan(), DSL.ref("*", STRING)); - List result = execute(plan); - assertEquals(5, result.size()); - assertThat(result, containsInAnyOrder( - ExprValueUtils.tupleValue(ImmutableMap.of("ip", "209.160.24.63", - "action", "GET", "response", 200, "referer", "www.amazon.com")), - ExprValueUtils.tupleValue(ImmutableMap.of("ip", "209.160.24.63", - "action", "GET", "response", 404, "referer", "www.amazon.com")), - ExprValueUtils.tupleValue(ImmutableMap.of("ip", "112.111.162.4", - "action", "GET", "response", 200, "referer", "www.amazon.com")), - ExprValueUtils.tupleValue(ImmutableMap.of("ip", "74.125.19.106", - "action", "POST", "response", 200, "referer", "www.google.com")), - ExprValueUtils.tupleValue(ImmutableMap.of("ip", "74.125.19.106", - "action", "POST", "response", 500)) - )); - } -} diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java index e721444399..cd561f3c09 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java @@ -120,10 +120,6 @@ public void test_PhysicalPlanVisitor_should_return_null() { assertNull(dedupe.accept(new PhysicalPlanNodeVisitor() { }, null)); - PhysicalPlan highlight = PhysicalPlanDSL.highlight(plan, ref); - assertNull(highlight.accept(new PhysicalPlanNodeVisitor() { - }, null)); - PhysicalPlan values = PhysicalPlanDSL.values(emptyList()); assertNull(values.accept(new PhysicalPlanNodeVisitor() { }, null)); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java index f36b7b97db..45d2b12620 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java @@ -14,7 +14,6 @@ import org.opensearch.sql.planner.physical.DedupeOperator; import org.opensearch.sql.planner.physical.EvalOperator; import org.opensearch.sql.planner.physical.FilterOperator; -import org.opensearch.sql.planner.physical.HighlightOperator; import org.opensearch.sql.planner.physical.LimitOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.planner.physical.ProjectOperator; @@ -151,13 +150,6 @@ public PhysicalPlan visitAD(PhysicalPlan node, Object context) { ); } - @Override - public PhysicalPlan visitHighlight(HighlightOperator node, Object context) { - return doProtect(new HighlightOperator(visitInput(node.getInput(), context), - node.getHighlightField()) - ); - } - PhysicalPlan visitInput(PhysicalPlan node, Object context) { if (null == node) { return node; diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java index 8d0f09ddd1..c028f283a2 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java @@ -37,7 +37,6 @@ import org.opensearch.sql.planner.logical.LogicalMLCommons; import org.opensearch.sql.planner.logical.LogicalPlan; import org.opensearch.sql.planner.logical.LogicalRelation; -import org.opensearch.sql.planner.physical.HighlightOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.storage.Table; @@ -193,7 +192,7 @@ public PhysicalPlan visitAD(LogicalAD node, OpenSearchIndexScan context) { @Override public PhysicalPlan visitHighlight(LogicalHighlight node, OpenSearchIndexScan context) { context.pushDownHighlight(node.getHighlightField().toString()); - return new HighlightOperator(visitChild(node, context), node.getHighlightField()); + return visitChild(node, context); } } } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java index 356036cb66..ee981a4abc 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java @@ -58,7 +58,6 @@ import org.opensearch.sql.opensearch.planner.physical.MLCommonsOperator; import org.opensearch.sql.opensearch.setting.OpenSearchSettings; import org.opensearch.sql.opensearch.storage.OpenSearchIndexScan; -import org.opensearch.sql.planner.physical.HighlightOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.planner.physical.PhysicalPlanDSL; @@ -291,16 +290,6 @@ public void testVisitAD() { executionProtector.visitAD(adOperator, null)); } - @Test - public void testVisitHighlight() { - HighlightOperator hl = - new HighlightOperator( - values(emptyList()), DSL.ref("*", STRING)); - - assertEquals(executionProtector.doProtect(hl), - executionProtector.visitHighlight(hl, null)); - } - PhysicalPlan resourceMonitor(PhysicalPlan input) { return new ResourceMonitorPlan(input, resourceMonitor); } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchDefaultImplementorTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchDefaultImplementorTest.java index b3bed007cd..c83172955c 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchDefaultImplementorTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchDefaultImplementorTest.java @@ -9,6 +9,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.verify; import static org.opensearch.sql.planner.logical.LogicalPlanDSL.relation; import org.junit.jupiter.api.Test; @@ -76,6 +78,8 @@ public void visitHighlight() { Mockito.when(node.getChild().get(0)).thenReturn(Mockito.mock(LogicalPlan.class)); OpenSearchIndex.OpenSearchDefaultImplementor implementor = new OpenSearchIndex.OpenSearchDefaultImplementor(indexScan, client); - assertNotNull(implementor.visitHighlight(node, indexScan)); + + implementor.visitHighlight(node, indexScan); + verify(indexScan).pushDownHighlight(any()); } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java index 9f2f8b5f92..8bf38b14a6 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java @@ -616,14 +616,12 @@ public void describe_and_column_compatible_with_old_engine_syntax() { @Test public void can_build_alias_by_keywords() { - var expected = project( - relation("test"), - alias("avg_age", qualifiedName("avg_age"), "avg") - ); - var comp = buildAST("SELECT avg_age AS avg FROM test"); assertEquals( - expected, - comp + project( + relation("test"), + alias("avg_age", qualifiedName("avg_age"), "avg") + ), + buildAST("SELECT avg_age AS avg FROM test") ); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 8cb782356b..ef881275e5 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -310,9 +310,17 @@ public void canBuildWindowFunctionWithNullOrderSpecified() { } @Test - public void canBuildHighlighFunction() { + public void canBuildStringLiteralHighlightFunction() { assertEquals( highlight(AstDSL.stringLiteral("fieldA")), + buildExprAst("highlight(\"fieldA\")") + ); + } + + @Test + public void canBuildQualifiedNameHighlightFunction() { + assertEquals( + highlight(AstDSL.qualifiedName("fieldA")), buildExprAst("highlight(fieldA)") ); } From 232b19f5924d2c46fd08b9d60abadb0d207ad016 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Wed, 3 Aug 2022 11:48:55 -0700 Subject: [PATCH 21/24] Adding highlight function to functions.rst and removing unecessary function call in OpenSearchIndexScan. Signed-off-by: forestmvey --- docs/user/dql/functions.rst | 41 +++++++++++++++++++ .../storage/OpenSearchIndexScan.java | 1 - 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/docs/user/dql/functions.rst b/docs/user/dql/functions.rst index 240bf32dfb..3e3197a916 100644 --- a/docs/user/dql/functions.rst +++ b/docs/user/dql/functions.rst @@ -2479,3 +2479,44 @@ Another example to show how to set custom values for the optional parameters:: |------+--------------------------+----------------------| | 1 | The House at Pooh Corner | Alan Alexander Milne | +------+--------------------------+----------------------+ + + +HIGHLIGHT +------------ + +Description +>>>>>>>>>>> + +``highlight(field_expression)`` + +The highlight function maps to the highlight function used in search engine to return highlight fields for the given search. +The syntax allows to specify the field in double quotes or single quotes or without any wrap. +All fields search using star ``"*"`` is also available (star symbol should be wrapped). +The star ``"*"`` can be used as a wildcard to complete a portion of a field or fields. +Please refer to examples below: + +| ``highlight(title)`` +| ``highlight("*")`` +| ``highlight("T*")`` + +Example searching for field Tags:: + + os> select highlight(title) from books where query_string(['title'], 'Pooh House'); + fetched rows / total rows = 2/2 + +------------------------------------------------------------------+ + | highlight(title) | + |------------------------------------------------------------------| + | [title], fragments[[The House at Pooh Corner]] | + | [title], fragments[[Winnie-the-Pooh]] | + +------------------------------------------------------------------+ + +Another example to show how to set custom values for the optional parameters:: + + os> select highlight('*') from books where query_string(['title'], 'Pooh House'); + fetched rows / total rows = 2/2 + +------------------------------------------------------------------------------------------------+ + | highlight('*') | + |------------------------------------------------------------------------------------------------| + | {'highlight("*").title': '"[title], fragments[[The House at Pooh Corner]]"'} | + | {'highlight("*").title': '"[title], fragments[[Winnie-the-Pooh]]"'} | + +------------------------------------------------------------------------------------------------+ diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java index c93b4ef6b5..6e88f3de89 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java @@ -168,7 +168,6 @@ public void pushDownHighlight(String field) { SearchSourceBuilder sourceBuilder = request.getSourceBuilder(); if (sourceBuilder.highlighter() != null) { sourceBuilder.highlighter().field(StringUtils.unquoteText(field)); - sourceBuilder.highlighter(sourceBuilder.highlighter()); } else { HighlightBuilder highlightBuilder = new HighlightBuilder().field(StringUtils.unquoteText(field)); From 9fbea3072bbf6cd3f89851a20344eecf720ac219 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Thu, 4 Aug 2022 10:04:31 -0700 Subject: [PATCH 22/24] Change highlight fields returned format to array list. Changed highlight all and wildcard to unsupported to open up output formatting changes for multiple returned highlight fields. Change tests for updated coverage and disable highlight all and wildcard tests. Signed-off-by: forestmvey --- .../sql/expression/HighlightExpression.java | 25 ++--------------- .../expression/HighlightExpressionTest.java | 6 ++++- docs/user/dql/functions.rst | 27 +++++-------------- .../sql/sql/HighlightFunctionIT.java | 10 ++++--- .../response/OpenSearchResponse.java | 7 ++--- .../response/OpenSearchResponseTest.java | 6 +++-- 6 files changed, 28 insertions(+), 53 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java index 240ec1ea8e..7dd0a5dbe3 100644 --- a/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java @@ -5,13 +5,10 @@ package org.opensearch.sql.expression; -import com.google.common.collect.ImmutableMap; import java.util.List; import lombok.Getter; import org.opensearch.sql.common.utils.StringUtils; -import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; -import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.expression.env.Environment; @@ -40,26 +37,8 @@ public HighlightExpression(Expression highlightField) { */ @Override public ExprValue valueOf(Environment valueEnv) { - String refName = "_highlight"; - if (!getHighlightField().toString().contains("*")) { - refName += "." + StringUtils.unquoteText(getHighlightField().toString()); - } - ExprValue retVal = valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); - - // If only one highlight returned, or no highlights can be parsed. - if (retVal.isMissing() || retVal.type() != ExprCoreType.STRUCT) { - return retVal; - } - - var highlightMapBuilder = ImmutableMap.builder(); - highlightMapBuilder.putAll(retVal.tupleValue()); - ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); - for (var entry : retVal.tupleValue().entrySet()) { - String entryKey = "highlight(" + getHighlightField() + ")." + entry.getKey(); - builder.put(entryKey, ExprValueUtils.stringValue(entry.getValue().toString())); - } - - return ExprTupleValue.fromExprValueMap(builder.build()); + String refName = "_highlight" + "." + StringUtils.unquoteText(getHighlightField().toString()); + return valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING)); } /** diff --git a/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java b/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java index a63e079418..ad33b3d0cd 100644 --- a/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java @@ -10,6 +10,7 @@ import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; import com.google.common.collect.ImmutableMap; +import com.google.errorprone.annotations.DoNotCall; import org.junit.jupiter.api.Test; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; @@ -39,7 +40,10 @@ public void missing_highlight_test() { assertTrue(resultVal.isMissing()); } - @Test + /** + * Enable me when '*' is supported in highlight. + */ + @DoNotCall public void highlight_all_test() { ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); var hlBuilder = ImmutableMap.builder(); diff --git a/docs/user/dql/functions.rst b/docs/user/dql/functions.rst index 3e3197a916..f1d4d987a3 100644 --- a/docs/user/dql/functions.rst +++ b/docs/user/dql/functions.rst @@ -2491,32 +2491,17 @@ Description The highlight function maps to the highlight function used in search engine to return highlight fields for the given search. The syntax allows to specify the field in double quotes or single quotes or without any wrap. -All fields search using star ``"*"`` is also available (star symbol should be wrapped). -The star ``"*"`` can be used as a wildcard to complete a portion of a field or fields. Please refer to examples below: | ``highlight(title)`` -| ``highlight("*")`` -| ``highlight("T*")`` Example searching for field Tags:: os> select highlight(title) from books where query_string(['title'], 'Pooh House'); fetched rows / total rows = 2/2 - +------------------------------------------------------------------+ - | highlight(title) | - |------------------------------------------------------------------| - | [title], fragments[[The House at Pooh Corner]] | - | [title], fragments[[Winnie-the-Pooh]] | - +------------------------------------------------------------------+ - -Another example to show how to set custom values for the optional parameters:: - - os> select highlight('*') from books where query_string(['title'], 'Pooh House'); - fetched rows / total rows = 2/2 - +------------------------------------------------------------------------------------------------+ - | highlight('*') | - |------------------------------------------------------------------------------------------------| - | {'highlight("*").title': '"[title], fragments[[The House at Pooh Corner]]"'} | - | {'highlight("*").title': '"[title], fragments[[Winnie-the-Pooh]]"'} | - +------------------------------------------------------------------------------------------------+ + +----------------------------------------------+ + | highlight(title) | + |----------------------------------------------| + | [The House at Pooh Corner] | + | [Winnie-the-Pooh] | + +----------------------------------------------+ diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java index 1c171d7bb1..815c6ed9ef 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java @@ -8,6 +8,7 @@ import static org.opensearch.sql.util.MatcherUtils.schema; import static org.opensearch.sql.util.MatcherUtils.verifySchema; +import com.google.errorprone.annotations.DoNotCall; import org.json.JSONObject; import org.junit.Test; import org.opensearch.sql.legacy.SQLIntegTestCase; @@ -47,7 +48,8 @@ public void multiple_highlight_test() { assertEquals(1, response.getInt("total")); } - @Test + // Enable me when * is supported + @DoNotCall public void wildcard_highlight_test() { String query = "SELECT highlight('*itle') FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); @@ -55,7 +57,8 @@ public void wildcard_highlight_test() { assertEquals(1, response.getInt("total")); } - @Test + // Enable me when * is supported + @DoNotCall public void wildcard_multi_field_highlight_test() { String query = "SELECT highlight('T*') FROM %s WHERE MULTI_MATCH([Title, Tags], 'hops') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); @@ -66,7 +69,8 @@ public void wildcard_multi_field_highlight_test() { assertTrue(resultMap.has("highlight(\"T*\").Tags")); } - @Test + // Enable me when * is supported + @DoNotCall public void highlight_all_test() { String query = "SELECT highlight('*') FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java index 0e551c422d..aadd73efdd 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java @@ -9,14 +9,13 @@ import com.google.common.collect.ImmutableMap; import java.util.Arrays; import java.util.Iterator; -import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import lombok.EqualsAndHashCode; import lombok.ToString; import org.opensearch.action.search.SearchResponse; import org.opensearch.search.SearchHits; import org.opensearch.search.aggregations.Aggregations; -import org.opensearch.search.fetch.subphase.highlight.HighlightField; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; @@ -103,7 +102,9 @@ public Iterator iterator() { builder.putAll(docData.tupleValue()); var hlBuilder = ImmutableMap.builder(); for (var es : hit.getHighlightFields().entrySet()) { - hlBuilder.put(es.getKey(), ExprValueUtils.stringValue(es.getValue().toString())); + hlBuilder.put(es.getKey(), ExprValueUtils.collectionValue( + Arrays.stream(es.getValue().fragments()).map( + t -> (t.toString())).collect(Collectors.toList()))); } builder.put("_highlight", ExprTupleValue.fromExprValueMap(hlBuilder.build())); return ExprTupleValue.fromExprValueMap(builder.build()); diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java index 2ad41c7a63..0a60503415 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableMap; import java.util.Arrays; import java.util.Map; +import java.util.stream.Collectors; import org.apache.lucene.search.TotalHits; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -176,8 +177,9 @@ void highlight_iterator() { when(factory.construct(any())).thenReturn(resultTuple); for (ExprValue resultHit : new OpenSearchResponse(searchResponse, factory)) { - var expected = ExprValueUtils.stringValue( - searchHit.getHighlightFields().get("highlights").toString()); + var expected = ExprValueUtils.collectionValue( + Arrays.stream(searchHit.getHighlightFields().get("highlights").getFragments()) + .map(t -> (t.toString())).collect(Collectors.toList())); var result = resultHit.tupleValue().get( "_highlight").tupleValue().get("highlights"); assertTrue(expected.equals(result)); From 23a24e2e97feef52fcbc499ea7d42005d9f53aed Mon Sep 17 00:00:00 2001 From: forestmvey Date: Thu, 4 Aug 2022 16:16:17 -0700 Subject: [PATCH 23/24] Fix bug where invalid schema name was being used for returned highlight fields Signed-off-by: forestmvey --- .../org/opensearch/sql/expression/HighlightExpression.java | 6 +++--- .../opensearch/sql/expression/HighlightExpressionTest.java | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java index 7dd0a5dbe3..9745696111 100644 --- a/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java @@ -31,9 +31,9 @@ public HighlightExpression(Expression highlightField) { } /** - * Return String or Map value matching highlight field. + * Return collection value matching highlight field. * @param valueEnv : Dataset to parse value from. - * @return : String or Map value of highlight fields. + * @return : collection value of highlight fields. */ @Override public ExprValue valueOf(Environment valueEnv) { @@ -47,7 +47,7 @@ public ExprValue valueOf(Environment valueEnv) { */ @Override public ExprType type() { - return ExprCoreType.STRING; + return ExprCoreType.ARRAY; } @Override diff --git a/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java b/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java index ad33b3d0cd..c6e2dccf69 100644 --- a/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/HighlightExpressionTest.java @@ -7,6 +7,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.opensearch.sql.data.type.ExprCoreType.ARRAY; import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; import com.google.common.collect.ImmutableMap; @@ -26,7 +27,7 @@ public void single_highlight_test() { HighlightExpression expr = new HighlightExpression(DSL.literal("Title")); ExprValue resultVal = expr.valueOf(hlTuple); - assertEquals(expr.type(), resultVal.type()); + assertEquals(expr.type(), ARRAY); assertEquals("result value", resultVal.stringValue()); } @@ -54,7 +55,7 @@ public void highlight_all_test() { HighlightExpression hlExpr = new HighlightExpression(DSL.literal("*")); ExprValue resultVal = hlExpr.valueOf( ExprTupleValue.fromExprValueMap(builder.build()).bindingTuples()); - assertEquals(STRUCT, resultVal.type()); + assertEquals(ARRAY, resultVal.type()); for (var field : resultVal.tupleValue().entrySet()) { assertTrue(field.toString().contains(hlExpr.getHighlightField().toString())); } From 449a330243959cdb8ca52a66a7dac450c381b81b Mon Sep 17 00:00:00 2001 From: forestmvey Date: Fri, 5 Aug 2022 08:06:32 -0700 Subject: [PATCH 24/24] Fix failing integration tests due to schema changes for highlight expression type. Signed-off-by: forestmvey --- .../opensearch/sql/sql/HighlightFunctionIT.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java index 815c6ed9ef..422f71968f 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java @@ -26,7 +26,7 @@ public void single_highlight_test() { String query = "SELECT Tags, highlight('Tags') FROM %s WHERE match(Tags, 'yeast') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); verifySchema(response, schema("Tags", null, "text"), - schema("highlight('Tags')", null, "keyword")); + schema("highlight('Tags')", null, "nested")); assertEquals(1, response.getInt("total")); } @@ -35,7 +35,7 @@ public void accepts_unquoted_test() { String query = "SELECT Tags, highlight(Tags) FROM %s WHERE match(Tags, 'yeast') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); verifySchema(response, schema("Tags", null, "text"), - schema("highlight(Tags)", null, "keyword")); + schema("highlight(Tags)", null, "nested")); assertEquals(1, response.getInt("total")); } @@ -43,8 +43,8 @@ public void accepts_unquoted_test() { public void multiple_highlight_test() { String query = "SELECT highlight(Title), highlight(Body) FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); - verifySchema(response, schema("highlight(Title)", null, "keyword"), - schema("highlight(Body)", null, "keyword")); + verifySchema(response, schema("highlight(Title)", null, "nested"), + schema("highlight(Body)", null, "nested")); assertEquals(1, response.getInt("total")); } @@ -53,7 +53,7 @@ public void multiple_highlight_test() { public void wildcard_highlight_test() { String query = "SELECT highlight('*itle') FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); - verifySchema(response, schema("highlight('*itle')", null, "keyword")); + verifySchema(response, schema("highlight('*itle')", null, "nested")); assertEquals(1, response.getInt("total")); } @@ -62,7 +62,7 @@ public void wildcard_highlight_test() { public void wildcard_multi_field_highlight_test() { String query = "SELECT highlight('T*') FROM %s WHERE MULTI_MATCH([Title, Tags], 'hops') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); - verifySchema(response, schema("highlight('T*')", null, "keyword")); + verifySchema(response, schema("highlight('T*')", null, "nested")); var resultMap = response.getJSONArray("datarows").getJSONArray(0).getJSONObject(0); assertEquals(1, response.getInt("total")); assertTrue(resultMap.has("highlight(\"T*\").Title")); @@ -74,7 +74,7 @@ public void wildcard_multi_field_highlight_test() { public void highlight_all_test() { String query = "SELECT highlight('*') FROM %s WHERE MULTI_MATCH([Title, Body], 'hops') LIMIT 1"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); - verifySchema(response, schema("highlight('*')", null, "keyword")); + verifySchema(response, schema("highlight('*')", null, "nested")); assertEquals(1, response.getInt("total")); } @@ -82,7 +82,7 @@ public void highlight_all_test() { public void highlight_no_limit_test() { String query = "SELECT highlight(Body) FROM %s WHERE MATCH(Body, 'hops')"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_BEER)); - verifySchema(response, schema("highlight(Body)", null, "keyword")); + verifySchema(response, schema("highlight(Body)", null, "nested")); assertEquals(2, response.getInt("total")); } }