From 1097d9f2fa5283dfa412bd008459a163dfddea29 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Thu, 30 Jun 2022 19:56:31 -0700 Subject: [PATCH 01/27] 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 00000000000..2ed5e8412ce --- /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 8b74e31aacc..8c4a092e624 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 d75893b6961..a9316b55a4d 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 3c686b5e8e3..78523d289ed 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 00000000000..558cb58e37c --- /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/27] 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 4056347d44f..285948ee045 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 9ce3dd6714d..78f9a26981c 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/27] 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 d576389595a..72542aaf9e6 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/27] 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 37163821e32..17321bc4739 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 78f9a26981c..fa350b5f79a 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/27] 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 2ed5e8412ce..4bf0da23745 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/27] 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 91b964d0749..ce1d4be614e 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 00000000000..e8e840fa127 --- /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 b05b0924a8e..69a7482452c 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 797c5ab11ad..110e48baaf8 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 dd0e199cf4c..73aabcc3eba 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/27] 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 285948ee045..510482c6455 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 4bf0da23745..71a4bfe4ec1 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 5eb9e715b83..cd343284530 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 78523d289ed..453162e3356 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 72542aaf9e6..a2a27bb2df4 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 fa350b5f79a..8cb782356b5 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/27] 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 9054f80e93d..4ac2a3a797a 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 ce1d4be614e..f97b0803a9a 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 00000000000..2005046e4a3 --- /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 e8e840fa127..96f9d70e88c 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 69a7482452c..45dcdde0126 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 00000000000..a7905d61715 --- /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 d5fef43c0db..4185d55c559 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 5163e44edb2..df23b9cd20d 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 110e48baaf8..28598e35a6d 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 73aabcc3eba..668af4263dd 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 a2455a9a3d5..e899351d4f9 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/27] 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 00000000000..8e3d52e927f --- /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/27] 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 dff3ec1f200..d330f35542d 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/27] 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 9f2c2c5fa88..5f1f76dc049 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 d182b5f84d3..8b23daad1ca 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 9d880bb4dcf..fb8aba2c906 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 49301cbf536..e2451367913 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 c35a5ba9dbf..a896764c925 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 2e10f337879..52e76a602b1 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/27] 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 00000000000..d1afb48a470 --- /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 938a4c532c8..e6e59990c82 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 87582df3bbf..6a81d66df6e 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 e2451367913..4cf776c677e 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/27] 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 96f9d70e88c..4fa532376b3 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 d330f35542d..1f857dfd531 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 d1afb48a470..0b0d4ca9572 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 45d2b126204..8253fe8c2f6 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 337f09308c5..46324b98986 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 7dc77d7d295..0223a0acba2 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 4cf776c677e..5b306cb9dc6 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/27] 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 4ac2a3a797a..dc12bdab73f 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 2005046e4a3..f3ccce1ee24 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 4fa532376b3..f5ac8849c29 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 71a4bfe4ec1..0e6f5c660d8 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 1f857dfd531..f295bbf57cc 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 0b0d4ca9572..2379ef5473b 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 8e3d52e927f..db536b93bf6 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 8253fe8c2f6..f36b7b97db3 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 0223a0acba2..0e551c422d4 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 a896764c925..9c79659a4a4 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 8c4a092e624..9fca2942cff 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 a9316b55a4d..dde02cfd3e8 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 a192a2131217fea5ba828bd7d0c4f204ff4a1163 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Mon, 25 Jul 2022 16:25:35 -0700 Subject: [PATCH 15/27] Add support for multiple highlight calls in select statement. Signed-off-by: forestmvey --- .../sql/opensearch/storage/OpenSearchIndexScan.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) 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 9c79659a4a4..2a3cc05f150 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 092d054131711ad23937f43f63f9ff63202c9cf4 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Tue, 26 Jul 2022 07:51:19 -0700 Subject: [PATCH 16/27] Cleaning up code, adding copyright, and adding javadocs Signed-off-by: forestmvey --- .../sql/analysis/HighlightExpression.java | 57 ++++++++++--------- .../sql/ast/expression/HighlightFunction.java | 13 ++--- 2 files changed, 36 insertions(+), 34 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 f5ac8849c29..ea3134fcaaf 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 0e6f5c660d8..f26cff0288a 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); From 9bdbb86d790d0e7e8f4e74288a79b159cbe6f4d1 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Tue, 26 Jul 2022 08:03:19 -0700 Subject: [PATCH 17/27] Removed OpenSearchLogicalIndexScan highlightFields and dependencies. Signed-off-by: forestmvey --- .../sql/expression/function/OpenSearchFunctions.java | 1 - .../planner/logical/OpenSearchLogicalIndexScan.java | 8 ++------ .../planner/logical/rule/MergeLimitAndIndexScan.java | 3 +-- .../planner/logical/rule/MergeSortAndIndexScan.java | 1 - .../sql/opensearch/storage/OpenSearchIndex.java | 6 ------ .../logical/OpenSearchLogicalIndexScanTest.java | 11 ----------- 6 files changed, 3 insertions(+), 27 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 f295bbf57cc..51084728f78 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 @@ -57,7 +57,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/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 8b23daad1ca..ff0b6e46b28 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 @@ -71,7 +71,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 +79,7 @@ public OpenSearchLogicalIndexScan( this.sortList = sortList; this.limit = limit; this.offset = offset; - this.highlightField = highlightField; +// this.highlightField = highlightField; } @Override @@ -91,10 +91,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 fb8aba2c906..9d880bb4dcf 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 46324b98986..337f09308c5 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 5b306cb9dc6..51c811d9358 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 @@ -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/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicalIndexScanTest.java index 52e76a602b1..dc598b4413c 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()); - } } From 9b463cfbf6d9d1371cabbffef8376179d0b5611f Mon Sep 17 00:00:00 2001 From: forestmvey Date: Tue, 26 Jul 2022 09:28:22 -0700 Subject: [PATCH 18/27] Adding tests and touching up code in prep for PR. Signed-off-by: forestmvey --- .../sql/analysis/HighlightAnalyzer.java | 7 ++- .../sql/analysis/HighlightExpression.java | 9 ++-- .../sql/ast/expression/HighlightFunction.java | 2 + .../function/OpenSearchFunctions.java | 1 - .../sql/planner/DefaultImplementor.java | 1 - .../sql/planner/logical/LogicalHighlight.java | 5 ++ .../planner/physical/HighlightOperator.java | 5 ++ .../sql/sql/HighlightFunctionIT.java | 53 ++++++++++++++++++- .../logical/OpenSearchLogicalIndexScan.java | 5 -- sql/src/main/antlr/OpenSearchSQLParser.g4 | 1 - .../sql/sql/antlr/HighlightTest.java | 24 ++++++++- 11 files changed, 95 insertions(+), 18 deletions(-) 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 f3ccce1ee24..61ba7d66f52 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 ea3134fcaaf..9e100cdbbad 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 f26cff0288a..eae1525aae0 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 51084728f78..d330f35542d 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 { 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 5f1f76dc049..9f2c2c5fa88 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 a7905d61715..986a5454860 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 2379ef5473b..5618f89a5c9 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/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java index db536b93bf6..c5bcd24fada 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 ff0b6e46b28..d182b5f84d3 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. */ @@ -79,7 +75,6 @@ public OpenSearchLogicalIndexScan( this.sortList = sortList; this.limit = limit; this.offset = offset; -// this.highlightField = highlightField; } @Override diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index dde02cfd3e8..a9316b55a4d 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 558cb58e37c..1464cc3e698 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 { @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 2648e0cd4551edce6702b4872d43abeffd689c13 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Tue, 26 Jul 2022 10:07:45 -0700 Subject: [PATCH 19/27] Fixing checkstyle errors. Signed-off-by: forestmvey --- .../sql/opensearch/storage/OpenSearchIndex.java | 2 +- .../sql/opensearch/storage/OpenSearchIndexScan.java | 5 +++-- .../java/org/opensearch/sql/sql/antlr/HighlightTest.java | 8 ++++---- 3 files changed, 8 insertions(+), 7 deletions(-) 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 51c811d9358..8d0f09ddd17 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; 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 2a3cc05f150..c93b4ef6b51 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/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java index 1464cc3e698..a94879a8057 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 @@ -15,14 +15,14 @@ void single_field_test() { @Test void multiple_highlights_test() { - acceptQuery("SELECT HIGHLIGHT(Tags), HIGHLIGHT(Body) FROM Index " + - "WHERE MULTI_MATCH([Tags, Body], 'Time')"); + 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')"); + acceptQuery("SELECT HIGHLIGHT('T*') FROM Index " + + "WHERE MULTI_MATCH([Tags, Body], 'Time')"); } @Test From 7290e90231e29c4c473df76602ba0b6c099bfa66 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Tue, 26 Jul 2022 12:57:36 -0700 Subject: [PATCH 20/27] Added HighlightOperatorTest and additional testing. Signed-off-by: forestmvey --- .../expression/ExpressionNodeVisitorTest.java | 6 +++ .../physical/HighlightOperatorTest.java | 40 +++++++++++++++++++ .../physical/PhysicalPlanNodeVisitorTest.java | 5 +++ .../sql/sql/antlr/HighlightTest.java | 2 +- 4 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java 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 caf11064ae8..3cebf4aaefe 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 00000000000..349820dadae --- /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 cd561f3c093..4f38708f894 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/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java index a94879a8057..d88b965368f 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 @@ -7,7 +7,7 @@ import org.junit.jupiter.api.Test; -public class HighlightTest extends SQLParserTest { +public class HighlightTest extends SQLParserTest { @Test void single_field_test() { acceptQuery("SELECT HIGHLIGHT(Tags) FROM Index WHERE MATCH(Tags, 'Time')"); From 2feb2c47a4a951a34adb9bda031f1acebb76e323 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Wed, 27 Jul 2022 06:50:57 -0700 Subject: [PATCH 21/27] 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 9e100cdbbad..cdd0d1896f4 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 00000000000..1b00f768e6e --- /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 dc598b4413c..2e10f337879 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 cd6f9119520576420c2b8aa6095905ebee8119cb Mon Sep 17 00:00:00 2001 From: forestmvey Date: Thu, 28 Jul 2022 16:05:57 -0700 Subject: [PATCH 22/27] Improving test coverage, fixing checkstyle errors, fixing jacoco errors. Signed-off-by: forestmvey --- .../sql/analysis/ExpressionAnalyzer.java | 1 + .../sql/expression/ExpressionNodeVisitor.java | 1 - .../HighlightExpression.java | 9 +++-- .../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 +++++ 14 files changed, 123 insertions(+), 40 deletions(-) rename core/src/main/java/org/opensearch/sql/{analysis => expression}/HighlightExpression.java (93%) 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 f97b0803a9a..670da5c85ce 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/expression/ExpressionNodeVisitor.java b/core/src/main/java/org/opensearch/sql/expression/ExpressionNodeVisitor.java index 45dcdde0126..d53371dd58f 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 93% 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 cdd0d1896f4..2f9d407fd23 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,7 @@ import org.opensearch.sql.expression.env.Environment; import org.opensearch.sql.expression.function.BuiltinFunctionName; -@EqualsAndHashCode(callSuper = false) @Getter -@ToString public class HighlightExpression extends FunctionExpression { private final Expression highlightField; @@ -73,4 +71,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 d330f35542d..f7d922cd9bf 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 e6e59990c82..f5535f64d16 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 28598e35a6d..b9de96b30a3 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 668af4263dd..72db4025522 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 3cebf4aaefe..b0b2bc5b2bf 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 1b00f768e6e..a63e0794189 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 4f38708f894..e7214443997 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 c5bcd24fada..871285f4546 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 5bffa1cfa86..356036cb667 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 fa25b4f4086..ba244bacc62 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 0770ea3938c..b3bed007cd6 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)); + } } From 3db8788d5f8cc9b20de68029bb8171b64206ab81 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Fri, 29 Jul 2022 08:06:59 -0700 Subject: [PATCH 23/27] Added javadocs, minor PR revisions, and fixed jacoco errors by improving coverage for OpenSearchIndexScan Signed-off-by: forestmvey --- .../sql/analysis/HighlightAnalyzer.java | 6 ++++- .../sql/ast/expression/HighlightFunction.java | 3 +++ .../sql/expression/HighlightExpression.java | 10 ++++--- .../storage/OpenSearchIndexScanTest.java | 27 +++++++++++++++++++ 4 files changed, 42 insertions(+), 4 deletions(-) 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 61ba7d66f52..06a601327ce 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 eae1525aae0..5f1bb652d95 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/HighlightExpression.java b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java index 2f9d407fd23..d623dd63933 100644 --- a/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java +++ b/core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java @@ -22,6 +22,9 @@ import org.opensearch.sql.expression.env.Environment; import org.opensearch.sql.expression.function.BuiltinFunctionName; +/** + * Highlight Expression. + */ @Getter public class HighlightExpression extends FunctionExpression { private final Expression highlightField; @@ -47,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())); 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 429c639da98..41769914d95 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 3ee34911797c496f87fbd69d15af0d87115670bc Mon Sep 17 00:00:00 2001 From: forestmvey Date: Fri, 29 Jul 2022 11:50:54 -0700 Subject: [PATCH 24/27] 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 f7d922cd9bf..c3e5cc55947 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 349820dadae..55745d51f46 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 871285f4546..1c171d7bb1c 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 ba244bacc62..2ad41c7a638 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 d88b965368f..6826a37c0b5 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 a2a27bb2df4..9f2f8b5f92d 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 e63b7b91fb55b2aa355bd5838ca5e53ca7f80048 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Fri, 29 Jul 2022 16:29:39 -0700 Subject: [PATCH 25/27] Removing HighlightOperator functionality and minor revisions based on PR. 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 | 4 +- .../OpenSearchExecutionProtectorTest.java | 11 ---- .../OpenSearchDefaultImplementorTest.java | 6 ++- .../sql/parser/AstExpressionBuilderTest.java | 10 +++- 12 files changed, 34 insertions(+), 130 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 d623dd63933..240ec1ea8ef 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 5618f89a5c9..00000000000 --- 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 f5535f64d16..e6e59990c82 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 6a81d66df6e..40046879b1c 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,7 +80,7 @@ public R visitAD(PhysicalPlan node, C context) { return visitNode(node, context); } - public R visitHighlight(HighlightOperator highlightOperator, C context) { - return visitNode(highlightOperator, context); + public R visitHighlight(PhysicalPlan node, C context) { + return visitNode(node, 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 738e81bfd60..2293d125aa2 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 55745d51f46..00000000000 --- 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 e7214443997..cd561f3c093 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 f36b7b97db3..45d2b126204 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 8d0f09ddd17..b2637268e7e 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,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.HighlightOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.storage.Table; @@ -193,7 +193,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 356036cb667..ee981a4abca 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 b3bed007cd6..c83172955c1 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/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 8cb782356b5..ef881275e5b 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 11d834b9cceb47d4606014b4dd58830acd1d0408 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Fri, 29 Jul 2022 16:40:53 -0700 Subject: [PATCH 26/27] Removed unnecessary visitHighlight call in PhysicalPlanNodeVisitor Signed-off-by: forestmvey --- .../sql/planner/physical/PhysicalPlanNodeVisitor.java | 4 ---- 1 file changed, 4 deletions(-) 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 40046879b1c..646aae8220a 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(PhysicalPlan node, C context) { - return visitNode(node, context); - } } From 373353b2634455e72360957c709208fd89e58c6e Mon Sep 17 00:00:00 2001 From: forestmvey Date: Fri, 29 Jul 2022 17:02:14 -0700 Subject: [PATCH 27/27] Fixing formatting errors Signed-off-by: forestmvey --- .../sql/opensearch/storage/OpenSearchIndex.java | 1 - .../opensearch/sql/sql/parser/AstBuilderTest.java | 12 +++++------- 2 files changed, 5 insertions(+), 8 deletions(-) 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 b2637268e7e..c028f283a27 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; 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 9f2f8b5f92d..8bf38b14a67 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") ); }