From be853e6f50a4e5f0c9681745f07a9822007e64fc Mon Sep 17 00:00:00 2001 From: forestmvey Date: Tue, 18 Apr 2023 14:43:51 -0700 Subject: [PATCH] Fixing bug where Nested functions used in WHERE and GROUP BY clauses don't fallback to legacy engine. Signed-off-by: forestmvey --- .../org/opensearch/sql/analysis/Analyzer.java | 29 ++++++++++- .../opensearch/sql/analysis/AnalyzerTest.java | 48 +++++++++++++++++++ .../java/org/opensearch/sql/sql/NestedIT.java | 27 ++++++++++- 3 files changed, 102 insertions(+), 2 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 4ea298a589..e7212f692b 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java @@ -60,6 +60,7 @@ import org.opensearch.sql.ast.tree.TableFunction; import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.ast.tree.Values; +import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.data.model.ExprMissingValue; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; @@ -67,11 +68,13 @@ 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.LiteralExpression; import org.opensearch.sql.expression.NamedExpression; import org.opensearch.sql.expression.ReferenceExpression; import org.opensearch.sql.expression.aggregation.Aggregator; import org.opensearch.sql.expression.aggregation.NamedAggregator; +import org.opensearch.sql.expression.function.BuiltinFunctionName; import org.opensearch.sql.expression.function.BuiltinFunctionRepository; import org.opensearch.sql.expression.function.FunctionName; import org.opensearch.sql.expression.function.TableFunctionImplementation; @@ -217,6 +220,7 @@ public LogicalPlan visitLimit(Limit node, AnalysisContext context) { public LogicalPlan visitFilter(Filter node, AnalysisContext context) { LogicalPlan child = node.getChild().get(0).accept(this, context); Expression condition = expressionAnalyzer.analyze(node.getCondition(), context); + verifySupportsCondition(condition); ExpressionReferenceOptimizer optimizer = new ExpressionReferenceOptimizer(expressionAnalyzer.getRepository(), child); @@ -224,6 +228,27 @@ public LogicalPlan visitFilter(Filter node, AnalysisContext context) { return new LogicalFilter(child, optimized); } + /** + * Ensure NESTED function is not used in WHERE clause. Fallback to legacy engine. Can remove when + * support is added for NESTED function in WHERE and GROUP BY clauses. + * @param condition : Filter condition + */ + private void verifySupportsCondition(Expression condition) { + if (condition instanceof FunctionExpression) { + if (((FunctionExpression) condition).getFunctionName().getFunctionName().equalsIgnoreCase( + BuiltinFunctionName.NESTED.name() + )) { + throw new SyntaxCheckException( + "Falling back to legacy engine. Nested function is not supported in WHERE" + + " and GROUP BY clauses." + ); + } + ((FunctionExpression)condition).getArguments().stream() + .forEach(e -> verifySupportsCondition(e) + ); + } + } + /** * Build {@link LogicalRename}. */ @@ -273,7 +298,9 @@ public LogicalPlan visitAggregation(Aggregation node, AnalysisContext context) { } for (UnresolvedExpression expr : node.getGroupExprList()) { - groupbyBuilder.add(namedExpressionAnalyzer.analyze(expr, context)); + NamedExpression resolvedExpr = namedExpressionAnalyzer.analyze(expr, context); + verifySupportsCondition(resolvedExpr.getDelegated()); + groupbyBuilder.add(resolvedExpr); } ImmutableList groupBys = groupbyBuilder.build(); 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 e8a1c6eb3f..aaca6d49af 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java @@ -79,6 +79,7 @@ import org.opensearch.sql.ast.tree.ML; import org.opensearch.sql.ast.tree.RareTopN.CommandType; import org.opensearch.sql.ast.tree.UnresolvedPlan; +import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.exception.ExpressionEvaluationException; import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.DSL; @@ -1016,6 +1017,53 @@ public void select_all_from_subquery() { ); } + /** + * Ensure Nested function falls back to legacy engine when used in GROUP BY clause. + * TODO Remove this test when support is added. + */ + @Test + public void nested_group_by_clause_throws_syntax_exception() { + SyntaxCheckException exception = assertThrows(SyntaxCheckException.class, + () -> analyze( + AstDSL.project( + AstDSL.agg( + AstDSL.relation("schema"), + emptyList(), + emptyList(), + ImmutableList.of(alias("nested(message.info)", + function("nested", + qualifiedName("message", "info")))), + emptyList() + ))) + ); + assertEquals("Falling back to legacy engine. Nested function is not supported in WHERE" + + " and GROUP BY clauses.", + exception.getMessage()); + } + + /** + * Ensure Nested function falls back to legacy engine when used in WHERE clause. + * TODO Remove this test when support is added. + */ + @Test + public void nested_where_clause_throws_syntax_exception() { + SyntaxCheckException exception = assertThrows(SyntaxCheckException.class, + () -> analyze( + AstDSL.filter( + AstDSL.relation("schema"), + AstDSL.equalTo( + AstDSL.function("nested", qualifiedName("message", "info")), + AstDSL.stringLiteral("str") + ) + ) + ) + ); + assertEquals("Falling back to legacy engine. Nested function is not supported in WHERE" + + " and GROUP BY clauses.", + exception.getMessage()); + } + + /** * SELECT name, AVG(age) FROM test GROUP BY name. */ 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 a7b5d46234..e2acb0fc17 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 @@ -17,7 +17,6 @@ import java.io.IOException; import java.util.List; import java.util.Map; - import org.json.JSONArray; import org.json.JSONObject; import org.junit.Test; @@ -171,6 +170,32 @@ public void nested_function_mixed_with_non_nested_type_test() { rows("zz", "a")); } + @Test + public void nested_function_with_where_clause() { + String query = + "SELECT nested(message.info) FROM " + TEST_INDEX_NESTED_TYPE + " WHERE nested(message.info) = 'a'"; + JSONObject result = executeJdbcRequest(query); + + assertEquals(2, result.getInt("total")); + verifyDataRows(result, + rows("a"), + rows("a")); + } + + // 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. + @Test + public void nested_function_with_group_by_clause() { + String query = + "SELECT count(*) FROM " + TEST_INDEX_NESTED_TYPE + " GROUP BY nested(message.info)"; + JSONObject result = executeJdbcRequest(query); + + assertTrue(result.getJSONObject("error").get("details").toString().contains( + "Aggregation type nested is not yet implemented" + )); + } + @Test public void nested_function_mixed_with_non_nested_types_test() { String query =