Skip to content

Commit

Permalink
Pagination, phase 1: Add unit tests for :core module with coverage. (
Browse files Browse the repository at this point in the history
…#230)

* Add unit tests for `:core` module with coverage. Uncovered: `toCursor`, because it is will be changed soon.

Signed-off-by: Yury-Fridlyand <[email protected]>
  • Loading branch information
Yury-Fridlyand authored Mar 8, 2023
1 parent a3ef2bf commit 2d29549
Show file tree
Hide file tree
Showing 26 changed files with 1,175 additions and 160 deletions.
1 change: 1 addition & 0 deletions core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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());
Expand Down Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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."));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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());
}
}
Original file line number Diff line number Diff line change
@@ -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));
}
}
Loading

0 comments on commit 2d29549

Please sign in to comment.