Disallow unquoted literals in LIKE clause in DESCRIBE statement (#…

…1181) * Allow quoted literals only in `DESCRIBE` and `SHOW` clauses. Tests. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> * Fix doctest after rebase. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Fix doctest after rebase. - Typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Update syntax section. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
opensearch-project · Jan 4, 2023 · 1b58f7d · 1b58f7d
1 parent 2d34d9e
commit 1b58f7d
Show file tree

Hide file tree

Showing 9 changed files with 129 additions and 119 deletions.
diff --git a/docs/category.json b/docs/category.json
@@ -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"
   ]
 }
diff --git a/docs/user/dql/metadata.rst b/docs/user/dql/metadata.rst
diff --git a/docs/user/img/rdd/showFilter.png b/docs/user/img/rdd/showFilter.png
diff --git a/docs/user/img/rdd/showStatement.png b/docs/user/img/rdd/showStatement.png
diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/AdminIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/AdminIT.java
@@ -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;
@@ -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:
@@ -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:
@@ -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 {
@@ -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));
+  }
 }
diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4
@@ -80,13 +80,8 @@ tableFilter
     ;
 
 showDescribePattern
-    : oldID=compatibleID | stringLiteral
-    ;
-
-compatibleID
-    : (MODULE | ID)+?
+    : stringLiteral
     ;
-
 //    Select Statement's Details
 
 querySpecification

diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java
@@ -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

diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java
@@ -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;
 
@@ -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",

diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java
@@ -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(
@@ -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 '%'")
     );
   }
 
@@ -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(