Changed visitor type to Boolean and added LIMIT to fall back to legacy

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<Void, JsonSupportVisitorContext> {
+public class JsonSupportVisitor extends AbstractNodeVisitor<Boolean, JsonSupportVisitorContext> {
   @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;
   }
 }

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<String> unsupportedNodes = new ArrayList<>();
+
+  public void addToUnsupportedNodes(String unsupportedNode) {
+    unsupportedNodes.add(unsupportedNode);
+  }
 }

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;
   }
 

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,78 +24,73 @@ 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
   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
   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
   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
   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
   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
   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()));
   }
 }

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.