KYLIN-2803 refine logic and error message
Project: http://git-wip-us.apache.org/repos/asf/kylin/repo Commit: http://git-wip-us.apache.org/repos/asf/kylin/commit/b1559c66 Tree: http://git-wip-us.apache.org/repos/asf/kylin/tree/b1559c66 Diff: http://git-wip-us.apache.org/repos/asf/kylin/diff/b1559c66 Branch: refs/heads/master Commit: b1559c669c3594b33f9e4aeff415d780a0a170ca Parents: 565fe9a Author: Hongbin Ma <mahong...@apache.org> Authored: Wed Sep 6 13:09:10 2017 +0800 Committer: liyang-gmt8 <liy...@apache.org> Committed: Wed Sep 6 13:26:23 2017 +0800 ---------------------------------------------------------------------- .../org/apache/kylin/query/KylinTestBase.java | 2 +- .../apache/kylin/query/util/PushDownUtil.java | 27 ++++++++++--- .../org/apache/kylin/query/util/QueryUtil.java | 17 ++++---- .../apache/kylin/query/util/QueryUtilTest.java | 33 +++++++++++++++- .../apache/kylin/rest/service/QueryService.java | 41 +++++++++++++------- 5 files changed, 90 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kylin/blob/b1559c66/kylin-it/src/test/java/org/apache/kylin/query/KylinTestBase.java ---------------------------------------------------------------------- diff --git a/kylin-it/src/test/java/org/apache/kylin/query/KylinTestBase.java b/kylin-it/src/test/java/org/apache/kylin/query/KylinTestBase.java index d3db995..0f11996 100644 --- a/kylin-it/src/test/java/org/apache/kylin/query/KylinTestBase.java +++ b/kylin-it/src/test/java/org/apache/kylin/query/KylinTestBase.java @@ -261,7 +261,7 @@ public class KylinTestBase { return output(resultSet, needDisplay); } catch (SQLException sqlException) { Pair<List<List<String>>, List<SelectedColumnMeta>> result = PushDownUtil - .tryPushDownQuery(ProjectInstance.DEFAULT_PROJECT_NAME, sql, "DEFAULT", sqlException); + .tryPushDownSelectQuery(ProjectInstance.DEFAULT_PROJECT_NAME, sql, "DEFAULT", sqlException); if (result == null) { throw sqlException; } http://git-wip-us.apache.org/repos/asf/kylin/blob/b1559c66/query/src/main/java/org/apache/kylin/query/util/PushDownUtil.java ---------------------------------------------------------------------- diff --git a/query/src/main/java/org/apache/kylin/query/util/PushDownUtil.java b/query/src/main/java/org/apache/kylin/query/util/PushDownUtil.java index 9262b20..50b2136 100644 --- a/query/src/main/java/org/apache/kylin/query/util/PushDownUtil.java +++ b/query/src/main/java/org/apache/kylin/query/util/PushDownUtil.java @@ -59,18 +59,35 @@ import com.google.common.collect.Lists; public class PushDownUtil { private static final Logger logger = LoggerFactory.getLogger(PushDownUtil.class); - public static Pair<List<List<String>>, List<SelectedColumnMeta>> tryPushDownQuery(String project, String sql, + public static Pair<List<List<String>>, List<SelectedColumnMeta>> tryPushDownSelectQuery(String project, String sql, String defaultSchema, SQLException sqlException) throws Exception { + return tryPushDownQuery(project, sql, defaultSchema, sqlException, true); + } + + public static Pair<List<List<String>>, List<SelectedColumnMeta>> tryPushDownNonSelectQuery(String project, + String sql, String defaultSchema) throws Exception { + return tryPushDownQuery(project, sql, defaultSchema, null, false); + } + + private static Pair<List<List<String>>, List<SelectedColumnMeta>> tryPushDownQuery(String project, String sql, + String defaultSchema, SQLException sqlException, boolean isSelect) throws Exception { KylinConfig kylinConfig = KylinConfig.getInstanceFromEnv(); if (!kylinConfig.isPushDownEnabled()) return null; - if (!isExpectedCause(sqlException)) - return null; + if (isSelect) { + logger.info("Query failed to utilize pre-calculation, routing to other engines", sqlException); + if (!isExpectedCause(sqlException)) { + logger.info("quit doPushDownQuery because prior exception thrown is unexpected"); + return null; + } + } else { + Preconditions.checkState(sqlException == null); + logger.info("Kylin cannot support non-select queries, routing to other engines"); + } - logger.info("Query failed to utilize pre-calculation, routing to other engines", sqlException); IPushDownRunner runner = (IPushDownRunner) ClassUtil.newInstance(kylinConfig.getPushDownRunnerClassName()); runner.init(kylinConfig); logger.debug("Query Pushdown runner {}", runner); @@ -103,7 +120,7 @@ public class PushDownUtil { List<List<String>> returnRows = Lists.newArrayList(); List<SelectedColumnMeta> returnColumnMeta = Lists.newArrayList(); - if (QueryUtil.isSelectStatement(sql)) { + if (isSelect) { runner.executeQuery(sql, returnRows, returnColumnMeta); } else { runner.executeUpdate(sql); http://git-wip-us.apache.org/repos/asf/kylin/blob/b1559c66/query/src/main/java/org/apache/kylin/query/util/QueryUtil.java ---------------------------------------------------------------------- diff --git a/query/src/main/java/org/apache/kylin/query/util/QueryUtil.java b/query/src/main/java/org/apache/kylin/query/util/QueryUtil.java index 2dff07c..7a1222e 100644 --- a/query/src/main/java/org/apache/kylin/query/util/QueryUtil.java +++ b/query/src/main/java/org/apache/kylin/query/util/QueryUtil.java @@ -58,8 +58,8 @@ public class QueryUtil { } // https://issues.apache.org/jira/browse/KYLIN-2649 - if (kylinConfig.getForceLimit() > 0 && - !sql.toLowerCase().contains("limit") && sql.toLowerCase().contains("*")) { + if (kylinConfig.getForceLimit() > 0 && !sql.toLowerCase().contains("limit") + && sql.toLowerCase().contains("*")) { sql += ("\nLIMIT " + kylinConfig.getForceLimit()); } @@ -198,15 +198,16 @@ public class QueryUtil { } public static boolean isSelectStatement(String sql) { - String sql1 = removeCommentInSql(sql); - return sql1.startsWith("select") || sql1.startsWith("with") && sql1.contains("select"); + String sql1 = sql.toLowerCase(); + sql1 = removeCommentInSql(sql1); + sql1 = sql1.trim(); + return sql1.startsWith("select") || (sql1.startsWith("with") && sql1.contains("select")); } - public static String removeCommentInSql(String sql) { - String sql1 = sql.toLowerCase(); + public static String removeCommentInSql(String sql1) { // match two patterns, one is "-- comment", the other is "/* comment */" - final String[] commentPatterns = new String[] {"--[^\r\n]*", "/\\*[^\\*/]*"}; - final int[] endOffset = new int[] {0, 2}; + final String[] commentPatterns = new String[] { "--[^\r\n]*", "/\\*[^\\*/]*" }; + final int[] endOffset = new int[] { 0, 2 }; for (int i = 0; i < commentPatterns.length; i++) { String commentPattern = commentPatterns[i]; http://git-wip-us.apache.org/repos/asf/kylin/blob/b1559c66/query/src/test/java/org/apache/kylin/query/util/QueryUtilTest.java ---------------------------------------------------------------------- diff --git a/query/src/test/java/org/apache/kylin/query/util/QueryUtilTest.java b/query/src/test/java/org/apache/kylin/query/util/QueryUtilTest.java index 46f5df4..de044ac 100644 --- a/query/src/test/java/org/apache/kylin/query/util/QueryUtilTest.java +++ b/query/src/test/java/org/apache/kylin/query/util/QueryUtilTest.java @@ -53,6 +53,37 @@ public class QueryUtilTest extends LocalFileMetadataTestCase { } @Test + public void testIsSelect() { + { + String sql = "select ( date '2001-09-28' + interval floor(1.2) day) from test_kylin_fact"; + boolean selectStatement = QueryUtil.isSelectStatement(sql); + Assert.assertEquals(true, selectStatement); + } + { + String sql = " Select ( date '2001-09-28' + interval floor(1.2) day) from test_kylin_fact"; + boolean selectStatement = QueryUtil.isSelectStatement(sql); + Assert.assertEquals(true, selectStatement); + } + { + String sql = " \n" + "Select ( date '2001-09-28' + interval floor(1.2) day) from test_kylin_fact"; + boolean selectStatement = QueryUtil.isSelectStatement(sql); + Assert.assertEquals(true, selectStatement); + } + { + String sql = "--comment\n" + + " /* comment */Select ( date '2001-09-28' + interval floor(1.2) day) from test_kylin_fact"; + boolean selectStatement = QueryUtil.isSelectStatement(sql); + Assert.assertEquals(true, selectStatement); + } + { + String sql = " UPDATE Customers\n" + "SET ContactName = 'Alfred Schmidt', City= 'Frankfurt'\n" + + "WHERE CustomerID = 1;"; + boolean selectStatement = QueryUtil.isSelectStatement(sql); + Assert.assertEquals(false, selectStatement); + } + } + + @Test public void testKeywordDefaultDirtyHack() { { String sql = "select * from DEFAULT.TEST_KYLIN_FACT"; @@ -64,7 +95,7 @@ public class QueryUtilTest extends LocalFileMetadataTestCase { @Test public void testRemoveCommentInSql() { - String originSql = "select count(*) from test_kylin_fact where price > 10.0"; + String originSql = "select count(*) from test_kylin_fact where price > 10.0"; { String sqlWithComment = "-- comment \n" + originSql; http://git-wip-us.apache.org/repos/asf/kylin/blob/b1559c66/server-base/src/main/java/org/apache/kylin/rest/service/QueryService.java ---------------------------------------------------------------------- diff --git a/server-base/src/main/java/org/apache/kylin/rest/service/QueryService.java b/server-base/src/main/java/org/apache/kylin/rest/service/QueryService.java index f57aeb1..e1d0712 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/service/QueryService.java +++ b/server-base/src/main/java/org/apache/kylin/rest/service/QueryService.java @@ -183,20 +183,24 @@ public class QueryService extends BasicService { } } - public SQLResponse update(SQLRequest sqlRequest) throws Exception { // non select operations, only supported when enable pushdown - logger.debug("Query pushdown enabled, redirect the query to alternative engine. "); + logger.debug("Query pushdown enabled, redirect the non-select query to pushdown engine."); Connection conn = null; try { conn = QueryConnection.getConnection(sqlRequest.getProject()); Pair<List<List<String>>, List<SelectedColumnMeta>> r = PushDownUtil - .tryPushDownQuery(sqlRequest.getProject(), sqlRequest.getSql(), conn.getSchema(), null); - return buildSqlResponse(true, r.getFirst(), r.getSecond()); - + .tryPushDownNonSelectQuery(sqlRequest.getProject(), sqlRequest.getSql(), conn.getSchema()); + + List<SelectedColumnMeta> columnMetas = Lists.newArrayList(); + columnMetas.add(new SelectedColumnMeta(false, false, false, false, 1, false, Integer.MAX_VALUE, "c0", "c0", + null, null, null, Integer.MAX_VALUE, 128, 1, "char", false, false, false)); + + return buildSqlResponse(true, r.getFirst(), columnMetas); + } catch (Exception e) { - logger.error("failed to do pushdown, error is " + e.getMessage(), e); - throw new InternalErrorException(e); + logger.info("pushdown engine failed to finish current non-select query"); + throw e; } finally { close(null, null, conn); } @@ -402,9 +406,9 @@ public class QueryService extends BasicService { try { if (null == sqlResponse) { - if (isSelect == true) { + if (isSelect) { sqlResponse = query(sqlRequest); - } else if (kylinConfig.isPushDownEnabled() == true) { + } else if (kylinConfig.isPushDownEnabled()) { sqlResponse = update(sqlRequest); } else { logger.debug( @@ -447,7 +451,7 @@ public class QueryService extends BasicService { checkQueryAuth(sqlResponse, project); } catch (Throwable e) { // calcite may throw AssertError - logger.error("Exception when execute sql", e); + logger.error("Exception while executing query", e); String errMsg = QueryUtil.makeErrorMsgUserFriendly(e); sqlResponse = new SQLResponse(null, null, 0, true, errMsg); @@ -829,17 +833,24 @@ public class QueryService extends BasicService { results.add(oneRow); } - + } catch (SQLException sqlException) { - Pair<List<List<String>>, List<SelectedColumnMeta>> r = PushDownUtil - .tryPushDownQuery(sqlRequest.getProject(), correctedSql, conn.getSchema(), sqlException); + Pair<List<List<String>>, List<SelectedColumnMeta>> r = null; + try { + r = PushDownUtil.tryPushDownSelectQuery(sqlRequest.getProject(), correctedSql, conn.getSchema(), + sqlException); + } catch (Exception e2) { + //exception in pushdown engine will be printed, but we'll not re-throw it, we'll + logger.info("pushdown engine failed current query too", e2); + } + if (r == null) throw sqlException; - + isPushDown = true; results = r.getFirst(); columnMetas = r.getSecond(); - + } finally { close(resultSet, stat, null); //conn is passed in, not my duty to close }
