diff --git a/core/src/main/java/org/opensearch/sql/analysis/NestedAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/NestedAnalyzer.java index 756c1f20b3..4e3939bb14 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/NestedAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/NestedAnalyzer.java @@ -17,6 +17,8 @@ import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.QualifiedName; import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.expression.FunctionExpression; import org.opensearch.sql.expression.NamedExpression; import org.opensearch.sql.expression.ReferenceExpression; import org.opensearch.sql.expression.function.BuiltinFunctionName; @@ -105,7 +107,18 @@ private void validateArgs(List args) { * @param field : Nested field to generate path of. * @return : Path of field derived from last level of nesting. */ - private ReferenceExpression generatePath(String field) { + public static ReferenceExpression generatePath(String field) { return new ReferenceExpression(field.substring(0, field.lastIndexOf(".")), STRING); } + + /** + * Check if supplied expression is a nested function. + * @param expr Expression checking if is nested function. + * @return True if expression is a nested function. + */ + public static Boolean isNestedFunction(Expression expr) { + return (expr instanceof FunctionExpression + && ((FunctionExpression) expr).getFunctionName().getFunctionName() + .equalsIgnoreCase(BuiltinFunctionName.NESTED.name())); + } } 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 59edde6f86..6d83ee53a8 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java @@ -9,9 +9,11 @@ import static java.util.Collections.emptyList; import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.opensearch.sql.analysis.DataSourceSchemaIdentifierNameResolver.DEFAULT_DATASOURCE_NAME; +import static org.opensearch.sql.analysis.NestedAnalyzer.isNestedFunction; import static org.opensearch.sql.ast.dsl.AstDSL.aggregate; import static org.opensearch.sql.ast.dsl.AstDSL.alias; import static org.opensearch.sql.ast.dsl.AstDSL.argument; @@ -39,6 +41,7 @@ import static org.opensearch.sql.data.type.ExprCoreType.LONG; import static org.opensearch.sql.data.type.ExprCoreType.STRING; import static org.opensearch.sql.data.type.ExprCoreType.TIMESTAMP; +import static org.opensearch.sql.expression.DSL.literal; import static org.opensearch.sql.utils.MLCommonsConstants.ACTION; import static org.opensearch.sql.utils.MLCommonsConstants.ALGO; import static org.opensearch.sql.utils.MLCommonsConstants.ASYNC; @@ -574,6 +577,10 @@ public void project_nested_field_arg() { function("nested", qualifiedName("message", "info")), null) ) ); + + assertTrue(isNestedFunction(DSL.nested(DSL.ref("message.info", STRING)))); + assertFalse(isNestedFunction(DSL.literal("fieldA"))); + assertFalse(isNestedFunction(DSL.match(DSL.namedArgument("field", literal("message"))))); } @Test diff --git a/docs/user/dql/functions.rst b/docs/user/dql/functions.rst index fa37dc7778..cef87624a5 100644 --- a/docs/user/dql/functions.rst +++ b/docs/user/dql/functions.rst @@ -4469,6 +4469,17 @@ Example with ``field`` and ``path`` parameters in the SELECT and WHERE clause:: | b | +---------------------------------+ +Example with ``field`` and ``path`` parameters in the SELECT and ORDER BY clause:: + + os> SELECT nested(message.info, message) FROM nested ORDER BY nested(message.info, message) DESC; + fetched rows / total rows = 2/2 + +---------------------------------+ + | nested(message.info, message) | + |---------------------------------| + | b | + | a | + +---------------------------------+ + System Functions ================ diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/NestedIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/NestedIT.java index 6cb7b7580b..9de6f05037 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/NestedIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/NestedIT.java @@ -187,6 +187,40 @@ public void nested_function_with_order_by_clause() { rows("zz")); } + @Test + public void nested_function_with_order_by_clause_desc() { + String query = + "SELECT nested(message.info) FROM " + TEST_INDEX_NESTED_TYPE + + " ORDER BY nested(message.info, message) DESC"; + JSONObject result = executeJdbcRequest(query); + + assertEquals(6, result.getInt("total")); + verifyDataRows(result, + rows("zz"), + rows("c"), + rows("c"), + rows("a"), + rows("b"), + rows("a")); + } + + @Test + public void nested_function_and_field_with_order_by_clause() { + String query = + "SELECT nested(message.info), myNum FROM " + TEST_INDEX_NESTED_TYPE + + " ORDER BY nested(message.info, message), myNum"; + JSONObject result = executeJdbcRequest(query); + + assertEquals(6, result.getInt("total")); + verifyDataRows(result, + rows("a", 1), + rows("c", 4), + rows("a", 4), + rows("b", 2), + rows("c", 3), + rows("zz", new JSONArray(List.of(3, 4)))); + } + // Nested function in GROUP BY clause is not yet implemented for JDBC format. This test ensures // that the V2 engine falls back to legacy implementation. // TODO Fix the test when NESTED is supported in GROUP BY in the V2 engine. diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanBuilder.java index 3a0d06d079..edcbedc7a7 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanBuilder.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanBuilder.java @@ -5,6 +5,8 @@ package org.opensearch.sql.opensearch.storage.scan; +import static org.opensearch.sql.analysis.NestedAnalyzer.isNestedFunction; + import java.util.function.Function; import lombok.EqualsAndHashCode; import org.opensearch.sql.expression.ReferenceExpression; @@ -113,9 +115,15 @@ public boolean pushDownNested(LogicalNested nested) { return delegate.pushDownNested(nested); } + /** + * Valid if sorting is only by fields. + * @param sort Logical sort + * @return True if sorting by fields only + */ private boolean sortByFieldsOnly(LogicalSort sort) { return sort.getSortList().stream() - .map(sortItem -> sortItem.getRight() instanceof ReferenceExpression) + .map(sortItem -> sortItem.getRight() instanceof ReferenceExpression + || isNestedFunction(sortItem.getRight())) .reduce(true, Boolean::logicalAnd); } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java index 4dcfec125e..a45c535383 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java @@ -6,6 +6,8 @@ package org.opensearch.sql.opensearch.storage.script.filter.lucene; +import static org.opensearch.sql.analysis.NestedAnalyzer.isNestedFunction; + import com.google.common.collect.ImmutableMap; import java.util.Map; import java.util.function.Function; @@ -62,10 +64,7 @@ public boolean canSupport(FunctionExpression func) { * @return return true if function has supported nested function expression. */ public boolean isNestedPredicate(FunctionExpression func) { - return ((func.getArguments().get(0) instanceof FunctionExpression - && ((FunctionExpression)func.getArguments().get(0)) - .getFunctionName().getFunctionName().equalsIgnoreCase(BuiltinFunctionName.NESTED.name())) - ); + return isNestedFunction(func.getArguments().get(0)); } /** diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilder.java index 1415fc22c6..46a25368b1 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilder.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilder.java @@ -6,15 +6,21 @@ package org.opensearch.sql.opensearch.storage.script.sort; +import static org.opensearch.sql.analysis.NestedAnalyzer.generatePath; +import static org.opensearch.sql.analysis.NestedAnalyzer.isNestedFunction; + import com.google.common.collect.ImmutableMap; import java.util.Map; import org.opensearch.search.sort.FieldSortBuilder; +import org.opensearch.search.sort.NestedSortBuilder; import org.opensearch.search.sort.SortBuilder; import org.opensearch.search.sort.SortBuilders; import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.ast.tree.Sort; import org.opensearch.sql.expression.Expression; +import org.opensearch.sql.expression.FunctionExpression; import org.opensearch.sql.expression.ReferenceExpression; +import org.opensearch.sql.expression.function.BuiltinFunctionName; import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; /** @@ -53,11 +59,44 @@ public SortBuilder build(Expression expression, Sort.SortOption option) { return SortBuilders.scoreSort().order(sortOrderMap.get(option.getSortOrder())); } return fieldBuild((ReferenceExpression) expression, option); + } else if (isNestedFunction(expression)) { + + validateNestedArgs((FunctionExpression) expression); + String orderByName = ((FunctionExpression)expression).getArguments().get(0).toString(); + // Generate path if argument not supplied in function. + ReferenceExpression path = ((FunctionExpression)expression).getArguments().size() == 2 + ? (ReferenceExpression) ((FunctionExpression)expression).getArguments().get(1) + : generatePath(orderByName); + return SortBuilders.fieldSort(orderByName) + .order(sortOrderMap.get(option.getSortOrder())) + .setNestedSort(new NestedSortBuilder(path.toString())); } else { throw new IllegalStateException("unsupported expression " + expression.getClass()); } } + /** + * Validate semantics for arguments in nested function. + * @param nestedFunc Nested function expression. + */ + private void validateNestedArgs(FunctionExpression nestedFunc) { + if (nestedFunc.getArguments().size() > 2) { + throw new IllegalArgumentException( + "nested function supports 2 parameters (field, path) or 1 parameter (field)" + ); + } + + for (Expression arg : nestedFunc.getArguments()) { + if (!(arg instanceof ReferenceExpression)) { + throw new IllegalArgumentException( + String.format("Illegal nested field name: %s", + arg.toString() + ) + ); + } + } + } + private FieldSortBuilder fieldBuild(ReferenceExpression ref, Sort.SortOption option) { return SortBuilders.fieldSort( OpenSearchTextType.convertTextToKeyword(ref.getAttr(), ref.type())) diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanOptimizationTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanOptimizationTest.java index d5283cecb7..e045bae3e3 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanOptimizationTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanOptimizationTest.java @@ -19,6 +19,7 @@ import static org.opensearch.sql.data.type.ExprCoreType.INTEGER; import static org.opensearch.sql.data.type.ExprCoreType.LONG; import static org.opensearch.sql.data.type.ExprCoreType.STRING; +import static org.opensearch.sql.expression.DSL.literal; import static org.opensearch.sql.planner.logical.LogicalPlanDSL.aggregation; import static org.opensearch.sql.planner.logical.LogicalPlanDSL.filter; import static org.opensearch.sql.planner.logical.LogicalPlanDSL.highlight; @@ -58,6 +59,7 @@ import org.opensearch.search.aggregations.AggregationBuilders; import org.opensearch.search.aggregations.bucket.composite.CompositeAggregationBuilder; import org.opensearch.search.aggregations.bucket.composite.TermsValuesSourceBuilder; +import org.opensearch.search.sort.NestedSortBuilder; import org.opensearch.search.sort.SortBuilder; import org.opensearch.search.sort.SortBuilders; import org.opensearch.search.sort.SortOrder; @@ -574,6 +576,76 @@ void only_one_project_should_be_push() { ); } + @Test + void test_nested_sort_filter_push_down() { + assertEqualsAfterOptimization( + project( + indexScanBuilder( + withFilterPushedDown(QueryBuilders.termQuery("intV", 1)), + withSortPushedDown( + SortBuilders.fieldSort("message.info") + .order(SortOrder.ASC) + .setNestedSort(new NestedSortBuilder("message")))), + DSL.named("intV", DSL.ref("intV", INTEGER)) + ), + project( + sort( + filter( + relation("schema", table), + DSL.equal(DSL.ref("intV", INTEGER), DSL.literal(integerValue(1))) + ), + Pair.of( + SortOption.DEFAULT_ASC, DSL.nested(DSL.ref("message.info", STRING)) + ) + ), + DSL.named("intV", DSL.ref("intV", INTEGER)) + ) + ); + } + + @Test + void test_function_expression_sort_returns_optimized_logical_sort() { + // Invalid use case coverage OpenSearchIndexScanBuilder::sortByFieldsOnly returns false + assertEqualsAfterOptimization( + sort( + indexScanBuilder(), + Pair.of( + SortOption.DEFAULT_ASC, + DSL.match(DSL.namedArgument("field", literal("message"))) + ) + ), + sort( + relation("schema", table), + Pair.of( + SortOption.DEFAULT_ASC, + DSL.match(DSL.namedArgument("field", literal("message")) + ) + ) + ) + ); + } + + @Test + void test_non_field_sort_returns_optimized_logical_sort() { + // Invalid use case coverage OpenSearchIndexScanBuilder::sortByFieldsOnly returns false + assertEqualsAfterOptimization( + sort( + indexScanBuilder(), + Pair.of( + SortOption.DEFAULT_ASC, + DSL.literal("field") + ) + ), + sort( + relation("schema", table), + Pair.of( + SortOption.DEFAULT_ASC, + DSL.literal("field") + ) + ) + ); + } + @Test void sort_with_expression_cannot_merge_with_relation() { assertEqualsAfterOptimization( diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilderTest.java index df6cfae78f..1ad244cecb 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilderTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilderTest.java @@ -10,11 +10,14 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.opensearch.sql.data.type.ExprCoreType.INTEGER; +import static org.opensearch.sql.data.type.ExprCoreType.STRING; import org.hamcrest.Matchers; import org.junit.jupiter.api.Test; import org.opensearch.sql.ast.tree.Sort; +import org.opensearch.sql.data.model.ExprShortValue; import org.opensearch.sql.expression.DSL; +import org.opensearch.sql.expression.LiteralExpression; class SortQueryBuilderTest { @@ -25,6 +28,62 @@ void build_sortbuilder_from_reference() { assertNotNull(sortQueryBuilder.build(DSL.ref("intV", INTEGER), Sort.SortOption.DEFAULT_ASC)); } + @Test + void build_sortbuilder_from_nested_function() { + assertNotNull( + sortQueryBuilder.build( + DSL.nested(DSL.ref("message.info", STRING)), + Sort.SortOption.DEFAULT_ASC + ) + ); + } + + @Test + void build_sortbuilder_from_nested_function_with_path_param() { + assertNotNull( + sortQueryBuilder.build( + DSL.nested(DSL.ref("message.info", STRING), DSL.ref("message", STRING)), + Sort.SortOption.DEFAULT_ASC + ) + ); + } + + @Test + void nested_with_too_many_args_throws_exception() { + assertThrows( + IllegalArgumentException.class, + () -> sortQueryBuilder.build( + DSL.nested( + DSL.ref("message.info", STRING), + DSL.ref("message", STRING), + DSL.ref("message", STRING) + ), + Sort.SortOption.DEFAULT_ASC + ) + ); + } + + @Test + void nested_with_invalid_arg_type_throws_exception() { + assertThrows( + IllegalArgumentException.class, + () -> sortQueryBuilder.build( + DSL.nested( + DSL.literal(1) + ), + Sort.SortOption.DEFAULT_ASC + ) + ); + } + + @Test + void build_sortbuilder_from_expression_should_throw_exception() { + final IllegalStateException exception = + assertThrows(IllegalStateException.class, () -> sortQueryBuilder.build( + new LiteralExpression(new ExprShortValue(1)), Sort.SortOption.DEFAULT_ASC)); + assertThat(exception.getMessage(), Matchers.containsString("unsupported expression")); + } + @Test void build_sortbuilder_from_function_should_throw_exception() { final IllegalStateException exception = diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstSortBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstSortBuilder.java index 993bc10615..1b872dce54 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstSortBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstSortBuilder.java @@ -17,17 +17,12 @@ import lombok.RequiredArgsConstructor; import org.opensearch.sql.ast.expression.Argument; import org.opensearch.sql.ast.expression.Field; -import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.ast.tree.Sort; import org.opensearch.sql.ast.tree.Sort.NullOrder; import org.opensearch.sql.ast.tree.Sort.SortOption; import org.opensearch.sql.ast.tree.Sort.SortOrder; import org.opensearch.sql.ast.tree.UnresolvedPlan; -import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; -import org.opensearch.sql.expression.FunctionExpression; -import org.opensearch.sql.expression.function.BuiltinFunctionName; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParserBaseVisitor; import org.opensearch.sql.sql.parser.context.QuerySpecification; @@ -53,15 +48,6 @@ private List createSortFields() { List items = querySpec.getOrderByItems(); List options = querySpec.getOrderByOptions(); for (int i = 0; i < items.size(); i++) { - // TODO remove me when Nested function is supported in ORDER BY clause. - if (items.get(i) instanceof Function - && ((Function)items.get(i)).getFuncName().equalsIgnoreCase( - BuiltinFunctionName.NESTED.name()) - ) { - throw new SyntaxCheckException( - "Falling back to legacy engine. Nested function is not supported in ORDER BY clause." - ); - } fields.add( new Field( querySpec.replaceIfAliasOrOrdinal(items.get(i)), diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java index 39fe8811b5..6d43daa60f 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java @@ -638,13 +638,28 @@ public void can_parse_wildcard_query_relevance_function() { @Test public void can_parse_nested_function() { assertNotNull( - parser.parse("SELECT NESTED(FIELD.DAYOFWEEK) FROM TEST")); + parser.parse("SELECT NESTED(PATH.INNER_FIELD) FROM TEST")); assertNotNull( - parser.parse("SELECT NESTED('FIELD.DAYOFWEEK') FROM TEST")); + parser.parse("SELECT NESTED('PATH.INNER_FIELD') FROM TEST")); assertNotNull( - parser.parse("SELECT SUM(NESTED(FIELD.SUBFIELD)) FROM TEST")); + parser.parse("SELECT SUM(NESTED(PATH.INNER_FIELD)) FROM TEST")); assertNotNull( - parser.parse("SELECT NESTED(FIELD.DAYOFWEEK, PATH) FROM TEST")); + parser.parse("SELECT NESTED(PATH.INNER_FIELD, PATH) FROM TEST")); + assertNotNull( + parser.parse( + "SELECT * FROM TEST WHERE NESTED(PATH.INNER_FIELDS) = 'A'" + ) + ); + assertNotNull( + parser.parse( + "SELECT * FROM TEST WHERE NESTED(PATH.INNER_FIELDS, PATH) = 'A'" + ) + ); + assertNotNull( + parser.parse( + "SELECT FIELD FROM TEST ORDER BY nested(PATH.INNER_FIELD, PATH)" + ) + ); } @Test 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 0c69909334..e017bd8cd6 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 @@ -462,21 +462,6 @@ public void can_build_order_by_null_option() { buildAST("SELECT name FROM test ORDER BY name NULLS LAST")); } - /** - * Ensure Nested function falls back to legacy engine when used in an ORDER BY clause. - * TODO Remove this test when support is added. - */ - @Test - public void nested_in_order_by_clause_throws_exception() { - SyntaxCheckException exception = assertThrows(SyntaxCheckException.class, - () -> buildAST("SELECT * FROM test ORDER BY nested(message.info)") - ); - - assertEquals( - "Falling back to legacy engine. Nested function is not supported in ORDER BY clause.", - exception.getMessage()); - } - /** * Ensure Nested function falls back to legacy engine when used in an HAVING clause. * TODO Remove this test when support is added.