Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pagination, phase 1: Add unit tests for SQL module with coverage. #239

Merged
merged 2 commits into from
Mar 10, 2023
+210 −85
Merged

Pagination, phase 1: Add unit tests for SQL module with coverage. #239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2019e84
Add unit tests for SQL module with coverage.
Yury-Fridlyand Mar 7, 2023
35aa3b3
Update sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryReques…
Yury-Fridlyand Mar 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions sql/src/main/java/org/opensearch/sql/sql/SQLService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.opensearch.sql.common.response.ResponseListener;
import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse;
import org.opensearch.sql.executor.ExecutionEngine.QueryResponse;
import org.opensearch.sql.executor.QueryId;
import org.opensearch.sql.executor.QueryManager;
import org.opensearch.sql.executor.execution.AbstractPlan;
import org.opensearch.sql.executor.execution.QueryPlanFactory;
Expand Down Expand Up @@ -68,9 +69,19 @@ private AbstractPlan plan(
if (request.getCursor().isPresent()) {
// Handle v2 cursor here -- legacy cursor was handled earlier.
if (queryListener.isEmpty() && explainListener.isPresent()) { // explain request
// TODO explain should be processed inside the plan
explainListener.get().onFailure(new UnsupportedOperationException(
"`explain` request for cursor requests is not supported. "
+ "Use `explain` for the initial query request."));
return new AbstractPlan(QueryId.queryId()) {
@Override
public void execute() {
}

@Override
public void explain(ResponseListener<ExplainResponse> listener) {
}
};
}
// non-explain request
return queryExecutionFactory.create(request.getCursor().get(), queryListener.get());
Expand Down
40 changes: 18 additions & 22 deletions sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,12 @@

package org.opensearch.sql.sql.domain;

import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
import java.util.Collections;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
Expand Down Expand Up @@ -64,7 +63,7 @@ public class SQLQueryRequest {
@Accessors(fluent = true)
private boolean sanitize = true;

private String cursor = "";
private String cursor;

/**
* Constructor of SQLQueryRequest that passes request params.
Expand All @@ -77,29 +76,35 @@ public SQLQueryRequest(JSONObject jsonContent, String query, String path,
this.params = params;
this.format = getFormat(params);
this.sanitize = shouldSanitize(params);
// TODO hack
this.cursor = cursor == null ? "" : cursor;
this.cursor = cursor;
}

/**
* Pre-check if the request can be supported by meeting ALL the following criteria:
* 1.Only supported fields present in request body, ex. "filter" and "cursor" are not supported
* 2.Response format is default or can be supported.
*
* @return true if supported.
* @return true if supported.
*/
public boolean isSupported() {
return (isCursor() || isOnlySupportedFieldInPayload())
&& isSupportedFormat();
var noCursor = !isCursor();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be booleans instead of var?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var could be anything, even boolean as here. My favorite lazy developer feature.

var noQuery = query == null;
var noParams = params.isEmpty();
var noContent = jsonContent == null || jsonContent.isEmpty();

return ((!noCursor && noQuery && noParams && noContent) // if cursor is given, but other things
|| (noCursor && !noQuery)) // or if cursor is not given, but query
&& isOnlySupportedFieldInPayload() // and request has supported fields only
&& isSupportedFormat(); // and request is in supported format
}

private boolean isCursor() {
return cursor != null && cursor.isEmpty() == false;
return cursor != null && !cursor.isEmpty();
}

/**
* Check if request is to explain rather than execute the query.
* @return true if it is a explain request
* @return true if it is an explain request
*/
public boolean isExplainRequest() {
return path.endsWith("/_explain");
Expand All @@ -122,29 +127,20 @@ private boolean isOnlySupportedFieldInPayload() {
return jsonContent == null || SUPPORTED_FIELDS.containsAll(jsonContent.keySet());
}


public Optional<String> getCursor() {
return cursor != "" ? Optional.of(cursor) : Optional.empty();
}

public boolean mayReturnCursor() {
return cursor != "" || getFetchSize() > 0;
return Optional.ofNullable(cursor);
}

public int getFetchSize() {
return jsonContent.optInt("fetch_size");
}

private boolean isSupportedFormat() {
return Strings.isNullOrEmpty(format) || "jdbc".equalsIgnoreCase(format)
|| "csv".equalsIgnoreCase(format) || "raw".equalsIgnoreCase(format);
return Stream.of("csv", "jdbc", "raw").anyMatch(format::equalsIgnoreCase);
}

private String getFormat(Map<String, String> params) {
if (params.containsKey(QUERY_PARAMS_FORMAT)) {
return params.get(QUERY_PARAMS_FORMAT);
}
return "jdbc";
return params.getOrDefault(QUERY_PARAMS_FORMAT, "jdbc");
}

private boolean shouldSanitize(Map<String, String> params) {
Expand Down
66 changes: 44 additions & 22 deletions sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,19 @@
package org.opensearch.sql.sql;

import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doAnswer;
import static org.opensearch.sql.executor.ExecutionEngine.QueryResponse;

import java.util.Collections;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import org.json.JSONObject;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayNameGeneration;
import org.junit.jupiter.api.DisplayNameGenerator;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
Expand All @@ -30,11 +33,11 @@
import org.opensearch.sql.executor.QueryService;
import org.opensearch.sql.executor.execution.PaginatedQueryService;
import org.opensearch.sql.executor.execution.QueryPlanFactory;
import org.opensearch.sql.opensearch.executor.Cursor;
import org.opensearch.sql.sql.antlr.SQLSyntaxParser;
import org.opensearch.sql.sql.domain.SQLQueryRequest;

@ExtendWith(MockitoExtension.class)
@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class SQLServiceTest {

private static String QUERY = "/_plugins/_sql";
Expand All @@ -51,9 +54,6 @@ class SQLServiceTest {
@Mock
private PaginatedQueryService paginatedQueryService;

@Mock
private ExecutionEngine.Schema schema;

@Mock
private PaginatedPlanCache paginatedPlanCache;

Expand All @@ -70,13 +70,7 @@ public void cleanup() throws InterruptedException {
}

@Test
public void canExecuteSqlQuery() {
doAnswer(invocation -> {
ResponseListener<QueryResponse> listener = invocation.getArgument(1);
listener.onResponse(new QueryResponse(schema, Collections.emptyList(), Cursor.None));
return null;
}).when(queryService).execute(any(), any());

public void can_execute_sql_query() {
sqlService.execute(
new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "jdbc"),
new ResponseListener<>() {
Expand All @@ -93,13 +87,24 @@ public void onFailure(Exception e) {
}

@Test
public void canExecuteCsvFormatRequest() {
doAnswer(invocation -> {
ResponseListener<QueryResponse> listener = invocation.getArgument(1);
listener.onResponse(new QueryResponse(schema, Collections.emptyList(), Cursor.None));
return null;
}).when(queryService).execute(any(), any());
public void can_execute_cursor_query() {
sqlService.execute(
new SQLQueryRequest(new JSONObject(), null, QUERY, Map.of("format", "jdbc"), "n:cursor"),
new ResponseListener<>() {
@Override
public void onResponse(QueryResponse response) {
assertNotNull(response);
}

@Override
public void onFailure(Exception e) {
fail(e);
}
});
}

@Test
public void can_execute_csv_format_request() {
sqlService.execute(
new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "csv"),
new ResponseListener<QueryResponse>() {
Expand All @@ -116,7 +121,7 @@ public void onFailure(Exception e) {
}

@Test
public void canExplainSqlQuery() {
public void can_explain_sql_query() {
doAnswer(invocation -> {
ResponseListener<ExplainResponse> listener = invocation.getArgument(1);
listener.onResponse(new ExplainResponse(new ExplainResponseNode("Test")));
Expand All @@ -138,7 +143,25 @@ public void onFailure(Exception e) {
}

@Test
public void canCaptureErrorDuringExecution() {
public void cannot_explain_cursor_query() {
sqlService.explain(new SQLQueryRequest(new JSONObject(), null, EXPLAIN,
Map.of("format", "jdbc"), "n:cursor"),
new ResponseListener<ExplainResponse>() {
@Override
public void onResponse(ExplainResponse response) {
fail(response.toString());
}

@Override
public void onFailure(Exception e) {
assertTrue(e.getMessage()
.contains("`explain` request for cursor requests is not supported."));
}
});
}

@Test
public void can_capture_error_during_execution() {
sqlService.execute(
new SQLQueryRequest(new JSONObject(), "SELECT", QUERY, ""),
new ResponseListener<QueryResponse>() {
Expand All @@ -155,7 +178,7 @@ public void onFailure(Exception e) {
}

@Test
public void canCaptureErrorDuringExplain() {
public void can_capture_error_during_explain() {
sqlService.explain(
new SQLQueryRequest(new JSONObject(), "SELECT", EXPLAIN, ""),
new ResponseListener<ExplainResponse>() {
Expand All @@ -170,5 +193,4 @@ public void onFailure(Exception e) {
}
});
}

}
Loading