From e2b970740d859a2d8c561ea1962d818637acbe54 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Tue, 28 Mar 2023 14:25:36 -0700 Subject: [PATCH 1/2] Changed visitor type to Boolean and added LIMIT to fall back to legacy Signed-off-by: Guian Gumpac --- .../sql/analysis/JsonSupportVisitor.java | 73 ++++++++++--------- .../analysis/JsonSupportVisitorContext.java | 10 +++ .../sql/ast/AbstractNodeVisitor.java | 2 +- .../sql/analysis/JsonSupportVisitorTest.java | 50 ++++++++----- .../opensearch/sql/legacy/DateFormatIT.java | 10 +-- .../org/opensearch/sql/legacy/QueryIT.java | 12 +-- .../org/opensearch/sql/sql/SQLService.java | 9 ++- 7 files changed, 99 insertions(+), 67 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java index 0155e1c56e7..c611f988ce7 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java @@ -13,6 +13,7 @@ import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.tree.Aggregation; +import org.opensearch.sql.ast.tree.Limit; import org.opensearch.sql.ast.tree.Project; /** @@ -21,86 +22,90 @@ * Unsupported features in V2 are ones the produce results that differ from * legacy results. */ -public class JsonSupportVisitor extends AbstractNodeVisitor { +public class JsonSupportVisitor extends AbstractNodeVisitor { @Override - public Void visit(Node node, JsonSupportVisitorContext context) { - visitChildren(node, context); - return null; + public Boolean visit(Node node, JsonSupportVisitorContext context) { + // UnresolvedPlan can only have one child until joins are supported + return node.getChild().get(0).accept(this, context); } @Override - public Void visitChildren(Node node, JsonSupportVisitorContext context) { - for (Node child : node.getChild()) { - child.accept(this, context); - } - return null; + protected Boolean defaultResult() { + return Boolean.TRUE; + } + + @Override + public Boolean visitLimit(Limit node, JsonSupportVisitorContext context) { + context.addToUnsupportedNodes("limit"); + return Boolean.FALSE; } @Override - public Void visitAggregation(Aggregation node, JsonSupportVisitorContext context) { + public Boolean visitAggregation(Aggregation node, JsonSupportVisitorContext context) { if (!node.getGroupExprList().isEmpty()) { - throw new UnsupportedOperationException( - "Queries with aggregation are not yet supported with json format in the new engine"); + context.addToUnsupportedNodes("aggregation"); + return Boolean.FALSE; } - return null; + return Boolean.TRUE; } @Override - public Void visitFunction(Function node, JsonSupportVisitorContext context) { + public Boolean visitFunction(Function node, JsonSupportVisitorContext context) { // Supported if outside of Project if (context.isVisitingProject()) { // queries with function calls are not supported. - throw new UnsupportedOperationException( - "Queries with functions are not yet supported with json format in the new engine"); + context.addToUnsupportedNodes("functions"); + return Boolean.FALSE; } - return null; + return Boolean.TRUE; } @Override - public Void visitLiteral(Literal node, JsonSupportVisitorContext context) { + public Boolean visitLiteral(Literal node, JsonSupportVisitorContext context) { // Supported if outside of Project if (context.isVisitingProject()) { // queries with literal values are not supported - throw new UnsupportedOperationException( - "Queries with literals are not yet supported with json format in the new engine"); + context.addToUnsupportedNodes("literal"); + return Boolean.FALSE; } - return null; + return Boolean.TRUE; } @Override - public Void visitCast(Cast node, JsonSupportVisitorContext context) { + public Boolean visitCast(Cast node, JsonSupportVisitorContext context) { // Supported if outside of Project if (context.isVisitingProject()) { // Queries with cast are not supported - throw new UnsupportedOperationException( - "Queries with casts are not yet supported with json format in the new engine"); + context.addToUnsupportedNodes("cast"); + return Boolean.FALSE; } - return null; + return Boolean.TRUE; } @Override - public Void visitAlias(Alias node, JsonSupportVisitorContext context) { + public Boolean visitAlias(Alias node, JsonSupportVisitorContext context) { // Supported if outside of Project if (context.isVisitingProject()) { // Alias node is accepted if it does not have a user-defined alias // and if the delegated expression is accepted. if (StringUtils.isEmpty(node.getAlias())) { - node.getDelegated().accept(this, context); + return node.getDelegated().accept(this, context); } else { - throw new UnsupportedOperationException( - "Queries with aliases are not yet supported with json format in the new engine"); + context.addToUnsupportedNodes("alias"); + return Boolean.FALSE; } } - return null; + return Boolean.TRUE; } @Override - public Void visitProject(Project node, JsonSupportVisitorContext context) { - visit(node, context); + public Boolean visitProject(Project node, JsonSupportVisitorContext context) { + Boolean isSupported = visit(node, context); context.setVisitingProject(true); - node.getProjectList().forEach(e -> e.accept(this, context)); + isSupported = node.getProjectList().stream() + .allMatch(e -> e.accept(this, context)) && isSupported; context.setVisitingProject(false); - return null; + return isSupported; } } diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java index 004e1a653b7..f50596fdd58 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java @@ -5,6 +5,8 @@ package org.opensearch.sql.analysis; +import java.util.ArrayList; +import java.util.List; import lombok.Getter; import lombok.Setter; @@ -15,4 +17,12 @@ public class JsonSupportVisitorContext { @Getter @Setter private boolean isVisitingProject = false; + + @Getter + @Setter + private List unsupportedNodes = new ArrayList<>(); + + public void addToUnsupportedNodes(String unsupportedNode) { + unsupportedNodes.add(unsupportedNode); + } } 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 393de051649..f4325fa7f64 100644 --- a/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java @@ -82,7 +82,7 @@ public T visitChildren(Node node, C context) { return result; } - private T defaultResult() { + protected T defaultResult() { return null; } diff --git a/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java b/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java index edd4fd99caf..3409900450a 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java @@ -5,8 +5,8 @@ package org.opensearch.sql.analysis; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; import com.google.common.collect.ImmutableList; @@ -15,6 +15,7 @@ import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.ast.tree.Limit; import org.opensearch.sql.ast.tree.UnresolvedPlan; class JsonSupportVisitorTest { @@ -23,14 +24,13 @@ public void visitLiteralInProject() { UnresolvedPlan project = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.intLiteral(1)); - assertThrows(UnsupportedOperationException.class, - () -> project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test public void visitLiteralOutsideProject() { Literal intLiteral = AstDSL.intLiteral(1); - assertNull(intLiteral.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertTrue(intLiteral.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -38,8 +38,7 @@ public void visitCastInProject() { UnresolvedPlan project = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.cast(AstDSL.intLiteral(1), AstDSL.stringLiteral("INT"))); - assertThrows(UnsupportedOperationException.class, - () -> project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -47,7 +46,7 @@ public void visitCastOutsideProject() { UnresolvedExpression intCast = AstDSL.cast( AstDSL.intLiteral(1), AstDSL.stringLiteral("INT")); - assertNull(intCast.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertTrue(intCast.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -55,8 +54,7 @@ public void visitAliasInProject() { UnresolvedPlan project = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.alias("alias", AstDSL.intLiteral(1))); - assertThrows(UnsupportedOperationException.class, - () -> project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -64,8 +62,7 @@ public void visitAliasInProjectWithUnsupportedDelegated() { UnresolvedPlan project = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.alias("alias", AstDSL.intLiteral(1), "alias")); - assertThrows(UnsupportedOperationException.class, - () -> project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -73,13 +70,13 @@ public void visitAliasInProjectWithSupportedDelegated() { UnresolvedPlan project = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.alias("alias", AstDSL.field("field"))); - assertNull(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertTrue(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test public void visitAliasOutsideProject() { UnresolvedExpression alias = AstDSL.alias("alias", AstDSL.intLiteral(1)); - assertNull(alias.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertTrue(alias.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -87,14 +84,13 @@ public void visitFunctionInProject() { UnresolvedPlan function = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.function("abs", AstDSL.intLiteral(-1))); - assertThrows(UnsupportedOperationException.class, - () -> function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertFalse(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test public void visitFunctionOutsideProject() { UnresolvedExpression function = AstDSL.function("abs", AstDSL.intLiteral(-1)); - assertNull(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertTrue(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -105,8 +101,7 @@ public void visitAggregationWithGroupExprList() { Collections.emptyList(), ImmutableList.of(AstDSL.alias("alias", qualifiedName("integer_value"))), Collections.emptyList())); - assertThrows(UnsupportedOperationException.class, - () -> projectAggr.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertFalse(projectAggr.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -120,6 +115,21 @@ public void visitAggregationWithAggExprList() { Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); - assertNull(aggregation.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertTrue(aggregation.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitLimit() { + Limit limit = AstDSL.limit(AstDSL.relation("table", "table"), 10, 5); + assertFalse(limit.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitWithMultipleUnsupportedProjectNodes() { + UnresolvedPlan plan = AstDSL.project( + AstDSL.relation("table", "table"), + AstDSL.function("abs", AstDSL.intLiteral(-1)), + AstDSL.alias("alias", AstDSL.intLiteral(1))); + assertFalse(plan.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } } diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java index 3d15d88ce06..770318a577c 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java @@ -75,7 +75,7 @@ public void lessThan() throws SqlParseException { // DATE_FORMAT with timezone argument is not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 - @Test(expected = SqlParseException.class) + @Test public void lessThanOrEqualTo() throws SqlParseException { assertThat( dateQuery( @@ -99,7 +99,7 @@ public void greaterThan() throws SqlParseException { // DATE_FORMAT with timezone argument is not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 - @Test(expected = SqlParseException.class) + @Test public void greaterThanOrEqualTo() throws SqlParseException { assertThat( dateQuery( @@ -112,7 +112,7 @@ public void greaterThanOrEqualTo() throws SqlParseException { // DATE_FORMAT with timezone argument is not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 - @Test(expected = SqlParseException.class) + @Test public void and() throws SqlParseException { assertThat( dateQuery(SELECT_FROM + @@ -137,7 +137,7 @@ public void andWithDefaultTimeZone() throws SqlParseException { // DATE_FORMAT with timezone argument is not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 - @Test(expected = SqlParseException.class) + @Test public void or() throws SqlParseException { assertThat( dateQuery(SELECT_FROM + @@ -151,7 +151,7 @@ public void or() throws SqlParseException { // DATE_FORMAT with timezone argument is not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 - @Test(expected = ResponseException.class) + @Test public void sortByDateFormat() throws IOException { // Sort by expression in descending order, but sort inside in ascending order, so we increase our confidence // that successful test isn't just random chance. diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java index 679596f67f6..d70d3d1ff55 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java @@ -104,7 +104,7 @@ public void multipleFromTest() throws IOException { // Duplicated column names like SELECT *, age is not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/785 - @Test(expected = ResponseException.class) + @Test public void selectAllWithFieldReturnsAll() throws IOException { JSONObject response = executeQuery(StringUtils.format( "SELECT *, age " + @@ -118,7 +118,7 @@ public void selectAllWithFieldReturnsAll() throws IOException { // Duplicated column names like SELECT *, age is not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/785 - @Test(expected = ResponseException.class) + @Test public void selectAllWithFieldReverseOrder() throws IOException { JSONObject response = executeQuery(StringUtils.format( "SELECT *, age " + @@ -132,7 +132,7 @@ public void selectAllWithFieldReverseOrder() throws IOException { // Duplicated column names like SELECT *, age is not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/785 - @Test(expected = ResponseException.class) + @Test public void selectAllWithMultipleFields() throws IOException { JSONObject response = executeQuery(StringUtils.format( "SELECT *, age, address " + @@ -146,7 +146,7 @@ public void selectAllWithMultipleFields() throws IOException { // Duplicated column names like SELECT *, age is not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/785 - @Test(expected = ResponseException.class) + @Test public void selectAllWithFieldAndOrderBy() throws IOException { JSONObject response = executeQuery(StringUtils.format( "SELECT *, age " + @@ -782,7 +782,7 @@ public void dateSearchBraces() throws IOException { // Unsupported date format in V2 // Can be tracked through https://github.com/opensearch-project/sql/issues/856 - @Test(expected = ResponseException.class) + @Test public void dateBetweenSearch() throws IOException { DateTimeFormatter formatter = DateTimeFormat.forPattern(TestsConstants.DATE_FORMAT); @@ -1284,7 +1284,7 @@ public void queryWithDotAtStartOfIndexName() throws Exception { // Objects in IS NOT NULL not yet supported in V2 // Can be tracked with https://github.com/opensearch-project/sql/issues/1425 - @Test(expected = ResponseException.class) + @Test public void notLikeTests() throws IOException { JSONObject response = executeQuery( String.format(Locale.ROOT, "SELECT name " + diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 0f2e38629d3..93ab0b71268 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -7,6 +7,7 @@ package org.opensearch.sql.sql; import java.util.Optional; +import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import org.antlr.v4.runtime.tree.ParseTree; import org.opensearch.sql.analysis.JsonSupportVisitor; @@ -87,7 +88,13 @@ private AbstractPlan plan( // Go through the tree and throw exceptions when unsupported JsonSupportVisitorContext jsonSupportVisitorContext = new JsonSupportVisitorContext(); - ((Query) statement).getPlan().accept(new JsonSupportVisitor(), jsonSupportVisitorContext); + if (!((Query) statement).getPlan().accept(new JsonSupportVisitor(), + jsonSupportVisitorContext)) { + throw new UnsupportedOperationException( + "The following features are not supported with JSON format: " + .concat(jsonSupportVisitorContext.getUnsupportedNodes().stream() + .collect(Collectors.joining(", ")))); + } } return queryExecutionFactory.create(statement, queryListener, explainListener); From b7cd2a699d81fa94b5e9b739de9ff429b246d66f Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Tue, 28 Mar 2023 14:47:31 -0700 Subject: [PATCH 2/2] Addressed PR comment Signed-off-by: Guian Gumpac --- .../java/org/opensearch/sql/planner/physical/PhysicalPlan.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlan.java b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlan.java index 21ded9df53b..068260ea331 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlan.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlan.java @@ -7,6 +7,7 @@ package org.opensearch.sql.planner.physical; import java.util.Iterator; +import org.apache.commons.lang3.StringUtils; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.executor.ExecutionEngine; import org.opensearch.sql.planner.PlanNode; @@ -48,6 +49,6 @@ public ExecutionEngine.Schema schema() { public String getRawResponse() { return getChild().stream().map(PhysicalPlan::getRawResponse) - .filter(r -> r != null && !r.isEmpty()).findFirst().orElse(""); + .filter(StringUtils::isNotEmpty).findFirst().orElse(""); } }