Pagination, phase 1: Add unit tests for :core module with coverage.

core/build.gradle

@@ -56,6 +56,7 @@ dependencies {
     testImplementation('org.junit.jupiter:junit-jupiter:5.6.2')
     testImplementation group: 'org.hamcrest', name: 'hamcrest-library', version: '2.1'
     testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.12.4'
+    testImplementation group: 'org.mockito', name: 'mockito-inline', version: '3.12.4'
     testImplementation group: 'org.mockito', name: 'mockito-junit-jupiter', version: '3.12.4'
 }
 

core/src/main/java/org/opensearch/sql/executor/CanPaginateVisitor.java

@@ -39,13 +39,13 @@ public Boolean visitRelation(Relation node, Object context) {
     return Boolean.TRUE;
   }
 
+  /*
   private Boolean canPaginate(Node node, Object context) {
     AtomicBoolean result = new AtomicBoolean(true);
     node.getChild().forEach(n -> result.set(result.get() && n.accept(this, context)));
     return result.get();
   }
 
-  /*
   For queries without `FROM` clause.
   Required to overload `toCursor` function in `ValuesOperator` and modify cursor parsing.
   @Override

core/src/main/java/org/opensearch/sql/executor/PaginatedPlanCache.java

@@ -47,7 +47,7 @@ static class SerializationContext {
   public Cursor convertToCursor(PhysicalPlan plan) {
     if (plan instanceof PaginateOperator) {
       var cursor = plan.toCursor();
-      if (cursor == null || cursor.isEmpty()) {
+      if (cursor == null) {
         return Cursor.None;
       }
       var raw = CURSOR_PREFIX + compress(cursor);
@@ -67,7 +67,6 @@ public static String compress(String str) {
     if (str == null || str.length() == 0) {
       return null;
     }
-
     ByteArrayOutputStream out = new ByteArrayOutputStream();
     GZIPOutputStream gzip = new GZIPOutputStream(out);
     gzip.write(str.getBytes());
@@ -123,6 +122,7 @@ public PhysicalPlan convertToPlan(String cursor) {
         if (!cursor.startsWith("(Paginate,")) {
           throw new UnsupportedOperationException("Unsupported cursor");
         }
+        // TODO add checks for > 0
         cursor = cursor.substring(cursor.indexOf(',') + 1);
         final int currentPageIndex = Integer.parseInt(cursor, 0, cursor.indexOf(','), 10);
 

core/src/main/java/org/opensearch/sql/executor/execution/ContinuePaginatedPlan.java

@@ -49,7 +49,8 @@ public void execute() {
   }
 
   @Override
+  // TODO why can't use listener given in the constructor?
   public void explain(ResponseListener<ExecutionEngine.ExplainResponse> listener) {
-    throw new NotImplementedException("Explain of query continuation is not supported");
+    throw new UnsupportedOperationException("Explain of query continuation is not supported");
   }
 }

core/src/main/java/org/opensearch/sql/executor/execution/PaginatedPlan.java

@@ -5,6 +5,7 @@
 
 package org.opensearch.sql.executor.execution;
 
+import org.apache.commons.lang3.NotImplementedException;
 import org.opensearch.sql.ast.tree.Paginate;
 import org.opensearch.sql.ast.tree.UnresolvedPlan;
 import org.opensearch.sql.common.response.ResponseListener;
@@ -38,8 +39,9 @@ public void execute() {
   }
 
   @Override
+  // TODO why can't use listener given in the constructor?
   public void explain(ResponseListener<ExecutionEngine.ExplainResponse> listener) {
-    listener.onFailure(new UnsupportedOperationException(
+    listener.onFailure(new NotImplementedException(
         "`explain` feature for paginated requests is not implemented yet."));
   }
 }

core/src/main/java/org/opensearch/sql/executor/execution/PaginatedQueryService.java

@@ -46,11 +46,7 @@ public void executePlan(LogicalPlan plan,
    */
   public void executePlan(PhysicalPlan plan,
                           ResponseListener<ExecutionEngine.QueryResponse> listener) {
-    try {
-      executionEngine.execute(plan, ExecutionContext.emptyExecutionContext(), listener);
-    } catch (Exception e) {
-      listener.onFailure(e);
-    }
+    executionEngine.execute(plan, ExecutionContext.emptyExecutionContext(), listener);
   }
 
   public LogicalPlan analyze(UnresolvedPlan plan) {

core/src/main/java/org/opensearch/sql/planner/PaginateOperator.java

@@ -54,6 +54,7 @@ public boolean hasNext() {
   @Override
   public void open() {
     super.open();
+    // TODO numReturned set to 0 for each new object. Do plans support re-opening?
     numReturned = 0;
   }
 
@@ -69,7 +70,10 @@ public List<PhysicalPlan> getChild() {
 
   @Override
   public ExecutionEngine.Schema schema() {
-    assert input instanceof ProjectOperator;
+    // TODO remove assert or do in constructor
+    if (!(input instanceof ProjectOperator)) {
+      throw new UnsupportedOperationException();
+    }
     return input.schema();
   }
 

core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java

@@ -75,6 +75,7 @@
 import org.opensearch.sql.ast.tree.AD;
 import org.opensearch.sql.ast.tree.Kmeans;
 import org.opensearch.sql.ast.tree.ML;
+import org.opensearch.sql.ast.tree.Paginate;
 import org.opensearch.sql.ast.tree.RareTopN.CommandType;
 import org.opensearch.sql.exception.ExpressionEvaluationException;
 import org.opensearch.sql.exception.SemanticCheckException;
@@ -83,6 +84,7 @@
 import org.opensearch.sql.expression.window.WindowDefinition;
 import org.opensearch.sql.planner.logical.LogicalAD;
 import org.opensearch.sql.planner.logical.LogicalMLCommons;
+import org.opensearch.sql.planner.logical.LogicalPaginate;
 import org.opensearch.sql.planner.logical.LogicalPlan;
 import org.opensearch.sql.planner.logical.LogicalPlanDSL;
 import org.opensearch.sql.planner.logical.LogicalProject;
@@ -1189,4 +1191,11 @@ public void ml_relation_predict_rcf_without_time_field() {
     assertTrue(((LogicalProject) actual).getProjectList()
             .contains(DSL.named(RCF_ANOMALOUS, DSL.ref(RCF_ANOMALOUS, BOOLEAN))));
   }
+
+  @Test
+  public void visit_paginate() {
+    LogicalPlan actual = analyze(new Paginate(10, AstDSL.relation("dummy")));
+    assertTrue(actual instanceof LogicalPaginate);
+    assertEquals(10, ((LogicalPaginate) actual).getPageSize());
+  }
 }

core/src/test/java/org/opensearch/sql/executor/CanPaginateVisitorTest.java

@@ -0,0 +1,131 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.sql.executor;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
+
+import java.util.List;
+import org.junit.jupiter.api.DisplayNameGeneration;
+import org.junit.jupiter.api.DisplayNameGenerator;
+import org.junit.jupiter.api.Test;
+import org.opensearch.sql.ast.dsl.AstDSL;
+import org.opensearch.sql.ast.tree.Project;
+import org.opensearch.sql.ast.tree.Relation;
+
+@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
+public class CanPaginateVisitorTest {
+
+  static final CanPaginateVisitor visitor = new CanPaginateVisitor();
+
+  @Test
+  // select * from y
+  public void accept_query_with_select_star_and_from() {
+    var plan = AstDSL.project(AstDSL.relation("dummy"), AstDSL.allFields());
+    assertTrue(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select x from y
+  public void reject_query_with_select_field_and_from() {
+    var plan = AstDSL.project(AstDSL.relation("dummy"), AstDSL.field("pewpew"));
+    assertFalse(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select x,z from y
+  public void reject_query_with_select_fields_and_from() {
+    var plan = AstDSL.project(AstDSL.relation("dummy"),
+        AstDSL.field("pewpew"), AstDSL.field("pewpew"));
+    assertFalse(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select x
+  public void reject_query_without_from() {
+    var plan = AstDSL.project(AstDSL.values(List.of(AstDSL.intLiteral(1))),
+        AstDSL.alias("1",AstDSL.intLiteral(1)));
+    assertFalse(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select * from y limit z
+  public void reject_query_with_limit() {
+    var plan = AstDSL.project(AstDSL.limit(AstDSL.relation("dummy"), 1, 2), AstDSL.allFields());
+    assertFalse(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select * from y where z
+  public void reject_query_with_where() {
+    var plan = AstDSL.project(AstDSL.filter(AstDSL.relation("dummy"),
+        AstDSL.booleanLiteral(true)), AstDSL.allFields());
+    assertFalse(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select * from y order by z
+  public void reject_query_with_order_by() {
+    var plan = AstDSL.project(AstDSL.sort(AstDSL.relation("dummy"), AstDSL.field("1")),
+        AstDSL.allFields());
+    assertFalse(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select * from y group by z
+  public void reject_query_with_group_by() {
+    var plan = AstDSL.project(AstDSL.agg(
+        AstDSL.relation("dummy"), List.of(), List.of(), List.of(AstDSL.field("1")), List.of()),
+        AstDSL.allFields());
+    assertFalse(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select agg(x) from y
+  public void reject_query_with_aggregation_function() {
+    var plan = AstDSL.project(AstDSL.agg(
+        AstDSL.relation("dummy"),
+        List.of(AstDSL.alias("agg", AstDSL.aggregate("func", AstDSL.field("pewpew")))),
+        List.of(), List.of(), List.of()),
+        AstDSL.allFields());
+    assertFalse(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select window(x) from y
+  public void reject_query_with_window_function() {
+    var plan = AstDSL.project(AstDSL.relation("dummy"),
+        AstDSL.alias("pewpew",
+            AstDSL.window(
+                AstDSL.aggregate("func", AstDSL.field("pewpew")),
+                    List.of(AstDSL.qualifiedName("1")), List.of())));
+    assertFalse(plan.accept(visitor, null));
+  }
+
+  @Test
+  // select * from y, z
+  public void reject_query_with_select_from_multiple_indices() {
+    var plan = mock(Project.class);
+    when(plan.getChild()).thenReturn(List.of(AstDSL.relation("dummy"), AstDSL.relation("pummy")));
+    when(plan.getProjectList()).thenReturn(List.of(AstDSL.allFields()));
+    assertFalse(visitor.visitProject(plan, null));
+  }
+
+  @Test
+  // unreal case, added for coverage only
+  public void reject_project_when_relation_has_child() {
+    var relation = mock(Relation.class, withSettings().useConstructor(AstDSL.qualifiedName("42")));
+    when(relation.getChild()).thenReturn(List.of(AstDSL.relation("pewpew")));
+    when(relation.accept(visitor, null)).thenCallRealMethod();
+    var plan = mock(Project.class);
+    when(plan.getChild()).thenReturn(List.of(relation));
+    when(plan.getProjectList()).thenReturn(List.of(AstDSL.allFields()));
+    assertFalse(visitor.visitProject((Project) plan, null));
+  }
+}