Support SELECT * and FROM clause in new SQL parser (#573)

* Support from

* Add more UT

* Update doc

* Update doc

* Add doctest

* Add IT

* Change doc and grammar for ANSI SQL

* Change doc and grammar

* Split grammar file

* Prepare PR

* Prepare PR

* Run IT with/without new engine

* Address PR comments

* Address PR comments

docs/category.json

@@ -16,6 +16,7 @@
   ],
   "sql_cli": [
     "user/dql/expressions.rst",
+    "user/general/identifiers.rst",
     "user/dql/functions.rst",
     "user/beyond/partiql.rst"
   ]

docs/user/dql/expressions.rst

@@ -14,7 +14,7 @@ Introduction
 
 Expressions, particularly value expressions, are those which return a scalar value. Expressions have different types and forms. For example, there are literal values as atom expression and arithmetic, predicate and function expression built on top of them. And also expressions can be used in different clauses, such as using arithmetic expression in ``SELECT``, ``WHERE`` or ``HAVING`` clause.
 
-Note that before you try out examples using the SQL features in this doc, you need to enable the new query engine by following the steps in ``opendistro.sql.engine.new.enabled`` section in `Plugin Settings <admin/settings.rst>`_.
+Note that before you try out examples using the SQL features in this doc, you need to enable the new query engine by following the steps in ``opendistro.sql.engine.new.enabled`` section in `Plugin Settings <../admin/settings.rst>`_.
 
 Literal Values
 ==============

docs/user/general/identifiers.rst

@@ -0,0 +1,108 @@
+===========
+Identifiers
+===========
+
+.. rubric:: Table of contents
+
+.. contents::
+   :local:
+   :depth: 2
+
+
+Introduction
+============
+
+Identifiers are used for naming your database objects, such as index name, field name, alias etc. Basically there are two types of identifiers: regular identifiers and delimited identifiers.
+
+
+Regular Identifiers
+===================
+
+Description
+-----------
+
+According to ANSI SQL standard, a regular identifier is a string of characters that must start with ASCII letter (lower or upper case). The subsequent character can be a combination of letter, digit, underscore (``_``). It cannot be a reversed key word. And whitespace and other special characters are not allowed. Additionally in our SQL parser, we make extension to the rule for Elasticsearch storage as shown in next sub-section.
+
+Extensions
+----------
+
+For Elasticsearch, the following identifiers are supported extensionally by our SQL parser for convenience (without the need of being delimited as shown in next section):
+
+1. Identifiers prefixed by dot ``.``: this is called hidden index in Elasticsearch, for example ``.kibana``.
+2. Identifiers prefixed by at sign ``@``: this is common for meta fields generated in Logstash ingestion.
+3. Identifiers with ``-`` in the middle: this is mostly the case for index name with date information.
+4. Identifiers with star ``*`` present: this is mostly an index pattern for wildcard match.
+
+Examples
+--------
+
+Here are examples for using index pattern directly without quotes::
+
+    od> SELECT * FROM *cc*nt*;
+    fetched rows / total rows = 4/4
+    +------------------+-------------+----------------------+-----------+----------+--------+------------+---------+-------+-----------------------+------------+
+    | account_number   | firstname   | address              | balance   | gender   | city   | employer   | state   | age   | email                 | lastname   |
+    |------------------+-------------+----------------------+-----------+----------+--------+------------+---------+-------+-----------------------+------------|
+    | 1                | Amber       | 880 Holmes Lane      | 39225     | M        | Brogan | Pyrami     | IL      | 32    | [email protected]  | Duke       |
+    | 6                | Hattie      | 671 Bristol Street   | 5686      | M        | Dante  | Netagy     | TN      | 36    | [email protected] | Bond       |
+    | 13               | Nanette     | 789 Madison Street   | 32838     | F        | Nogal  | Quility    | VA      | 28    | null                  | Bates      |
+    | 18               | Dale        | 467 Hutchinson Court | 4180      | M        | Orick  | null       | MD      | 33    | [email protected]   | Adams      |
+    +------------------+-------------+----------------------+-----------+----------+--------+------------+---------+-------+-----------------------+------------+
+
+
+Delimited Identifiers
+=====================
+
+Description
+-----------
+
+A delimited identifier is an identifier enclosed in back ticks ````` or double quotation marks ``"``. In this case, the identifier enclosed is not necessarily a regular identifier. In other words, it can contain any special character not allowed by regular identifier.
+
+Please note the difference between single quote and double quotes in SQL syntax. Single quote is used to enclose a string literal while double quotes have same purpose as back ticks to escape special characters in an identifier.
+
+Use Cases
+---------
+
+Here are typical examples of the use of delimited identifiers:
+
+1. Identifiers of reserved key word name
+2. Identifiers with dot ``.`` present: similarly as ``-`` in index name to include date information, it is required to be quoted so parser can differentiate it from identifier with qualifiers.
+3. Identifiers with other special character: Elasticsearch has its own rule which allows more special character, for example Unicode character is supported in index name.
+
+Examples
+--------
+
+Here are examples for quoting an index name by back ticks::
+
+    od> SELECT * FROM `accounts`;
+    fetched rows / total rows = 4/4
+    +------------------+-------------+----------------------+-----------+----------+--------+------------+---------+-------+-----------------------+------------+
+    | account_number   | firstname   | address              | balance   | gender   | city   | employer   | state   | age   | email                 | lastname   |
+    |------------------+-------------+----------------------+-----------+----------+--------+------------+---------+-------+-----------------------+------------|
+    | 1                | Amber       | 880 Holmes Lane      | 39225     | M        | Brogan | Pyrami     | IL      | 32    | [email protected]  | Duke       |
+    | 6                | Hattie      | 671 Bristol Street   | 5686      | M        | Dante  | Netagy     | TN      | 36    | [email protected] | Bond       |
+    | 13               | Nanette     | 789 Madison Street   | 32838     | F        | Nogal  | Quility    | VA      | 28    | null                  | Bates      |
+    | 18               | Dale        | 467 Hutchinson Court | 4180      | M        | Orick  | null       | MD      | 33    | [email protected]   | Adams      |
+    +------------------+-------------+----------------------+-----------+----------+--------+------------+---------+-------+-----------------------+------------+
+
+
+Case Sensitivity
+================
+
+Description
+-----------
+
+In SQL-92, regular identifiers are case insensitive and converted to upper case automatically just like key word. While characters in a delimited identifier appear as they are. However, in our SQL implementation, identifiers are treated in case sensitive manner. So it must be exactly same as what is stored in Elasticsearch which is different from ANSI standard.
+
+Examples
+--------
+
+For example, if you run ``SELECT * FROM ACCOUNTS``, it will end up with an index not found exception from our plugin because the actual index name is under lower case.
+
+
+Identifier Qualifiers
+=====================
+
+For now, we do not support using Elasticsearch cluster name as catalog name to qualify an index name, such as ``my-cluster.logs``.
+
+TODO: field name qualifiers

docs/user/index.rst

@@ -17,6 +17,10 @@ Open Distro for Elasticsearch SQL enables you to extract insights out of Elastic
 
   - `Plugin Settings <admin/settings.rst>`_
 
+* **Language Structure**
+
+  - `Identifiers <general/identifiers.rst>`_
+
 * **Data Query Language**
 
   - `Expressions <dql/expressions.rst>`_

integ-test/build.gradle

@@ -56,12 +56,13 @@ compileTestJava {
     }
 }
 
-tasks.integTest.dependsOn(':plugin:bundlePlugin')
+tasks.integTest.dependsOn(':plugin:bundlePlugin', ':integ-test:integTestWithNewEngine')
 testClusters.integTest {
     testDistribution = 'oss'
     plugin file(tasks.getByPath(':plugin:bundlePlugin').archiveFile)
 }
 
+// Run only legacy SQL ITs with new SQL engine disabled
 integTest.runner {
     systemProperty 'tests.security.manager', 'false'
     systemProperty('project.root', project.projectDir.absolutePath)
@@ -78,12 +79,44 @@ integTest.runner {
         jvmArgs '-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005'
     }
 
-    include 'com/amazon/opendistroforelasticsearch/sql/ppl/**/*IT.class'
-    include 'com/amazon/opendistroforelasticsearch/sql/legacy/**/*IT.class'
+    exclude 'com/amazon/opendistroforelasticsearch/sql/ppl/**/*IT.class'
+    exclude 'com/amazon/opendistroforelasticsearch/sql/sql/**/*IT.class'
     exclude 'com/amazon/opendistroforelasticsearch/sql/doctest/**/*IT.class'
     exclude 'com/amazon/opendistroforelasticsearch/sql/correctness/**'
 }
 
+// Run PPL ITs and new, legacy and comparison SQL ITs with new SQL engine enabled
+task integTestWithNewEngine(type: RestIntegTestTask) {
+    dependsOn ':plugin:bundlePlugin'
+    runner {
+        systemProperty 'tests.security.manager', 'false'
+        systemProperty('project.root', project.projectDir.absolutePath)
+
+        systemProperty "https", System.getProperty("https")
+        systemProperty "user", System.getProperty("user")
+        systemProperty "password", System.getProperty("password")
+
+        // Enable new SQL engine
+        systemProperty 'enableNewEngine', 'true'
+
+        // Tell the test JVM if the cluster JVM is running under a debugger so that tests can use longer timeouts for
+        // requests. The 'doFirst' delays reading the debug setting on the cluster till execution time.
+        doFirst { systemProperty 'cluster.debug', getDebug() }
+
+        if (System.getProperty("test.debug") != null) {
+            jvmArgs '-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005'
+        }
+
+        exclude 'com/amazon/opendistroforelasticsearch/sql/doctest/**/*IT.class'
+        exclude 'com/amazon/opendistroforelasticsearch/sql/correctness/**'
+    }
+}
+
+testClusters.integTestWithNewEngine {
+    testDistribution = 'oss'
+    plugin file(tasks.getByPath(':plugin:bundlePlugin').archiveFile)
+}
+
 
 task docTest(type: RestIntegTestTask) {
     dependsOn ':plugin:bundlePlugin'

integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/SQLIntegTestCase.java

@@ -82,6 +82,7 @@ public void setUpIndices() throws Exception {
     }
 
     increaseScriptMaxCompilationsRate();
+    enableNewQueryEngine();
     init();
   }
 
@@ -149,6 +150,13 @@ private void increaseScriptMaxCompilationsRate() throws IOException {
         new ClusterSetting("transient", "script.max_compilations_rate", "10000/1m"));
   }
 
+  private void enableNewQueryEngine() throws IOException {
+    boolean isEnabled = Boolean.parseBoolean(System.getProperty("enableNewEngine", "false"));
+    if (isEnabled) {
+      com.amazon.opendistroforelasticsearch.sql.util.TestUtils.enableNewQueryEngine(client());
+    }
+  }
+
   protected static void wipeAllClusterSettings() throws IOException {
     updateClusterSettings(new ClusterSetting("persistent", "*", null));
     updateClusterSettings(new ClusterSetting("transient", "*", null));

integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java → integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/CorrectnessTestBase.java

@@ -39,7 +39,7 @@
  * enforce the success of all tests rather than report failures only.
  */
 @ThreadLeakScope(ThreadLeakScope.Scope.NONE)
-public abstract class SQLIntegTestCase extends RestIntegTestCase {
+public abstract class CorrectnessTestBase extends RestIntegTestCase {
 
   /**
    * Comparison test runner shared by all methods in this IT class.

integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/IdentifierIT.java

@@ -0,0 +1,98 @@
+/*
+ *    Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License").
+ *    You may not use this file except in compliance with the License.
+ *    A copy of the License is located at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    or in the "license" file accompanying this file. This file is distributed
+ *    on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ *    express or implied. See the License for the specific language governing
+ *    permissions and limitations under the License.
+ *
+ */
+
+package com.amazon.opendistroforelasticsearch.sql.sql;
+
+import static com.amazon.opendistroforelasticsearch.sql.util.TestUtils.createHiddenIndexByRestClient;
+import static com.amazon.opendistroforelasticsearch.sql.util.TestUtils.performRequest;
+
+import com.amazon.opendistroforelasticsearch.sql.legacy.SQLIntegTestCase;
+import java.io.IOException;
+import org.elasticsearch.client.Request;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration tests for identifiers including index and field name symbol.
+ */
+public class IdentifierIT extends SQLIntegTestCase {
+
+  @Test
+  public void testIndexNames() throws IOException {
+    createIndexWithOneDoc("logs", "logs_2020_01");
+    queryAndAssertTheDoc("SELECT * FROM logs");
+    queryAndAssertTheDoc("SELECT * FROM logs_2020_01");
+  }
+
+  @Test
+  public void testSpecialIndexNames() throws IOException {
+    createIndexWithOneDoc(".system", "logs-2020-01");
+    queryAndAssertTheDoc("SELECT * FROM .system");
+    queryAndAssertTheDoc("SELECT * FROM logs-2020-01");
+  }
+
+  @Test
+  public void testQuotedIndexNames() throws IOException {
+    createIndexWithOneDoc("logs+2020+01", "logs.2020.01");
+    queryAndAssertTheDoc("SELECT * FROM `logs+2020+01`");
+    queryAndAssertTheDoc("SELECT * FROM \"logs.2020.01\"");
+  }
+
+  private void createIndexWithOneDoc(String... indexNames) throws IOException {
+    for (String indexName : indexNames) {
+      new Index(indexName).addDoc("{\"age\": 30}");
+    }
+  }
+
+  private void queryAndAssertTheDoc(String sql) {
+    assertEquals(
+        "{\n"
+            + "  \"schema\": [{\n"
+            + "    \"name\": \"age\",\n"
+            + "    \"type\": \"integer\"\n"
+            + "  }],\n"
+            + "  \"total\": 1,\n"
+            + "  \"datarows\": [[30]],\n"
+            + "  \"size\": 1\n"
+            + "}\n",
+        executeQuery(sql.replace("\"", "\\\""), "jdbc")
+    );
+  }
+
+  /**
+   * Index abstraction for test code readability.
+   */
+  private static class Index {
+
+    private final String indexName;
+
+    Index(String indexName) throws IOException {
+      this.indexName = indexName;
+
+      if (indexName.startsWith(".")) {
+        createHiddenIndexByRestClient(client(), indexName, "");
+      } else {
+        executeRequest(new Request("PUT", "/" + indexName));
+      }
+    }
+
+    void addDoc(String doc) {
+      Request indexDoc = new Request("POST", String.format("/%s/_doc?refresh=true", indexName));
+      indexDoc.setJsonEntity(doc);
+      performRequest(client(), indexDoc);
+    }
+  }
+
+}

integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java

@@ -28,7 +28,7 @@
 /**
  * SQL integration test automated by comparison test framework.
  */
-public class SQLCorrectnessIT extends SQLIntegTestCase {
+public class SQLCorrectnessIT extends CorrectnessTestBase {
 
   private static final String ROOT_DIR = "correctness/";
   private static final String[] EXPR_TEST_DIR = { "expressions" };

integ-test/src/test/resources/correctness/queries/select.txt

@@ -1,2 +1,4 @@
+SELECT 1 + 2 FROM kibana_sample_data_flights
+SELECT abs(-10) FROM kibana_sample_data_flights
 SELECT DistanceMiles FROM kibana_sample_data_flights
 SELECT AvgTicketPrice, Carrier FROM kibana_sample_data_flights WHERE AvgTicketPrice <= 500

legacy/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryActionTest.java

@@ -67,8 +67,8 @@ public void skipExplainThatNotSupport() {
   @Test
   public void skipQueryThatNotSupport() {
     SQLQueryRequest request = new SQLQueryRequest(
-        new JSONObject("{\"query\": \"SELECT * FROM test\"}"),
-        "SELECT * FROM test",
+        new JSONObject("{\"query\": \"SELECT * FROM test WHERE age = 10\"}"),
+        "SELECT * FROM test WHERE age = 10",
         QUERY_API_ENDPOINT,
         "");
 

sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4

@@ -0,0 +1,45 @@
+/*
+MySQL (Positive Technologies) grammar
+The MIT License (MIT).
+Copyright (c) 2015-2017, Ivan Kochurkin ([email protected]), Positive Technologies.
+Copyright (c) 2017, Ivan Khudyashev ([email protected])
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in
+all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+THE SOFTWARE.
+*/
+
+parser grammar OpenDistroSQLIdentifierParser;
+
+options { tokenVocab=OpenDistroSQLLexer; }
+
+
+//    Identifiers
+
+tableName
+    : qualifiedName
+    ;
+
+qualifiedName
+    : ident (DOT ident)*
+    ;
+
+ident
+    : DOT? ID
+    | DOUBLE_QUOTE_ID
+    | BACKTICK_QUOTE_ID
+    ;