From f6113ac2e54ad59699652901ff309e9771a2b8d2 Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 22 Jul 2020 13:58:53 -0700 Subject: [PATCH 1/3] Change grammar and add UT --- .../sql/legacy/QueryAnalysisIT.java | 11 +++++++++++ legacy/src/main/antlr/OpenDistroSqlParser.g4 | 2 +- .../sql/legacy/query/ESActionFactory.java | 5 ++++- .../sql/legacy/antlr/SyntaxAnalysisTest.java | 7 ++++++- sql/src/main/antlr/OpenDistroSQLParser.g4 | 2 +- .../sql/sql/antlr/SQLSyntaxParserTest.java | 5 +++++ 6 files changed, 28 insertions(+), 4 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryAnalysisIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryAnalysisIT.java index 4a5e6d4ccc..aac1329833 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryAnalysisIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryAnalysisIT.java @@ -48,6 +48,17 @@ protected void init() throws Exception { loadIndex(Index.BANK); } + @Test + public void queryEndWithSemiColonShouldPassNoMatterIfAnalyzerEnabledOrNot() { + String queryEndWithSemiColon = "SELECT * FROM elasticsearch-sql_test_index_bank;"; + queryShouldPassAnalysis(queryEndWithSemiColon); + + runWithClusterSetting( + new ClusterSetting("transient", QUERY_ANALYSIS_ENABLED, "false"), + () -> queryShouldPassAnalysis(queryEndWithSemiColon) + ); + } + @Test public void missingFromClauseShouldThrowSyntaxException() { queryShouldThrowSyntaxException("SELECT 1"); diff --git a/legacy/src/main/antlr/OpenDistroSqlParser.g4 b/legacy/src/main/antlr/OpenDistroSqlParser.g4 index ce30892e16..86c5c89d20 100644 --- a/legacy/src/main/antlr/OpenDistroSqlParser.g4 +++ b/legacy/src/main/antlr/OpenDistroSqlParser.g4 @@ -32,7 +32,7 @@ options { tokenVocab=OpenDistroSqlLexer; } // Root rule root - : sqlStatement? EOF + : sqlStatement? SEMI? EOF ; // Only SELECT, DELETE, SHOW and DSCRIBE are supported for now diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/query/ESActionFactory.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/query/ESActionFactory.java index ac05da3236..6da45cb8f8 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/query/ESActionFactory.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/query/ESActionFactory.java @@ -87,8 +87,11 @@ public static QueryAction create(Client client, String sql) public static QueryAction create(Client client, QueryActionRequest request) throws SqlParseException, SQLFeatureNotSupportedException { String sql = request.getSql(); - // Linebreak matcher + // Remove linebreak matcher and semicolon at the end sql = sql.replaceAll("\\R", " ").trim(); + if (sql.endsWith(";")) { + sql = sql.substring(0, sql.length() - 1); + } switch (getFirstWord(sql)) { case "SELECT": diff --git a/legacy/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/antlr/SyntaxAnalysisTest.java b/legacy/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/antlr/SyntaxAnalysisTest.java index af7fceecd9..b15c5090cf 100644 --- a/legacy/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/antlr/SyntaxAnalysisTest.java +++ b/legacy/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/antlr/SyntaxAnalysisTest.java @@ -79,7 +79,7 @@ public void missingWhereKeywordShouldThrowException() { expectValidationFailWithErrorMessage( "SELECT * FROM accounts age = 1", "offending symbol [=]", // parser thought 'age' is alias of 'accounts' and failed at '=' - "Expecting", "'WHERE'" // "Expecting tokens in {, 'INNER', 'JOIN', ... 'WHERE', ','}" + "Expecting", ";" // "Expecting tokens in {, ';'}" ); } @@ -130,6 +130,11 @@ public void arithmeticExpressionInWhereClauseShouldPass() { validate("SELECT * FROM accounts WHERE age + 1 = 10"); } + @Test + public void queryEndWithSemiColonShouldPass() { + validate("SELECT * FROM accounts;"); + } + private void expectValidationFailWithErrorMessage(String query, String... messages) { exception.expect(SyntaxAnalysisException.class); exception.expectMessage(allOf(Arrays.stream(messages). diff --git a/sql/src/main/antlr/OpenDistroSQLParser.g4 b/sql/src/main/antlr/OpenDistroSQLParser.g4 index c9b3babc4e..49422c061c 100644 --- a/sql/src/main/antlr/OpenDistroSQLParser.g4 +++ b/sql/src/main/antlr/OpenDistroSQLParser.g4 @@ -34,7 +34,7 @@ options { tokenVocab=OpenDistroSQLLexer; } // Root rule root - : sqlStatement? EOF + : sqlStatement? SEMI? EOF ; // Only SELECT diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/antlr/SQLSyntaxParserTest.java index 6d3dee9585..94d668c4ba 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/antlr/SQLSyntaxParserTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/antlr/SQLSyntaxParserTest.java @@ -26,6 +26,11 @@ class SQLSyntaxParserTest { private final SQLSyntaxParser parser = new SQLSyntaxParser(); + @Test + public void canParseQueryEndWithSemiColon() { + assertNotNull(parser.parse("SELECT 123;")); + } + @Test public void canParseSelectLiterals() { assertNotNull(parser.parse("SELECT 123, 'hello'")); From fa155ea0c1f11f1ab34657ce99791a33bc559115 Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 22 Jul 2020 14:38:16 -0700 Subject: [PATCH 2/3] Add doc --- docs/user/dql/basics.rst | 2 ++ integ-test/build.gradle | 1 + integ-test/src/test/resources/doctest/templates/dql/basics.rst | 2 ++ 3 files changed, 5 insertions(+) diff --git a/docs/user/dql/basics.rst b/docs/user/dql/basics.rst index 3b7d0c4746..082abe9aee 100644 --- a/docs/user/dql/basics.rst +++ b/docs/user/dql/basics.rst @@ -28,6 +28,8 @@ The syntax of ``SELECT`` statement is as follows:: [ORDER BY expression [IS [NOT] NULL] [ASC | DESC] [, ...]] [LIMIT [offset, ] size] +Although multiple query statements to execute in batch is not supported, ending with semicolon ``;`` is still allowed. For example, you can run ``SELECT * FROM accounts;`` without issue. This is useful to support queries generated by other tool, such as Microsoft Excel or BI tool. + Fundamentals ------------ diff --git a/integ-test/build.gradle b/integ-test/build.gradle index 73422f4435..4999d5520e 100644 --- a/integ-test/build.gradle +++ b/integ-test/build.gradle @@ -135,6 +135,7 @@ task docTest(type: RestIntegTestTask) { include 'com/amazon/opendistroforelasticsearch/sql/doctest/**/*IT.class' exclude 'com/amazon/opendistroforelasticsearch/sql/correctness/**/*IT.class' exclude 'com/amazon/opendistroforelasticsearch/sql/ppl/**/*IT.class' + exclude 'com/amazon/opendistroforelasticsearch/sql/sql/**/*IT.class' exclude 'com/amazon/opendistroforelasticsearch/sql/legacy/**/*IT.class' } } diff --git a/integ-test/src/test/resources/doctest/templates/dql/basics.rst b/integ-test/src/test/resources/doctest/templates/dql/basics.rst index 34cd5d3d09..4c8877d20a 100644 --- a/integ-test/src/test/resources/doctest/templates/dql/basics.rst +++ b/integ-test/src/test/resources/doctest/templates/dql/basics.rst @@ -28,6 +28,8 @@ The syntax of ``SELECT`` statement is as follows:: [ORDER BY expression [IS [NOT] NULL] [ASC | DESC] [, ...]] [LIMIT [offset, ] size] +Although multiple query statements to execute in batch is not supported, ending with semicolon ``;`` is still allowed. For example, you can run ``SELECT * FROM accounts;`` without issue. This is useful to support queries generated by other tool, such as Microsoft Excel or BI tool. + Fundamentals ------------ From d2ef6ca21f070c98dffeb996424d73893c6aece5 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 09:09:59 -0700 Subject: [PATCH 3/3] Move IT --- .../sql/legacy/QueryAnalysisIT.java | 11 ----------- .../sql/legacy/QueryIT.java | 5 +++++ .../sql/legacy/query/ESActionFactory.java | 2 +- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryAnalysisIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryAnalysisIT.java index aac1329833..4a5e6d4ccc 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryAnalysisIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryAnalysisIT.java @@ -48,17 +48,6 @@ protected void init() throws Exception { loadIndex(Index.BANK); } - @Test - public void queryEndWithSemiColonShouldPassNoMatterIfAnalyzerEnabledOrNot() { - String queryEndWithSemiColon = "SELECT * FROM elasticsearch-sql_test_index_bank;"; - queryShouldPassAnalysis(queryEndWithSemiColon); - - runWithClusterSetting( - new ClusterSetting("transient", QUERY_ANALYSIS_ENABLED, "false"), - () -> queryShouldPassAnalysis(queryEndWithSemiColon) - ); - } - @Test public void missingFromClauseShouldThrowSyntaxException() { queryShouldThrowSyntaxException("SELECT 1"); diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java index f4a4c36a16..bdf5e09435 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java @@ -90,6 +90,11 @@ protected void init() throws Exception { loadIndex(Index.BANK_WITH_NULL_VALUES); } + @Test + public void queryEndWithSemiColonTest() { + executeQuery(StringUtils.format("SELECT * FROM %s;", TEST_INDEX_BANK), "jdbc"); + } + @Test public void searchTypeTest() throws IOException { JSONObject response = executeQuery(String.format(Locale.ROOT, "SELECT * FROM %s LIMIT 1000", diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/query/ESActionFactory.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/query/ESActionFactory.java index 6da45cb8f8..eeb3efe49a 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/query/ESActionFactory.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/query/ESActionFactory.java @@ -87,7 +87,7 @@ public static QueryAction create(Client client, String sql) public static QueryAction create(Client client, QueryActionRequest request) throws SqlParseException, SQLFeatureNotSupportedException { String sql = request.getSql(); - // Remove linebreak matcher and semicolon at the end + // Remove line breaker anywhere and semicolon at the end sql = sql.replaceAll("\\R", " ").trim(); if (sql.endsWith(";")) { sql = sql.substring(0, sql.length() - 1);