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.

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

Disallow unquoted literals in LIKE clause in DESCRIBE statement #1181

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion docs/category.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"user/dql/window.rst",
"user/beyond/partiql.rst",
"user/dql/aggregations.rst",
"user/dql/complex.rst"
"user/dql/complex.rst",
"user/dql/metadata.rst"
]
}
120 changes: 55 additions & 65 deletions docs/user/dql/metadata.rst

Large diffs are not rendered by default.

Binary file removed docs/user/img/rdd/showFilter.png
Binary file not shown.
Binary file removed docs/user/img/rdd/showStatement.png
Binary file not shown.
44 changes: 36 additions & 8 deletions integ-test/src/test/java/org/opensearch/sql/sql/AdminIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,23 @@
package org.opensearch.sql.sql;

import static org.hamcrest.Matchers.equalTo;
import static org.opensearch.sql.legacy.plugin.RestSqlAction.QUERY_API_ENDPOINT;
import static org.opensearch.sql.util.MatcherUtils.assertJsonEquals;
import static org.opensearch.sql.util.TestUtils.getResponseBody;

import com.google.common.io.Resources;
import java.io.IOException;
import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Locale;

import org.json.JSONArray;
import org.json.JSONObject;
import org.junit.Test;
import org.opensearch.client.Request;
import org.opensearch.client.RequestOptions;
import org.opensearch.client.Response;
import org.opensearch.sql.common.utils.StringUtils;
import org.opensearch.sql.legacy.SQLIntegTestCase;
import org.opensearch.sql.legacy.TestsConstants;
Expand All @@ -34,7 +40,7 @@ public void init() throws Exception {
public void showSingleIndexAlias() throws IOException {
String alias = "acc";
addAlias(TestsConstants.TEST_INDEX_ACCOUNT, alias);
JSONObject response = new JSONObject(executeQuery("SHOW TABLES LIKE acc", "jdbc"));
JSONObject response = new JSONObject(executeQuery("SHOW TABLES LIKE 'acc'", "jdbc"));

/*
* Assumed indices of fields in dataRows based on "schema" output for SHOW given above:
Expand All @@ -48,7 +54,7 @@ public void showSingleIndexAlias() throws IOException {
public void describeSingleIndexAlias() throws IOException {
String alias = "acc";
addAlias(TestsConstants.TEST_INDEX_ACCOUNT, alias);
JSONObject response = new JSONObject(executeQuery("DESCRIBE TABLES LIKE acc", "jdbc"));
JSONObject response = new JSONObject(executeQuery("DESCRIBE TABLES LIKE 'acc'", "jdbc"));

/*
* Assumed indices of fields in dataRows based on "schema" output for DESCRIBE given above:
Expand All @@ -58,15 +64,25 @@ public void describeSingleIndexAlias() throws IOException {
assertThat(row.get(2), equalTo(alias));
}

@Test
public void describeSingleIndexWildcard() throws IOException {
JSONObject response1 = executeQuery("DESCRIBE TABLES LIKE \\\"%account\\\"");
JSONObject response2 = executeQuery("DESCRIBE TABLES LIKE '%account'");
JSONObject response3 = executeQuery("DESCRIBE TABLES LIKE '%account' COLUMNS LIKE \\\"%name\\\"");
JSONObject response4 = executeQuery("DESCRIBE TABLES LIKE \\\"%account\\\" COLUMNS LIKE '%name'");
// 11 rows in the output, each corresponds to a column in the table
assertEquals(11, response1.getJSONArray("datarows").length());
assertTrue(response1.similar(response2));
// 2 columns should match the wildcard
assertEquals(2, response3.getJSONArray("datarows").length());
assertTrue(response3.similar(response4));
}

@Test
public void explainShow() throws Exception {
String expected = loadFromFile("expectedOutput/sql/explain_show.json");

final String actual = explainQuery("SHOW TABLES LIKE %");
assertJsonEquals(
expected,
explainQuery("SHOW TABLES LIKE %")
);
String actual = explainQuery("SHOW TABLES LIKE '%'");
assertTrue(new JSONObject(expected).similar(new JSONObject(actual)));
}

private void addAlias(String index, String alias) throws IOException {
Expand All @@ -77,4 +93,16 @@ private String loadFromFile(String filename) throws Exception {
URI uri = Resources.getResource(filename).toURI();
return new String(Files.readAllBytes(Paths.get(uri)));
}

protected JSONObject executeQuery(String query) throws IOException {
Request request = new Request("POST", QUERY_API_ENDPOINT);
request.setJsonEntity(String.format(Locale.ROOT, "{\n" + " \"query\": \"%s\"\n" + "}", query));

RequestOptions.Builder restOptionsBuilder = RequestOptions.DEFAULT.toBuilder();
restOptionsBuilder.addHeader("Content-Type", "application/json");
request.setOptions(restOptionsBuilder);

Response response = client().performRequest(request);
return new JSONObject(getResponseBody(response));
}
}
7 changes: 1 addition & 6 deletions sql/src/main/antlr/OpenSearchSQLParser.g4
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,8 @@ tableFilter
;

showDescribePattern
: oldID=compatibleID | stringLiteral
;

compatibleID
: (MODULE | ID)+?
: stringLiteral
;

// Select Statement's Details

querySpecification
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,7 @@ public UnresolvedExpression visitColumnFilter(ColumnFilterContext ctx) {
@Override
public UnresolvedExpression visitShowDescribePattern(
ShowDescribePatternContext ctx) {
if (ctx.compatibleID() != null) {
return stringLiteral(ctx.compatibleID().getText());
} else {
return visit(ctx.stringLiteral());
}
return visit(ctx.stringLiteral());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

package org.opensearch.sql.sql.antlr;

import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;

Expand Down Expand Up @@ -474,6 +475,38 @@ public void can_parse_wildcard_query_relevance_function() {
+ "boost=1.5, case_insensitive=true, rewrite=\"scoring_boolean\")"));
}

@Test
public void describe_request_accepts_only_quoted_string_literals() {
assertAll(
() -> assertThrows(SyntaxCheckException.class,
() -> parser.parse("DESCRIBE TABLES LIKE bank")),
() -> assertThrows(SyntaxCheckException.class,
() -> parser.parse("DESCRIBE TABLES LIKE %bank%")),
() -> assertThrows(SyntaxCheckException.class,
() -> parser.parse("DESCRIBE TABLES LIKE `bank`")),
() -> assertThrows(SyntaxCheckException.class,
() -> parser.parse("DESCRIBE TABLES LIKE %bank% COLUMNS LIKE %status%")),
() -> assertThrows(SyntaxCheckException.class,
() -> parser.parse("DESCRIBE TABLES LIKE 'bank' COLUMNS LIKE status")),
() -> assertNotNull(parser.parse("DESCRIBE TABLES LIKE 'bank' COLUMNS LIKE \"status\"")),
() -> assertNotNull(parser.parse("DESCRIBE TABLES LIKE \"bank\" COLUMNS LIKE 'status'"))
);
}

@Test
public void show_request_accepts_only_quoted_string_literals() {
assertAll(
() -> assertThrows(SyntaxCheckException.class,
() -> parser.parse("SHOW TABLES LIKE bank")),
() -> assertThrows(SyntaxCheckException.class,
() -> parser.parse("SHOW TABLES LIKE %bank%")),
() -> assertThrows(SyntaxCheckException.class,
() -> parser.parse("SHOW TABLES LIKE `bank`")),
() -> assertNotNull(parser.parse("SHOW TABLES LIKE 'bank'")),
() -> assertNotNull(parser.parse("SHOW TABLES LIKE \"bank\""))
);
}

@ParameterizedTest
@MethodSource({
"matchPhraseComplexQueries",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -567,10 +567,6 @@ public void can_build_show_selected_tables() {
);
}

/**
* Todo, ideally the identifier (%) couldn't be used in LIKE operator, only the string literal
* is allowed.
*/
@Test
public void show_compatible_with_old_engine_syntax() {
assertEquals(
Expand All @@ -581,18 +577,7 @@ public void show_compatible_with_old_engine_syntax() {
),
AllFields.of()
),
buildAST("SHOW TABLES LIKE %")
);
}

@Test
public void describe_compatible_with_old_engine_syntax() {
assertEquals(
project(
relation(mappingTable("a_c%")),
AllFields.of()
),
buildAST("DESCRIBE TABLES LIKE a_c%")
buildAST("SHOW TABLES LIKE '%'")
);
}

Expand Down Expand Up @@ -621,24 +606,6 @@ public void can_build_describe_selected_tables_field_filter() {
);
}

/**
* Todo, ideally the identifier (%) couldn't be used in LIKE operator, only the string literal
* is allowed.
*/
@Test
public void describe_and_column_compatible_with_old_engine_syntax() {
assertEquals(
project(
filter(
relation(mappingTable("a_c%")),
function("like", qualifiedName("COLUMN_NAME"), stringLiteral("name%"))
),
AllFields.of()
),
buildAST("DESCRIBE TABLES LIKE a_c% COLUMNS LIKE name%")
);
}

@Test
public void can_build_alias_by_keywords() {
assertEquals(
Expand Down