From 441b99726fa70e2248726c573d8ea24034504a49 Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Wed, 23 Nov 2022 19:37:26 -0800 Subject: [PATCH 1/8] Add position() to V2 engine Signed-off-by: Margarit Hakobyan --- .../sql/analysis/ExpressionAnalyzer.java | 8 ++++ .../sql/ast/AbstractNodeVisitor.java | 4 ++ .../sql/ast/expression/PositionFunction.java | 39 +++++++++++++++++++ .../org/opensearch/sql/expression/DSL.java | 4 ++ .../function/BuiltinFunctionName.java | 1 + .../sql/expression/text/TextFunction.java | 14 +++++++ sql/src/main/antlr/OpenSearchSQLLexer.g4 | 1 + sql/src/main/antlr/OpenSearchSQLParser.g4 | 9 +++++ .../sql/sql/parser/AstExpressionBuilder.java | 7 ++++ 9 files changed, 87 insertions(+) create mode 100644 core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.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 a3ba9b1b6ba..48ead178bb4 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java @@ -35,6 +35,7 @@ import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.Not; import org.opensearch.sql.ast.expression.Or; +import org.opensearch.sql.ast.expression.PositionFunction; import org.opensearch.sql.ast.expression.QualifiedName; import org.opensearch.sql.ast.expression.RelevanceFieldList; import org.opensearch.sql.ast.expression.Span; @@ -213,6 +214,13 @@ public Expression visitHighlightFunction(HighlightFunction node, AnalysisContext return new HighlightExpression(expr); } + @Override + public Expression visitPositionFunction(PositionFunction node, AnalysisContext context) { + Expression left = node.getLeft().accept(this, context); + Expression right = node.getRight().accept(this, context); + return DSL.position(left, right); + } + @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/ast/AbstractNodeVisitor.java b/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java index 53ff93eec11..0f971f91f86 100644 --- a/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java @@ -27,6 +27,7 @@ import org.opensearch.sql.ast.expression.Map; import org.opensearch.sql.ast.expression.Not; import org.opensearch.sql.ast.expression.Or; +import org.opensearch.sql.ast.expression.PositionFunction; import org.opensearch.sql.ast.expression.QualifiedName; import org.opensearch.sql.ast.expression.RelevanceFieldList; import org.opensearch.sql.ast.expression.Span; @@ -277,6 +278,9 @@ public T visitML(ML node, C context) { public T visitHighlightFunction(HighlightFunction node, C context) { return visitChildren(node, context); } + public T visitPositionFunction(PositionFunction node, C context) { + return visitChildren(node, context); + } public T visitStatement(Statement node, C context) { return visit(node, context); diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java new file mode 100644 index 00000000000..5b99952199b --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java @@ -0,0 +1,39 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.ast.expression; + +import lombok.AllArgsConstructor; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import org.opensearch.sql.ast.AbstractNodeVisitor; + +import java.util.Arrays; +import java.util.List; + +/** + * Expression node of Highlight function. + */ +@AllArgsConstructor +@EqualsAndHashCode(callSuper = false) +@Getter +@ToString +public class PositionFunction extends UnresolvedExpression { + @Getter + private UnresolvedExpression left; + @Getter + private UnresolvedExpression right; + + @Override + public List getChild() { + return Arrays.asList(left, right); + } + + @Override + public R accept(AbstractNodeVisitor nodeVisitor, C context) { + return nodeVisitor.visitPositionFunction(this, context); + } +} diff --git a/core/src/main/java/org/opensearch/sql/expression/DSL.java b/core/src/main/java/org/opensearch/sql/expression/DSL.java index 6486c4b6760..96e9e45048e 100644 --- a/core/src/main/java/org/opensearch/sql/expression/DSL.java +++ b/core/src/main/java/org/opensearch/sql/expression/DSL.java @@ -229,6 +229,10 @@ public static FunctionExpression cbrt(Expression... expressions) { return compile(BuiltinFunctionName.CBRT, expressions); } + public static FunctionExpression position(Expression... expressions) { + return compile(BuiltinFunctionName.POSITION, expressions); + } + public static FunctionExpression truncate(Expression... expressions) { return compile(BuiltinFunctionName.TRUNCATE, expressions); } 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 cc3db479824..b6590b5cfea 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 @@ -170,6 +170,7 @@ public enum BuiltinFunctionName { ASCII(FunctionName.of("ascii")), LOCATE(FunctionName.of("locate")), REPLACE(FunctionName.of("replace")), + POSITION(FunctionName.of("position")), /** * NULL Test. diff --git a/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java b/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java index 8035728d19f..c7696e120ed 100644 --- a/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java +++ b/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java @@ -55,6 +55,7 @@ public void register(BuiltinFunctionRepository repository) { repository.register(ascii()); repository.register(locate()); repository.register(replace()); + repository.register(position()); } /** @@ -241,6 +242,15 @@ private DefaultFunctionResolver locate() { TextFunction::exprLocate), INTEGER, STRING, STRING, INTEGER)); } + /** + * POSITION(substr IN str) returns the position of the first occurrence of a substring in a string. + * If the substring is not found within the original string, this function returns 0. + */ + private DefaultFunctionResolver position() { + return define(BuiltinFunctionName.POSITION.getName(), + impl(nullMissingHandling(TextFunction::exprPosition), STRING, STRING, STRING)); + } + /** * REPLACE(str, from_str, to_str) returns the string str with all occurrences of * the string from_str replaced by the string to_str. @@ -313,6 +323,10 @@ private static ExprValue exprLocate(ExprValue subStr, ExprValue str, ExprValue p str.stringValue().indexOf(subStr.stringValue(), pos.integerValue() - 1) + 1); } + private static ExprValue exprPosition(ExprValue subStr, ExprValue str) { + return exprLocate(subStr, str); + } + private static ExprValue exprReplace(ExprValue str, ExprValue from, ExprValue to) { return new ExprStringValue(str.stringValue().replaceAll(from.stringValue(), to.stringValue())); } diff --git a/sql/src/main/antlr/OpenSearchSQLLexer.g4 b/sql/src/main/antlr/OpenSearchSQLLexer.g4 index 9e0a4094019..b15363445ea 100644 --- a/sql/src/main/antlr/OpenSearchSQLLexer.g4 +++ b/sql/src/main/antlr/OpenSearchSQLLexer.g4 @@ -234,6 +234,7 @@ NULLIF: 'NULLIF'; PERIOD_ADD: 'PERIOD_ADD'; PERIOD_DIFF: 'PERIOD_DIFF'; PI: 'PI'; +POSITION: 'POSITION'; POW: 'POW'; POWER: 'POWER'; RADIANS: 'RADIANS'; diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index b3fd29b342d..8bfbaa74986 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -302,6 +302,7 @@ functionCall | relevanceFunction #relevanceFunctionCall | highlightFunction #highlightFunctionCall | constantFunction #constantFunctionCall + | positionFunction #positionFunctionCall ; constantFunction @@ -312,6 +313,10 @@ highlightFunction : HIGHLIGHT LR_BRACKET relevanceField (COMMA highlightArg)* RR_BRACKET ; +positionFunction + : POSITION LR_BRACKET inFunctionsArgs RR_BRACKET + ; + scalarFunctionName : mathematicalFunctionName | dateTimeFunctionName @@ -455,6 +460,10 @@ highlightArg : highlightArgName EQUAL_SYMBOL highlightArgValue ; +inFunctionsArgs + : functionArg IN functionArg + ; + relevanceArgName : ALLOW_LEADING_WILDCARD | ANALYZER | ANALYZE_WILDCARD | AUTO_GENERATE_SYNONYMS_PHRASE_QUERY | BOOST | CUTOFF_FREQUENCY | DEFAULT_FIELD | DEFAULT_OPERATOR | ENABLE_POSITION_INCREMENTS 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 131b6d9116d..c15d7d42294 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 @@ -72,6 +72,7 @@ import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.Not; import org.opensearch.sql.ast.expression.Or; +import org.opensearch.sql.ast.expression.PositionFunction; import org.opensearch.sql.ast.expression.QualifiedName; import org.opensearch.sql.ast.expression.RelevanceFieldList; import org.opensearch.sql.ast.expression.UnresolvedArgument; @@ -427,6 +428,12 @@ private UnresolvedExpression visitConstantFunction(String functionName, .map(this::visitFunctionArg) .collect(Collectors.toList())); } + @Override + public UnresolvedExpression visitPositionFunction(OpenSearchSQLParser.PositionFunctionContext ctx) { + return new PositionFunction(visitFunctionArg(ctx.inFunctionsArgs().functionArg(0)), + visitFunctionArg(ctx.inFunctionsArgs().functionArg(1))); + + } private QualifiedName visitIdentifiers(List identifiers) { return new QualifiedName( From 8025fdb75658373a53d338f7b072363e850fecbb Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Thu, 24 Nov 2022 15:04:42 -0800 Subject: [PATCH 2/8] Add integration tests Signed-off-by: Margarit Hakobyan --- .../sql/sql/PositionFunctionIT.java | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java new file mode 100644 index 00000000000..895a27fdac7 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java @@ -0,0 +1,97 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +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; + +import static org.opensearch.sql.util.MatcherUtils.rows; +import static org.opensearch.sql.util.MatcherUtils.schema; +import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; +import static org.opensearch.sql.util.MatcherUtils.verifySchema; + +public class PositionFunctionIT extends SQLIntegTestCase { + + @Override + protected void init() throws Exception { + loadIndex(Index.PEOPLE2); + loadIndex(Index.CALCS); + } + + @Test + public void position_function_test() { + String query = "SELECT firstname, position('a' IN firstname) FROM %s"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_PEOPLE2)); + + verifySchema(response, schema("firstname", null, "keyword"), + schema("position('a' IN firstname)", null, "keyword")); + assertEquals(12, response.getInt("total")); + + verifyDataRows(response, + rows("Daenerys", 2), rows("Hattie", 2), + rows("Nanette", 2), rows("Dale", 2), + rows("Elinor", 0), rows("Virginia", 8), + rows("Dillard", 5), rows("Mcgee", 0), + rows("Aurelia", 7), rows("Fulton", 0), + rows("Burton", 0), rows("Josie", 0)); + } + + @Test + public void position_function_with_nulls_test() { + String query = "SELECT str2, position('ee' IN str2) FROM %s"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_CALCS)); + + verifySchema(response, schema("str2", null, "keyword"), + schema("position('ee' IN str2)", null, "keyword")); + assertEquals(17, response.getInt("total")); + + verifyDataRows(response, + rows("one", 0), rows("two", 0), + rows("three", 4), rows(null, null), + rows("five", 0), rows("six", 0), + rows(null, null), rows("eight", 0), + rows("nine", 0), rows("ten", 0), + rows("eleven", 0), rows("twelve", 0), + rows(null, null), rows("fourteen", 6), + rows("fifteen", 5), rows("sixteen", 5), + rows(null, null)); + } + + @Test + public void position_function_with_string_literals_test() { + String query = "SELECT position('world' IN 'hello world')"; + JSONObject response = executeJdbcRequest(query); + + verifySchema(response, schema("position('world' IN 'hello world')", null, "keyword")); + assertEquals(1, response.getInt("total")); + + verifyDataRows(response, rows(7)); + } + + @Test + public void position_function_with_only_fields_as_args_test() { + String query = "SELECT position(str3 IN str2) FROM %s where str2 IN ('one', 'two', 'three')"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_CALCS)); + + verifySchema(response, schema("position(str3 IN str2)", null, "keyword")); + assertEquals(3, response.getInt("total")); + + verifyDataRows(response, rows(3), rows(0), rows(4)); + } + + @Test + public void position_function_with_function_as_arg_test() { + String query = "SELECT position(upper(str3) IN str1) FROM %s where str1 LIKE 'BINDING SUPPLIES'"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_CALCS)); + + verifySchema(response, schema("position(upper(str3) IN str1)", null, "keyword")); + assertEquals(1, response.getInt("total")); + + verifyDataRows(response, rows(15)); + } +} From 0950e7d49c3dce271e059496ecf50db4eb9a19dd Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Thu, 24 Nov 2022 15:14:13 -0800 Subject: [PATCH 3/8] Address PR review comment Signed-off-by: Margarit Hakobyan --- sql/src/main/antlr/OpenSearchSQLParser.g4 | 5 +---- .../org/opensearch/sql/sql/parser/AstExpressionBuilder.java | 3 +-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index 8bfbaa74986..712f0e5cb33 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -314,7 +314,7 @@ highlightFunction ; positionFunction - : POSITION LR_BRACKET inFunctionsArgs RR_BRACKET + : POSITION LR_BRACKET functionArg IN functionArg RR_BRACKET ; scalarFunctionName @@ -460,9 +460,6 @@ highlightArg : highlightArgName EQUAL_SYMBOL highlightArgValue ; -inFunctionsArgs - : functionArg IN functionArg - ; relevanceArgName : ALLOW_LEADING_WILDCARD | ANALYZER | ANALYZE_WILDCARD | AUTO_GENERATE_SYNONYMS_PHRASE_QUERY 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 c15d7d42294..97bf6decc8f 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 @@ -430,8 +430,7 @@ private UnresolvedExpression visitConstantFunction(String functionName, } @Override public UnresolvedExpression visitPositionFunction(OpenSearchSQLParser.PositionFunctionContext ctx) { - return new PositionFunction(visitFunctionArg(ctx.inFunctionsArgs().functionArg(0)), - visitFunctionArg(ctx.inFunctionsArgs().functionArg(1))); + return new PositionFunction(visitFunctionArg(ctx.functionArg(0)), visitFunctionArg(ctx.functionArg(1))); } From 8609e8bc02a09e2b54ca714e1ec7b62ee82fe7e9 Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Thu, 24 Nov 2022 15:26:07 -0800 Subject: [PATCH 4/8] Address another PR review comment Signed-off-by: Margarit Hakobyan --- .../opensearch/sql/expression/text/TextFunction.java | 9 ++++++--- .../org/opensearch/sql/sql/PositionFunctionIT.java | 10 +++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java b/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java index c7696e120ed..dac2c3566a4 100644 --- a/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java +++ b/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java @@ -243,12 +243,15 @@ private DefaultFunctionResolver locate() { } /** - * POSITION(substr IN str) returns the position of the first occurrence of a substring in a string. - * If the substring is not found within the original string, this function returns 0. + * Returns the position of the first occurrence of a substring in a string. + * Returns 0 if substring is not in string. + * Returns NULL if any argument is NULL. + * Supports following signature: + * (STRING IN STRING) -> INTEGER */ private DefaultFunctionResolver position() { return define(BuiltinFunctionName.POSITION.getName(), - impl(nullMissingHandling(TextFunction::exprPosition), STRING, STRING, STRING)); + impl(nullMissingHandling(TextFunction::exprPosition), INTEGER, STRING, STRING)); } /** diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java index 895a27fdac7..b87173af1d5 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java @@ -29,7 +29,7 @@ public void position_function_test() { JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_PEOPLE2)); verifySchema(response, schema("firstname", null, "keyword"), - schema("position('a' IN firstname)", null, "keyword")); + schema("position('a' IN firstname)", null, "integer")); assertEquals(12, response.getInt("total")); verifyDataRows(response, @@ -47,7 +47,7 @@ public void position_function_with_nulls_test() { JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_CALCS)); verifySchema(response, schema("str2", null, "keyword"), - schema("position('ee' IN str2)", null, "keyword")); + schema("position('ee' IN str2)", null, "integer")); assertEquals(17, response.getInt("total")); verifyDataRows(response, @@ -67,7 +67,7 @@ public void position_function_with_string_literals_test() { String query = "SELECT position('world' IN 'hello world')"; JSONObject response = executeJdbcRequest(query); - verifySchema(response, schema("position('world' IN 'hello world')", null, "keyword")); + verifySchema(response, schema("position('world' IN 'hello world')", null, "integer")); assertEquals(1, response.getInt("total")); verifyDataRows(response, rows(7)); @@ -78,7 +78,7 @@ public void position_function_with_only_fields_as_args_test() { String query = "SELECT position(str3 IN str2) FROM %s where str2 IN ('one', 'two', 'three')"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_CALCS)); - verifySchema(response, schema("position(str3 IN str2)", null, "keyword")); + verifySchema(response, schema("position(str3 IN str2)", null, "integer")); assertEquals(3, response.getInt("total")); verifyDataRows(response, rows(3), rows(0), rows(4)); @@ -89,7 +89,7 @@ public void position_function_with_function_as_arg_test() { String query = "SELECT position(upper(str3) IN str1) FROM %s where str1 LIKE 'BINDING SUPPLIES'"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_CALCS)); - verifySchema(response, schema("position(upper(str3) IN str1)", null, "keyword")); + verifySchema(response, schema("position(upper(str3) IN str1)", null, "integer")); assertEquals(1, response.getInt("total")); verifyDataRows(response, rows(15)); From e254462d9820a4a7ffaa7dd694cc5acbd282beee Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Thu, 24 Nov 2022 19:47:49 -0800 Subject: [PATCH 5/8] Add a unit test Signed-off-by: Margarit Hakobyan --- .../sql/expression/text/TextFunctionTest.java | 14 ++++++++++++++ sql/src/main/antlr/OpenSearchSQLParser.g4 | 1 - 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/opensearch/sql/expression/text/TextFunctionTest.java b/core/src/test/java/org/opensearch/sql/expression/text/TextFunctionTest.java index 502fe70ec86..3879d4ce10f 100644 --- a/core/src/test/java/org/opensearch/sql/expression/text/TextFunctionTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/text/TextFunctionTest.java @@ -377,6 +377,20 @@ void locate() { DSL.locate(missingRef, DSL.literal("hello"), DSL.literal(1)))); } + @Test + void position() { + FunctionExpression expression = DSL.position( + DSL.literal("world"), + DSL.literal("helloworld")); + assertEquals(INTEGER, expression.type()); + assertEquals(6, eval(expression).integerValue()); + + when(nullRef.type()).thenReturn(STRING); + assertEquals(nullValue(), eval(DSL.position(nullRef, DSL.literal("hello")))); + when(missingRef.type()).thenReturn(STRING); + assertEquals(missingValue(), eval(DSL.position(missingRef, DSL.literal("hello")))); + } + @Test void replace() { FunctionExpression expression = DSL.replace( diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index 712f0e5cb33..620a1811fd8 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -460,7 +460,6 @@ highlightArg : highlightArgName EQUAL_SYMBOL highlightArgValue ; - relevanceArgName : ALLOW_LEADING_WILDCARD | ANALYZER | ANALYZE_WILDCARD | AUTO_GENERATE_SYNONYMS_PHRASE_QUERY | BOOST | CUTOFF_FREQUENCY | DEFAULT_FIELD | DEFAULT_OPERATOR | ENABLE_POSITION_INCREMENTS From 07637e9476803fb5eb37b49c1318b902a5d5e6bc Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Fri, 25 Nov 2022 14:24:30 -0800 Subject: [PATCH 6/8] Add unit tests and docs Signed-off-by: Margarit Hakobyan --- .../sql/ast/AbstractNodeVisitor.java | 1 + .../org/opensearch/sql/ast/dsl/AstDSL.java | 5 ++++ .../sql/ast/expression/PositionFunction.java | 4 +-- .../analysis/NamedExpressionAnalyzerTest.java | 11 ++++++++ docs/user/dql/functions.rst | 25 +++++++++++++++++++ .../sql/sql/parser/AstExpressionBuilder.java | 8 +++--- .../sql/parser/AstExpressionBuilderTest.java | 9 +++++++ 7 files changed, 58 insertions(+), 5 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 0f971f91f86..e515de86667 100644 --- a/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java @@ -278,6 +278,7 @@ public T visitML(ML node, C context) { public T visitHighlightFunction(HighlightFunction node, C context) { return visitChildren(node, context); } + public T visitPositionFunction(PositionFunction node, C context) { return visitChildren(node, context); } 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 24ada1fd927..cca88eac05a 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 @@ -33,6 +33,7 @@ import org.opensearch.sql.ast.expression.Not; import org.opensearch.sql.ast.expression.Or; import org.opensearch.sql.ast.expression.ParseMethod; +import org.opensearch.sql.ast.expression.PositionFunction; import org.opensearch.sql.ast.expression.QualifiedName; import org.opensearch.sql.ast.expression.Span; import org.opensearch.sql.ast.expression.SpanUnit; @@ -288,6 +289,10 @@ public UnresolvedExpression highlight(UnresolvedExpression fieldName, return new HighlightFunction(fieldName, arguments); } + public UnresolvedExpression position(UnresolvedExpression left, UnresolvedExpression right) { + return new PositionFunction(left, right); + } + public UnresolvedExpression window(UnresolvedExpression function, List partitionByList, List> sortList) { diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java index 5b99952199b..65fe546421f 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java @@ -5,14 +5,14 @@ package org.opensearch.sql.ast.expression; +import java.util.Arrays; +import java.util.List; import lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.ToString; import org.opensearch.sql.ast.AbstractNodeVisitor; -import java.util.Arrays; -import java.util.List; /** * Expression node of Highlight function. 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 913593add34..2f97c6480e1 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/NamedExpressionAnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/NamedExpressionAnalyzerTest.java @@ -16,6 +16,7 @@ import org.opensearch.sql.ast.expression.Alias; import org.opensearch.sql.ast.expression.HighlightFunction; import org.opensearch.sql.ast.expression.Literal; +import org.opensearch.sql.ast.expression.PositionFunction; import org.opensearch.sql.expression.NamedExpression; import org.springframework.context.annotation.Configuration; import org.springframework.test.context.ContextConfiguration; @@ -48,4 +49,14 @@ void visit_highlight() { NamedExpression analyze = analyzer.analyze(alias, analysisContext); assertEquals("highlight(fieldA)", analyze.getNameOrAlias()); } + + @Test + void visit_position() { + Alias alias = AstDSL.alias("position(fieldA IN fieldB)", + new PositionFunction(AstDSL.stringLiteral("fieldA"), AstDSL.stringLiteral("fieldB"))); + NamedExpressionAnalyzer analyzer = new NamedExpressionAnalyzer(expressionAnalyzer); + + NamedExpression analyze = analyzer.analyze(alias, analysisContext); + assertEquals("position(fieldA IN fieldB)", analyze.getNameOrAlias()); + } } diff --git a/docs/user/dql/functions.rst b/docs/user/dql/functions.rst index 9c26427143a..5864f9809d0 100644 --- a/docs/user/dql/functions.rst +++ b/docs/user/dql/functions.rst @@ -2331,6 +2331,31 @@ Example:: +---------------------+---------------------+ +POSITION +------ + +Description +>>>>>>>>>>> + +Usage: The syntax POSITION(substr IN str) returns the position of the first occurrence of substring substr in string str. Returns 0 if substr is not in str. Returns NULL if any argument is NULL. + +Argument type: STRING, STRING, INTEGER + +Return type integer: + +(STRING IN STRING) -> INTEGER + +Example:: + + os> SELECT POSITION('world' IN 'helloworld') + fetched rows / total rows = 1/1 + +-------------------------------------+ + | POSITION('world' IN 'helloworld') | + |-------------------------------------| + | 6 | + +-------------------------------------+ + + REPLACE ------- 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 97bf6decc8f..7f6b52e87cb 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 @@ -428,10 +428,12 @@ private UnresolvedExpression visitConstantFunction(String functionName, .map(this::visitFunctionArg) .collect(Collectors.toList())); } - @Override - public UnresolvedExpression visitPositionFunction(OpenSearchSQLParser.PositionFunctionContext ctx) { - return new PositionFunction(visitFunctionArg(ctx.functionArg(0)), visitFunctionArg(ctx.functionArg(1))); + @Override + public UnresolvedExpression visitPositionFunction( + OpenSearchSQLParser.PositionFunctionContext ctx) { + return new PositionFunction(visitFunctionArg(ctx.functionArg(0)), + visitFunctionArg(ctx.functionArg(1))); } private QualifiedName visitIdentifiers(List identifiers) { 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 cb00ea2f185..216146715cf 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 @@ -21,6 +21,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.not; import static org.opensearch.sql.ast.dsl.AstDSL.nullLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.or; +import static org.opensearch.sql.ast.dsl.AstDSL.position; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; import static org.opensearch.sql.ast.dsl.AstDSL.stringLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.timeLiteral; @@ -328,6 +329,14 @@ public void canBuildQualifiedNameHighlightFunction() { ); } + @Test + public void canBuildPositionFunction() { + assertEquals( + position(AstDSL.qualifiedName("fieldA"), AstDSL.qualifiedName("fieldB")), + buildExprAst("position(fieldA IN fieldB)") + ); + } + @Test public void canBuildWindowFunctionWithoutOrderBy() { assertEquals( From c8cd1bb21deda0de7a89ffa8f92f4187a5b9c750 Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Mon, 28 Nov 2022 13:36:10 -0800 Subject: [PATCH 7/8] Added a test case Signed-off-by: Margarit Hakobyan --- .../opensearch/sql/sql/PositionFunctionIT.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java index b87173af1d5..1e4cf8a363d 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java @@ -75,7 +75,7 @@ public void position_function_with_string_literals_test() { @Test public void position_function_with_only_fields_as_args_test() { - String query = "SELECT position(str3 IN str2) FROM %s where str2 IN ('one', 'two', 'three')"; + String query = "SELECT position(str3 IN str2) FROM %s WHERE str2 IN ('one', 'two', 'three')"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_CALCS)); verifySchema(response, schema("position(str3 IN str2)", null, "integer")); @@ -86,7 +86,7 @@ public void position_function_with_only_fields_as_args_test() { @Test public void position_function_with_function_as_arg_test() { - String query = "SELECT position(upper(str3) IN str1) FROM %s where str1 LIKE 'BINDING SUPPLIES'"; + String query = "SELECT position(upper(str3) IN str1) FROM %s WHERE str1 LIKE 'BINDING SUPPLIES'"; JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_CALCS)); verifySchema(response, schema("position(upper(str3) IN str1)", null, "integer")); @@ -94,4 +94,15 @@ public void position_function_with_function_as_arg_test() { verifyDataRows(response, rows(15)); } + + @Test + public void position_function_in_where_clause_test() { + String query = "SELECT str2 FROM %s WHERE position(str3 IN str2)=1"; + JSONObject response = executeJdbcRequest(String.format(query, TestsConstants.TEST_INDEX_CALCS)); + + verifySchema(response, schema("str2", null, "keyword")); + assertEquals(2, response.getInt("total")); + + verifyDataRows(response, rows("eight"), rows("eleven")); + } } From 9a6b7b5d8962cff8af410638da5151e9ee6a36b9 Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Mon, 28 Nov 2022 16:43:30 -0800 Subject: [PATCH 8/8] Address PR review comments Signed-off-by: Margarit Hakobyan --- .../sql/analysis/ExpressionAnalyzer.java | 6 ++--- .../org/opensearch/sql/ast/dsl/AstDSL.java | 5 ++-- .../sql/ast/expression/PositionFunction.java | 8 +++--- .../function/BuiltinFunctionName.java | 26 +++++++++---------- .../sql/expression/text/TextFunction.java | 26 +++++++++---------- .../sql/expression/text/TextFunctionTest.java | 8 +++++- docs/user/dql/functions.rst | 10 +++---- .../sql/sql/PositionFunctionIT.java | 21 +++++++++++++++ .../sql/parser/AstExpressionBuilderTest.java | 8 ++++++ 9 files changed, 77 insertions(+), 41 deletions(-) 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 48ead178bb4..feae0c8fe00 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java @@ -216,9 +216,9 @@ public Expression visitHighlightFunction(HighlightFunction node, AnalysisContext @Override public Expression visitPositionFunction(PositionFunction node, AnalysisContext context) { - Expression left = node.getLeft().accept(this, context); - Expression right = node.getRight().accept(this, context); - return DSL.position(left, right); + Expression stringPatternExpr = node.getStringPatternExpr().accept(this, context); + Expression searchStringExpr = node.getSearchStringExpr().accept(this, context); + return DSL.position(stringPatternExpr, searchStringExpr); } @Override 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 cca88eac05a..1b8c76bd4df 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 @@ -289,8 +289,9 @@ public UnresolvedExpression highlight(UnresolvedExpression fieldName, return new HighlightFunction(fieldName, arguments); } - public UnresolvedExpression position(UnresolvedExpression left, UnresolvedExpression right) { - return new PositionFunction(left, right); + public UnresolvedExpression position(UnresolvedExpression stringPatternExpr, + UnresolvedExpression searchStringExpr) { + return new PositionFunction(stringPatternExpr, searchStringExpr); } public UnresolvedExpression window(UnresolvedExpression function, diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java b/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java index 65fe546421f..988237ebd0b 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/PositionFunction.java @@ -15,7 +15,7 @@ /** - * Expression node of Highlight function. + * Expression node of Position function. */ @AllArgsConstructor @EqualsAndHashCode(callSuper = false) @@ -23,13 +23,13 @@ @ToString public class PositionFunction extends UnresolvedExpression { @Getter - private UnresolvedExpression left; + private UnresolvedExpression stringPatternExpr; @Getter - private UnresolvedExpression right; + private UnresolvedExpression searchStringExpr; @Override public List getChild() { - return Arrays.asList(left, right); + return Arrays.asList(stringPatternExpr, searchStringExpr); } @Override 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 b6590b5cfea..69f86b7c534 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 @@ -153,24 +153,24 @@ public enum BuiltinFunctionName { /** * Text Functions. */ - SUBSTR(FunctionName.of("substr")), - SUBSTRING(FunctionName.of("substring")), - RTRIM(FunctionName.of("rtrim")), - LTRIM(FunctionName.of("ltrim")), - TRIM(FunctionName.of("trim")), - UPPER(FunctionName.of("upper")), - LOWER(FunctionName.of("lower")), - REGEXP(FunctionName.of("regexp")), + ASCII(FunctionName.of("ascii")), CONCAT(FunctionName.of("concat")), CONCAT_WS(FunctionName.of("concat_ws")), - LENGTH(FunctionName.of("length")), - STRCMP(FunctionName.of("strcmp")), - RIGHT(FunctionName.of("right")), LEFT(FunctionName.of("left")), - ASCII(FunctionName.of("ascii")), + LENGTH(FunctionName.of("length")), LOCATE(FunctionName.of("locate")), - REPLACE(FunctionName.of("replace")), + LOWER(FunctionName.of("lower")), + LTRIM(FunctionName.of("ltrim")), POSITION(FunctionName.of("position")), + REGEXP(FunctionName.of("regexp")), + REPLACE(FunctionName.of("replace")), + RIGHT(FunctionName.of("right")), + RTRIM(FunctionName.of("rtrim")), + STRCMP(FunctionName.of("strcmp")), + SUBSTR(FunctionName.of("substr")), + SUBSTRING(FunctionName.of("substring")), + TRIM(FunctionName.of("trim")), + UPPER(FunctionName.of("upper")), /** * NULL Test. diff --git a/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java b/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java index dac2c3566a4..b51d6a2716c 100644 --- a/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java +++ b/core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java @@ -39,23 +39,23 @@ public class TextFunction { * @param repository {@link BuiltinFunctionRepository}. */ public void register(BuiltinFunctionRepository repository) { - repository.register(substr()); - repository.register(substring()); - repository.register(ltrim()); - repository.register(rtrim()); - repository.register(trim()); - repository.register(lower()); - repository.register(upper()); + repository.register(ascii()); repository.register(concat()); repository.register(concat_ws()); - repository.register(length()); - repository.register(strcmp()); - repository.register(right()); repository.register(left()); - repository.register(ascii()); + repository.register(length()); repository.register(locate()); - repository.register(replace()); + repository.register(lower()); + repository.register(ltrim()); repository.register(position()); + repository.register(replace()); + repository.register(right()); + repository.register(rtrim()); + repository.register(strcmp()); + repository.register(substr()); + repository.register(substring()); + repository.register(trim()); + repository.register(upper()); } /** @@ -243,7 +243,7 @@ private DefaultFunctionResolver locate() { } /** - * Returns the position of the first occurrence of a substring in a string. + * Returns the position of the first occurrence of a substring in a string starting from 1. * Returns 0 if substring is not in string. * Returns NULL if any argument is NULL. * Supports following signature: diff --git a/core/src/test/java/org/opensearch/sql/expression/text/TextFunctionTest.java b/core/src/test/java/org/opensearch/sql/expression/text/TextFunctionTest.java index 3879d4ce10f..5e32678b944 100644 --- a/core/src/test/java/org/opensearch/sql/expression/text/TextFunctionTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/text/TextFunctionTest.java @@ -381,10 +381,16 @@ void locate() { void position() { FunctionExpression expression = DSL.position( DSL.literal("world"), - DSL.literal("helloworld")); + DSL.literal("helloworldworld")); assertEquals(INTEGER, expression.type()); assertEquals(6, eval(expression).integerValue()); + expression = DSL.position( + DSL.literal("abc"), + DSL.literal("hello world")); + assertEquals(INTEGER, expression.type()); + assertEquals(0, eval(expression).integerValue()); + when(nullRef.type()).thenReturn(STRING); assertEquals(nullValue(), eval(DSL.position(nullRef, DSL.literal("hello")))); when(missingRef.type()).thenReturn(STRING); diff --git a/docs/user/dql/functions.rst b/docs/user/dql/functions.rst index 5864f9809d0..3d084152c1c 100644 --- a/docs/user/dql/functions.rst +++ b/docs/user/dql/functions.rst @@ -2349,11 +2349,11 @@ Example:: os> SELECT POSITION('world' IN 'helloworld') fetched rows / total rows = 1/1 - +-------------------------------------+ - | POSITION('world' IN 'helloworld') | - |-------------------------------------| - | 6 | - +-------------------------------------+ + +-------------------------------------+---------------------------------------+ + | POSITION('world' IN 'helloworld') | POSITION('invalid' IN 'helloworld') | + |-------------------------------------+---------------------------------------| + | 6 | 0 | + +-------------------------------------+---------------------------------------+ REPLACE diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java index 1e4cf8a363d..f51a3a09775 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/PositionFunctionIT.java @@ -105,4 +105,25 @@ public void position_function_in_where_clause_test() { verifyDataRows(response, rows("eight"), rows("eleven")); } + + @Test + public void position_function_with_null_args_test() { + String query1 = "SELECT str2, position(null IN str2) FROM %s WHERE str2 IN ('one')"; + String query2 = "SELECT str2, position(str2 IN null) FROM %s WHERE str2 IN ('one')"; + JSONObject response1 = executeJdbcRequest(String.format(query1, TestsConstants.TEST_INDEX_CALCS)); + JSONObject response2 = executeJdbcRequest(String.format(query2, TestsConstants.TEST_INDEX_CALCS)); + + verifySchema(response1, + schema("str2", null, "keyword"), + schema("position(null IN str2)", null, "integer")); + assertEquals(1, response1.getInt("total")); + + verifySchema(response2, + schema("str2", null, "keyword"), + schema("position(str2 IN null)", null, "integer")); + assertEquals(1, response2.getInt("total")); + + verifyDataRows(response1, rows("one", null)); + verifyDataRows(response2, rows("one", null)); + } } 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 216146715cf..c1e3faad755 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 @@ -337,6 +337,14 @@ public void canBuildPositionFunction() { ); } + @Test + public void canBuildStringLiteralPositionFunction() { + assertEquals( + position(AstDSL.stringLiteral("fieldA"), AstDSL.stringLiteral("fieldB")), + buildExprAst("position(\"fieldA\" IN \"fieldB\")") + ); + } + @Test public void canBuildWindowFunctionWithoutOrderBy() { assertEquals(