From 3b10e2d0cf540f3493ce39ef3d91141ace36755e Mon Sep 17 00:00:00 2001 From: David Cui Date: Wed, 5 Feb 2020 14:31:02 -0800 Subject: [PATCH 1/8] correcting Type Information return for JDBC format --- .../sql/executor/format/Protocol.java | 14 +- .../sql/executor/format/SelectResultSet.java | 8 +- .../sql/plugin/RestSqlAction.java | 3 +- .../sql/query/DefaultQueryAction.java | 11 +- .../sql/utils/SQLFunctions.java | 24 +-- .../sql/esintgtest/TypeInformationIT.java | 196 ++++++++++++++++++ .../rewriter/inline/AliasInliningTests.java | 38 +++- 7 files changed, 257 insertions(+), 37 deletions(-) create mode 100644 src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java index c533e03582..363250753b 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java @@ -15,17 +15,20 @@ package com.amazon.opendistroforelasticsearch.sql.executor.format; +import com.amazon.opendistroforelasticsearch.sql.domain.ColumnTypeProvider; import com.amazon.opendistroforelasticsearch.sql.domain.Delete; import com.amazon.opendistroforelasticsearch.sql.domain.IndexStatement; import com.amazon.opendistroforelasticsearch.sql.domain.Query; import com.amazon.opendistroforelasticsearch.sql.domain.QueryStatement; -import com.amazon.opendistroforelasticsearch.sql.executor.format.DataRows.Row; -import com.amazon.opendistroforelasticsearch.sql.executor.format.Schema.Column; import com.amazon.opendistroforelasticsearch.sql.executor.adapter.QueryPlanQueryAction; import com.amazon.opendistroforelasticsearch.sql.executor.adapter.QueryPlanRequestBuilder; +import com.amazon.opendistroforelasticsearch.sql.executor.format.DataRows.Row; +import com.amazon.opendistroforelasticsearch.sql.executor.format.Schema.Column; import com.amazon.opendistroforelasticsearch.sql.expression.domain.BindingTuple; -import com.amazon.opendistroforelasticsearch.sql.query.planner.core.ColumnNode; +import com.amazon.opendistroforelasticsearch.sql.plugin.RestSqlAction; +import com.amazon.opendistroforelasticsearch.sql.query.DefaultQueryAction; import com.amazon.opendistroforelasticsearch.sql.query.QueryAction; +import com.amazon.opendistroforelasticsearch.sql.query.planner.core.ColumnNode; import org.elasticsearch.client.Client; import org.json.JSONArray; import org.json.JSONObject; @@ -48,11 +51,14 @@ public class Protocol { private ResultSet resultSet; private ErrorMessage error; private List columnNodeList; + private ColumnTypeProvider scriptColumnType = new ColumnTypeProvider(); public Protocol(Client client, QueryAction queryAction, Object queryResult, String formatType) { if (queryAction instanceof QueryPlanQueryAction) { this.columnNodeList = ((QueryPlanRequestBuilder) (((QueryPlanQueryAction) queryAction).explain())).outputColumns(); + } else if (queryAction instanceof DefaultQueryAction) { + scriptColumnType = RestSqlAction.performAnalysis(queryAction.getSqlRequest().getSql()); } this.formatType = formatType; QueryStatement query = queryAction.getQueryStatement(); @@ -75,7 +81,7 @@ private ResultSet loadResultSet(Client client, QueryStatement queryStatement, Ob if (queryStatement instanceof Delete) { return new DeleteResultSet(client, (Delete) queryStatement, queryResult); } else if (queryStatement instanceof Query) { - return new SelectResultSet(client, (Query) queryStatement, queryResult); + return new SelectResultSet(client, (Query) queryStatement, queryResult, scriptColumnType); } else if (queryStatement instanceof IndexStatement) { IndexStatement statement = (IndexStatement) queryStatement; StatementType statementType = statement.getStatementType(); diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java index be436b3e3a..335f10e8f0 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java @@ -16,6 +16,7 @@ package com.amazon.opendistroforelasticsearch.sql.executor.format; import com.alibaba.druid.sql.ast.expr.SQLCaseExpr; +import com.amazon.opendistroforelasticsearch.sql.domain.ColumnTypeProvider; import com.amazon.opendistroforelasticsearch.sql.domain.Field; import com.amazon.opendistroforelasticsearch.sql.domain.JoinSelect; import com.amazon.opendistroforelasticsearch.sql.domain.MethodField; @@ -65,17 +66,19 @@ public class SelectResultSet extends ResultSet { private String indexName; private String typeName; private List columns = new ArrayList<>(); + private ColumnTypeProvider scriptColumnType; private List head; private long size; private long totalHits; private List rows; - public SelectResultSet(Client client, Query query, Object queryResult) { + public SelectResultSet(Client client, Query query, Object queryResult, ColumnTypeProvider scriptColumnType) { this.client = client; this.query = query; this.queryResult = queryResult; this.selectAll = false; + this.scriptColumnType = scriptColumnType; if (isJoinQuery()) { JoinSelect joinQuery = (JoinSelect) query; @@ -325,7 +328,7 @@ private Schema.Type fetchMethodReturnType(Field field) { if (field.getExpression() instanceof SQLCaseExpr) { return Schema.Type.TEXT; } - return SQLFunctions.getScriptFunctionReturnType(field); + return SQLFunctions.getScriptFunctionReturnType(field, scriptColumnType); } default: throw new UnsupportedOperationException( @@ -374,6 +377,7 @@ private List populateColumns(Query query, String[] fieldNames, Ma * name instead. */ if (fieldMap.get(fieldName) instanceof MethodField) { + Field methodField = fieldMap.get(fieldName); columns.add( new Schema.Column( diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java index 8c175c943d..1a1ae0f75e 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java @@ -117,6 +117,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli LOG.info("[{}] Incoming request {}: {}", LogUtils.getRequestId(), request.uri(), sqlRequest.getSql()); final QueryAction queryAction = + explainRequest(client, sqlRequest, SqlRequestParam.getFormat(request.params())); return channel -> executeSqlRequest(request, queryAction, client, channel); } catch (Exception e) { @@ -209,7 +210,7 @@ private boolean isSQLFeatureEnabled() { return allowExplicitIndex && isSqlEnabled; } - private static ColumnTypeProvider performAnalysis(String sql) { + public static ColumnTypeProvider performAnalysis(String sql) { LocalClusterState clusterState = LocalClusterState.state(); SqlAnalysisConfig config = new SqlAnalysisConfig( clusterState.getSettingValue(QUERY_ANALYSIS_ENABLED), diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/DefaultQueryAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/DefaultQueryAction.java index fa18ca2677..2caefe4239 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/DefaultQueryAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/DefaultQueryAction.java @@ -19,6 +19,7 @@ import com.alibaba.druid.sql.ast.expr.SQLBinaryOpExpr; import com.alibaba.druid.sql.ast.expr.SQLBinaryOperator; import com.alibaba.druid.sql.ast.expr.SQLCastExpr; +import com.amazon.opendistroforelasticsearch.sql.domain.ColumnTypeProvider; import com.amazon.opendistroforelasticsearch.sql.domain.Field; import com.amazon.opendistroforelasticsearch.sql.domain.KVValue; import com.amazon.opendistroforelasticsearch.sql.domain.MethodField; @@ -29,6 +30,7 @@ import com.amazon.opendistroforelasticsearch.sql.domain.hints.HintType; import com.amazon.opendistroforelasticsearch.sql.exception.SqlParseException; import com.amazon.opendistroforelasticsearch.sql.executor.format.Schema; +import com.amazon.opendistroforelasticsearch.sql.plugin.RestSqlAction; import com.amazon.opendistroforelasticsearch.sql.query.maker.QueryMaker; import com.amazon.opendistroforelasticsearch.sql.rewriter.nestedfield.NestedFieldProjection; import com.amazon.opendistroforelasticsearch.sql.utils.SQLFunctions; @@ -268,12 +270,9 @@ private String getNullOrderString(SQLBinaryOpExpr expr) { private ScriptSortType getScriptSortType(Order order) { ScriptSortType scriptSortType; Schema.Type scriptFunctionReturnType; - if (order.getSortField().getExpression() instanceof SQLCastExpr) { - scriptFunctionReturnType = SQLFunctions.getCastFunctionReturnType( - ((SQLCastExpr) order.getSortField().getExpression()).getDataType().getName()); - } else { - scriptFunctionReturnType = SQLFunctions.getScriptFunctionReturnType(order.getSortField()); - } + ColumnTypeProvider scriptColumnType = RestSqlAction.performAnalysis(this.sqlRequest.getSql()); + scriptFunctionReturnType = SQLFunctions.getScriptFunctionReturnType( + order.getSortField(), scriptColumnType); // as of now script function return type returns only text and double diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java index 33565db746..fafd681468 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java @@ -26,6 +26,7 @@ import com.alibaba.druid.sql.ast.expr.SQLPropertyExpr; import com.alibaba.druid.sql.ast.expr.SQLTextLiteralExpr; import com.alibaba.druid.sql.ast.expr.SQLVariantRefExpr; +import com.amazon.opendistroforelasticsearch.sql.domain.ColumnTypeProvider; import com.amazon.opendistroforelasticsearch.sql.domain.Field; import com.amazon.opendistroforelasticsearch.sql.domain.KVValue; import com.amazon.opendistroforelasticsearch.sql.domain.MethodField; @@ -973,28 +974,21 @@ public String getCastScriptStatement(String name, String castType, List * approach will return type of result column as DOUBLE, although there is enough information to understand that * it might be safely treated as INTEGER. */ - public static Schema.Type getScriptFunctionReturnType(Field field) { + public static Schema.Type getScriptFunctionReturnType(Field field, ColumnTypeProvider scriptColumnType) { + Schema.Type returnType = null; + if (scriptColumnType != null) { + returnType = scriptColumnType.get(0); + } String functionName = ((ScriptMethodField) field).getFunctionName().toLowerCase(); if (functionName.equals("cast")) { String castType = ((SQLCastExpr) field.getExpression()).getDataType().getName(); return getCastFunctionReturnType(castType); } - if (dateFunctions.contains(functionName) || stringOperators.contains(functionName)) { + if (dateFunctions.contains(functionName)) { return Schema.Type.TEXT; } - - if (mathConstants.contains(functionName) || numberOperators.contains(functionName) - || trigFunctions.contains(functionName) || binaryOperators.contains(functionName) - || utilityFunctions.contains(functionName)) { - return Schema.Type.DOUBLE; - } - - if (stringFunctions.contains(functionName)) { - return Schema.Type.INTEGER; - } - - if (conditionalFunctions.contains(functionName)) { - return Schema.Type.KEYWORD; + if (returnType != null) { + return returnType; } throw new UnsupportedOperationException( diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java new file mode 100644 index 0000000000..63a259ecaf --- /dev/null +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java @@ -0,0 +1,196 @@ +/* + * Copyright 2019 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.esintgtest; + +import org.json.JSONObject; +import org.junit.Test; + +import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.rows; +import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.schema; +import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.verifyDataRows; +import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.verifySchema; + +public class TypeInformationIT extends SQLIntegTestCase { + + @Override + protected void init() throws Exception { + loadIndex(Index.ACCOUNT); + loadIndex(Index.ONLINE); + loadIndex(Index.DATE); + } + + /* + numberOperators + */ + @Test + public void testAbsWithIntFieldReturnsInt() { + JSONObject response = executeJdbcRequest("SELECT ABS(age) FROM " + TestsConstants.TEST_INDEX_ACCOUNT + + " ORDER BY age LIMIT 5"); + + verifySchema(response, schema("ABS(age)", null, "long")); + verifyDataRows(response, + rows(20), + rows(20), + rows(20), + rows(20), + rows(20)); + } + + @Test + public void testCeilWithLongFieldReturnsLong() { + JSONObject response = executeJdbcRequest("SELECT CEIL(balance) FROM " + TestsConstants.TEST_INDEX_ACCOUNT + + " ORDER BY balance LIMIT 5"); + + verifySchema(response, schema("CEIL(balance)", null, "long")); + verifyDataRows(response, + rows(1011), + rows(1031), + rows(1110), + rows(1133), + rows(1172)); + } + + /* + stringOperators + */ + @Test + public void testUpperWithStringFieldReturnsString() { + JSONObject response = executeJdbcRequest("SELECT UPPER(firstname) FROM " + + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); + + verifySchema(response, schema("UPPER(firstname)", null, "text")); + verifyDataRows(response, + rows("ABBOTT"), + rows("ABIGAIL")); + } + + @Test + public void testLowerWithTextFieldReturnsText() { + JSONObject response = executeJdbcRequest("SELECT LOWER(firstname) FROM " + + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); + + verifySchema(response, schema("LOWER(firstname)", null, "text")); + verifyDataRows(response, + rows("abbott"), + rows("abigail")); + } + + /* + stringFunctions + */ + @Test + public void testLengthWithTextFieldReturnsInt() { + JSONObject response = executeJdbcRequest("SELECT length(firstname) FROM " + + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); + + verifySchema(response, schema("length(firstname)", null, "integer")); + verifyDataRows(response, + rows(6), + rows(7)); + } + + /* + trigFunctions + */ + @Test + public void testSinWithLongFieldReturnsLong() { + JSONObject response = executeJdbcRequest("SELECT sin(balance) FROM " + + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); + + verifySchema(response, schema("sin(balance)", null, "long")); + verifyDataRows(response, + rows(0.544804964572613), + rows(0.5375391881734781)); + } + + @Test + public void testRadiansWithLongFieldReturnsLong() { + JSONObject response = executeJdbcRequest("SELECT radians(balance) FROM " + + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); + + verifySchema(response, schema("radians(balance)", null, "long")); + verifyDataRows(response, + rows(192.2480171071754), + rows(235.23547658379573)); + } + + /* + binaryOperators + */ + + @Test + public void testAddWithIntReturnsInt() { + JSONObject response = executeJdbcRequest("SELECT (balance + 5) FROM " + + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); + + verifySchema(response, schema("add(balance, 5)", null, "integer")); + verifyDataRows(response, + rows(11020), + rows(13483)); + } + + @Test + public void testSubtractWithLongReturnsLong() { + JSONObject response = executeJdbcRequest("SELECT (balance - balance) FROM " + + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); + + verifySchema(response, schema("subtract(balance, balance)", null, "long")); + verifyDataRows(response, + rows(0), + rows(0)); + } + + /* + dateFunctions + */ + @Test + public void testDayOfWeekWithKeywordReturnsText() { + JSONObject response = executeJdbcRequest("SELECT DAY_OF_WEEK(insert_time) FROM " + + TestsConstants.TEST_INDEX_ONLINE + " LIMIT 2"); + + verifySchema(response, + schema("DAY_OF_WEEK(insert_time)", null, "text")); + + verifyDataRows(response, + rows(7), + rows(7)); + } + + @Test + public void testYearWithKeywordReturnsText() { + JSONObject response = executeJdbcRequest("SELECT YEAR(insert_time) FROM " + + TestsConstants.TEST_INDEX_ONLINE + " LIMIT 2"); + + verifySchema(response, schema("YEAR(insert_time)", null, "text")); + + verifyDataRows(response, + rows(2014), + rows(2014)); + } + + /* + utilityFunctions + */ + @Test + public void testAssign() { + + } + + private JSONObject executeJdbcRequest(String query) { + return new JSONObject(executeQuery(query, "jdbc")); + } + +} diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/inline/AliasInliningTests.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/inline/AliasInliningTests.java index daac610dd2..449198e02e 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/inline/AliasInliningTests.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/inline/AliasInliningTests.java @@ -15,14 +15,25 @@ package com.amazon.opendistroforelasticsearch.sql.unittest.rewriter.inline; +import com.amazon.opendistroforelasticsearch.sql.esdomain.LocalClusterState; +import com.amazon.opendistroforelasticsearch.sql.esintgtest.SQLIntegTestCase; import com.amazon.opendistroforelasticsearch.sql.exception.SqlParseException; import com.amazon.opendistroforelasticsearch.sql.parser.SqlParser; import com.amazon.opendistroforelasticsearch.sql.query.AggregationQueryAction; import com.amazon.opendistroforelasticsearch.sql.query.DefaultQueryAction; +import com.amazon.opendistroforelasticsearch.sql.request.SqlRequest; +import com.google.common.base.Charsets; +import com.google.common.io.Resources; import org.elasticsearch.client.Client; import org.json.JSONObject; +import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; +import java.io.IOException; +import java.net.URL; + +import static com.amazon.opendistroforelasticsearch.sql.util.CheckScriptContents.mockLocalClusterState; import static com.amazon.opendistroforelasticsearch.sql.util.SqlParserUtils.parse; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; @@ -31,6 +42,14 @@ public class AliasInliningTests { + private static final String TEST_MAPPING_FILE = "mappings/semantics.json"; + @Before + public void setUp() throws IOException { + URL url = Resources.getResource(TEST_MAPPING_FILE); + String mappings = Resources.toString(url, Charsets.UTF_8); + mockLocalClusterState(mappings); + } + @Test public void orderByAliasedFieldTest() throws SqlParseException { String originalQuery = "SELECT utc_time date " + @@ -50,16 +69,14 @@ public void orderByAliasedFieldTest() throws SqlParseException { @Test public void orderByAliasedScriptedField() throws SqlParseException { - String originalDsl = parseAsSimpleQuery("SELECT date_format(utc_time, 'dd-MM-YYYY') date " + - "FROM kibana_sample_data_logs " + + String originalDsl = parseAsSimpleQuery("SELECT date_format(birthday, 'dd-MM-YYYY') date " + + "FROM bank " + "ORDER BY date"); - - String rewrittenQuery = "SELECT date_format(utc_time, 'dd-MM-YYYY') date " + - "FROM kibana_sample_data_logs " + - "ORDER BY date_format(utc_time, 'dd-MM-YYYY')"; + String rewrittenQuery = "SELECT date_format(birthday, 'dd-MM-YYYY') date " + + "FROM bank " + + "ORDER BY date_format(birthday, 'dd-MM-YYYY')"; String rewrittenDsl = parseAsSimpleQuery(rewrittenQuery); - assertThat(originalDsl, equalTo(rewrittenDsl)); } @@ -111,8 +128,11 @@ public void groupByAndSortAliased() throws SqlParseException { } private String parseAsSimpleQuery(String originalQuery) throws SqlParseException { - return new DefaultQueryAction(mock(Client.class), - new SqlParser().parseSelect(parse(originalQuery))).explain().explain(); + SqlRequest sqlRequest = new SqlRequest(originalQuery, new JSONObject()); + DefaultQueryAction defaultQueryAction = new DefaultQueryAction(mock(Client.class), + new SqlParser().parseSelect(parse(originalQuery))); + defaultQueryAction.setSqlRequest(sqlRequest); + return defaultQueryAction.explain().explain(); } private String parseAsAggregationQuery(String originalQuery) throws SqlParseException { From ee178aed0d25d4d9d719c80e8d41db10dcc7a5e5 Mon Sep 17 00:00:00 2001 From: David Cui Date: Wed, 5 Feb 2020 14:58:38 -0800 Subject: [PATCH 2/8] added math Constants test --- .../sql/esintgtest/TypeInformationIT.java | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java index 63a259ecaf..30a7bb12e1 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java @@ -63,15 +63,28 @@ public void testCeilWithLongFieldReturnsLong() { rows(1172)); } + /* + mathConstants + */ + @Test + public void testPiReturnsDouble() { + JSONObject response = executeJdbcRequest("SELECT PI() FROM " + TestsConstants.TEST_INDEX_ACCOUNT + + " LIMIT 1"); + + verifySchema(response, schema("PI()", null, "double")); + verifyDataRows(response, + rows(3.141592653589793)); + } + /* stringOperators */ @Test public void testUpperWithStringFieldReturnsString() { - JSONObject response = executeJdbcRequest("SELECT UPPER(firstname) FROM " + - TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); + JSONObject response = executeJdbcRequest("SELECT UPPER(firstname) AS firstname_alias FROM " + + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname_alias LIMIT 2"); - verifySchema(response, schema("UPPER(firstname)", null, "text")); + verifySchema(response, schema("firstname_alias", null, "text")); verifyDataRows(response, rows("ABBOTT"), rows("ABIGAIL")); @@ -133,17 +146,17 @@ public void testRadiansWithLongFieldReturnsLong() { @Test public void testAddWithIntReturnsInt() { - JSONObject response = executeJdbcRequest("SELECT (balance + 5) FROM " + + JSONObject response = executeJdbcRequest("SELECT (balance + 5) AS balance_add_five FROM " + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); - verifySchema(response, schema("add(balance, 5)", null, "integer")); + verifySchema(response, schema("balance_add_five", null, "integer")); verifyDataRows(response, rows(11020), rows(13483)); } @Test - public void testSubtractWithLongReturnsLong() { + public void testSubtractLongWithLongReturnsLong() { JSONObject response = executeJdbcRequest("SELECT (balance - balance) FROM " + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); @@ -181,14 +194,6 @@ public void testYearWithKeywordReturnsText() { rows(2014)); } - /* - utilityFunctions - */ - @Test - public void testAssign() { - - } - private JSONObject executeJdbcRequest(String query) { return new JSONObject(executeQuery(query, "jdbc")); } From 537c42e861fa35aa6a69829c23bd77e7d4cf2ade Mon Sep 17 00:00:00 2001 From: David Cui Date: Thu, 6 Feb 2020 17:07:29 -0800 Subject: [PATCH 3/8] fixed type assignment for multiple fields and added hard-coding for ORDER BY script fields --- .../sql/executor/format/SelectResultSet.java | 7 ++-- .../sql/query/DefaultQueryAction.java | 7 +--- .../sql/utils/SQLFunctions.java | 42 +++++++++++++++---- .../sql/esintgtest/TypeInformationIT.java | 7 ++-- 4 files changed, 42 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java index 335f10e8f0..84183743b1 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java @@ -311,7 +311,7 @@ private String[] emptyArrayIfNull(String typeName) { } } - private Schema.Type fetchMethodReturnType(Field field) { + private Schema.Type fetchMethodReturnType(int fieldIndex, Field field) { switch (field.getName().toLowerCase()) { case "count": return Schema.Type.LONG; @@ -328,7 +328,7 @@ private Schema.Type fetchMethodReturnType(Field field) { if (field.getExpression() instanceof SQLCaseExpr) { return Schema.Type.TEXT; } - return SQLFunctions.getScriptFunctionReturnType(field, scriptColumnType); + return SQLFunctions.getScriptFunctionReturnType(fieldIndex, field, scriptColumnType); } default: throw new UnsupportedOperationException( @@ -379,11 +379,12 @@ private List populateColumns(Query query, String[] fieldNames, Ma if (fieldMap.get(fieldName) instanceof MethodField) { Field methodField = fieldMap.get(fieldName); + int fieldIndex = fieldNameList.indexOf(fieldName); columns.add( new Schema.Column( methodField.getAlias(), null, - fetchMethodReturnType(methodField) + fetchMethodReturnType(fieldIndex, methodField) ) ); } diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/DefaultQueryAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/DefaultQueryAction.java index 2caefe4239..25854a5db6 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/DefaultQueryAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/DefaultQueryAction.java @@ -19,7 +19,6 @@ import com.alibaba.druid.sql.ast.expr.SQLBinaryOpExpr; import com.alibaba.druid.sql.ast.expr.SQLBinaryOperator; import com.alibaba.druid.sql.ast.expr.SQLCastExpr; -import com.amazon.opendistroforelasticsearch.sql.domain.ColumnTypeProvider; import com.amazon.opendistroforelasticsearch.sql.domain.Field; import com.amazon.opendistroforelasticsearch.sql.domain.KVValue; import com.amazon.opendistroforelasticsearch.sql.domain.MethodField; @@ -30,7 +29,6 @@ import com.amazon.opendistroforelasticsearch.sql.domain.hints.HintType; import com.amazon.opendistroforelasticsearch.sql.exception.SqlParseException; import com.amazon.opendistroforelasticsearch.sql.executor.format.Schema; -import com.amazon.opendistroforelasticsearch.sql.plugin.RestSqlAction; import com.amazon.opendistroforelasticsearch.sql.query.maker.QueryMaker; import com.amazon.opendistroforelasticsearch.sql.rewriter.nestedfield.NestedFieldProjection; import com.amazon.opendistroforelasticsearch.sql.utils.SQLFunctions; @@ -269,10 +267,7 @@ private String getNullOrderString(SQLBinaryOpExpr expr) { private ScriptSortType getScriptSortType(Order order) { ScriptSortType scriptSortType; - Schema.Type scriptFunctionReturnType; - ColumnTypeProvider scriptColumnType = RestSqlAction.performAnalysis(this.sqlRequest.getSql()); - scriptFunctionReturnType = SQLFunctions.getScriptFunctionReturnType( - order.getSortField(), scriptColumnType); + Schema.Type scriptFunctionReturnType = SQLFunctions.getOrderByFieldType(order.getSortField()); // as of now script function return type returns only text and double diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java index fafd681468..8274290ac0 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java @@ -521,9 +521,10 @@ public Tuple split(SQLExpr field, String pattern, String valueNa private Tuple date_format(SQLExpr field, String pattern, String zoneId, String valueName) { String name = nextId("date_format"); if (valueName == null) { - return new Tuple<>(name, "def " + name + " = DateTimeFormatter.ofPattern('" + pattern + "').withZone(" + return new Tuple<>(name, "def " + name + " = DateTimeFormatter.ofPattern('" + + pattern + "').withZone(" + (zoneId != null ? "ZoneId.of('" + zoneId + "')" : "ZoneId.systemDefault()") - + ").format(Instant.ofEpochMilli(" + getPropertyOrValue(field) + ".getMillis()))"); + + ").format(Instant.ofEpochMilli(" + getPropertyOrValue(field) + ".toInstant().toEpochMilli()))"); } else { return new Tuple<>(name, exprString(field) + "; " + "def " + name + " = new SimpleDateFormat('" + pattern + "').format(" @@ -974,19 +975,17 @@ public String getCastScriptStatement(String name, String castType, List * approach will return type of result column as DOUBLE, although there is enough information to understand that * it might be safely treated as INTEGER. */ - public static Schema.Type getScriptFunctionReturnType(Field field, ColumnTypeProvider scriptColumnType) { + public static Schema.Type getScriptFunctionReturnType( + int fieldIndex, Field field, ColumnTypeProvider scriptColumnType) { Schema.Type returnType = null; - if (scriptColumnType != null) { - returnType = scriptColumnType.get(0); + if (scriptColumnType != null && fieldIndex != -1) { + returnType = scriptColumnType.get(fieldIndex); } String functionName = ((ScriptMethodField) field).getFunctionName().toLowerCase(); if (functionName.equals("cast")) { String castType = ((SQLCastExpr) field.getExpression()).getDataType().getName(); return getCastFunctionReturnType(castType); } - if (dateFunctions.contains(functionName)) { - return Schema.Type.TEXT; - } if (returnType != null) { return returnType; } @@ -1017,4 +1016,31 @@ public static Schema.Type getCastFunctionReturnType(String castType) { ); } } + + public static Schema.Type getOrderByFieldType(Field field) { + String functionName = ((ScriptMethodField) field).getFunctionName().toLowerCase(); + if (functionName.equals("cast")) { + String castType = ((SQLCastExpr) field.getExpression()).getDataType().getName(); + return getCastFunctionReturnType(castType); + } + + if (numberOperators.contains(functionName) || mathConstants.contains(functionName) + || trigFunctions.contains(functionName) || binaryOperators.contains(functionName)) { + return Schema.Type.DOUBLE; + } else if (dateFunctions.contains(functionName)) { + if (functionName.equals("date_format") || functionName.equals("now") + || functionName.equals("curdate") || functionName.equals("date") + || functionName.equals("timestamp") || functionName.equals("monthname")) { + return Schema.Type.TEXT; + } + return Schema.Type.DOUBLE; + } else if (stringFunctions.contains(functionName) || stringOperators.contains(functionName)) { + return Schema.Type.TEXT; + } + + throw new UnsupportedOperationException( + String.format( + "The following method is not supported in Schema for Order By: %s", + functionName)); + } } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java index 30a7bb12e1..0cc307cd0f 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java @@ -15,6 +15,7 @@ package com.amazon.opendistroforelasticsearch.sql.esintgtest; +import org.json.JSONArray; import org.json.JSONObject; import org.junit.Test; @@ -29,7 +30,6 @@ public class TypeInformationIT extends SQLIntegTestCase { protected void init() throws Exception { loadIndex(Index.ACCOUNT); loadIndex(Index.ONLINE); - loadIndex(Index.DATE); } /* @@ -143,7 +143,6 @@ public void testRadiansWithLongFieldReturnsLong() { /* binaryOperators */ - @Test public void testAddWithIntReturnsInt() { JSONObject response = executeJdbcRequest("SELECT (balance + 5) AS balance_add_five FROM " + @@ -175,7 +174,7 @@ public void testDayOfWeekWithKeywordReturnsText() { + TestsConstants.TEST_INDEX_ONLINE + " LIMIT 2"); verifySchema(response, - schema("DAY_OF_WEEK(insert_time)", null, "text")); + schema("DAY_OF_WEEK(insert_time)", null, "integer")); verifyDataRows(response, rows(7), @@ -187,7 +186,7 @@ public void testYearWithKeywordReturnsText() { JSONObject response = executeJdbcRequest("SELECT YEAR(insert_time) FROM " + TestsConstants.TEST_INDEX_ONLINE + " LIMIT 2"); - verifySchema(response, schema("YEAR(insert_time)", null, "text")); + verifySchema(response, schema("YEAR(insert_time)", null, "integer")); verifyDataRows(response, rows(2014), From f05894ff645f149c4eb6a27d3b31ec018e78ea28 Mon Sep 17 00:00:00 2001 From: David Cui Date: Thu, 6 Feb 2020 17:11:02 -0800 Subject: [PATCH 4/8] minor formatting corrections --- .../opendistroforelasticsearch/sql/plugin/RestSqlAction.java | 1 - .../opendistroforelasticsearch/sql/utils/SQLFunctions.java | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java index 1a1ae0f75e..5501d5d90e 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java @@ -117,7 +117,6 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli LOG.info("[{}] Incoming request {}: {}", LogUtils.getRequestId(), request.uri(), sqlRequest.getSql()); final QueryAction queryAction = - explainRequest(client, sqlRequest, SqlRequestParam.getFormat(request.params())); return channel -> executeSqlRequest(request, queryAction, client, channel); } catch (Exception e) { diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java index 8274290ac0..348ab0d55d 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java @@ -521,8 +521,7 @@ public Tuple split(SQLExpr field, String pattern, String valueNa private Tuple date_format(SQLExpr field, String pattern, String zoneId, String valueName) { String name = nextId("date_format"); if (valueName == null) { - return new Tuple<>(name, "def " + name + " = DateTimeFormatter.ofPattern('" - + pattern + "').withZone(" + return new Tuple<>(name, "def " + name + " = DateTimeFormatter.ofPattern('" + pattern + "').withZone(" + (zoneId != null ? "ZoneId.of('" + zoneId + "')" : "ZoneId.systemDefault()") + ").format(Instant.ofEpochMilli(" + getPropertyOrValue(field) + ".toInstant().toEpochMilli()))"); } else { From 65b58e3396470b9c7ee8c35a73f2a25f5ab6bf63 Mon Sep 17 00:00:00 2001 From: David Cui Date: Tue, 18 Feb 2020 11:14:53 -0800 Subject: [PATCH 5/8] eliminated second performAnalysis() call, refactored ITs and added UTs. --- .../types/function/ScalarFunction.java | 24 +++--- .../sql/executor/format/Protocol.java | 3 +- .../sql/executor/format/SelectResultSet.java | 6 +- .../sql/plugin/RestSqlAction.java | 1 + .../sql/query/QueryAction.java | 10 +++ .../sql/utils/SQLFunctions.java | 35 +++++---- .../sql/esintgtest/TypeInformationIT.java | 54 +------------- .../sql/unittest/utils/SQLFunctionsTest.java | 74 +++++++++++++++++++ 8 files changed, 127 insertions(+), 80 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/types/function/ScalarFunction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/types/function/ScalarFunction.java index 82dee84297..4602f3ce0d 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/types/function/ScalarFunction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/types/function/ScalarFunction.java @@ -33,20 +33,20 @@ public enum ScalarFunction implements TypeExpression { ABS(func(T(NUMBER)).to(T)), // translate to Java: T ABS(T) - ACOS(func(T(NUMBER)).to(T)), + ACOS(func(T(NUMBER)).to(DOUBLE)), ADD(func(T(NUMBER), NUMBER).to(T)), ASCII(func(T(STRING)).to(INTEGER)), - ASIN(func(T(NUMBER)).to(T)), - ATAN(func(T(NUMBER)).to(T)), - ATAN2(func(T(NUMBER), NUMBER).to(T)), + ASIN(func(T(NUMBER)).to(DOUBLE)), + ATAN(func(T(NUMBER)).to(DOUBLE)), + ATAN2(func(T(NUMBER), NUMBER).to(DOUBLE)), CAST(), CBRT(func(T(NUMBER)).to(T)), CEIL(func(T(NUMBER)).to(T)), CONCAT(), // TODO: varargs support required CONCAT_WS(), - COS(func(T(NUMBER)).to(T)), - COSH(func(T(NUMBER)).to(T)), - COT(func(T(NUMBER)).to(T)), + COS(func(T(NUMBER)).to(DOUBLE)), + COSH(func(T(NUMBER)).to(DOUBLE)), + COT(func(T(NUMBER)).to(DOUBLE)), CURDATE(func().to(ESDataType.DATE)), DATE(func(ESDataType.DATE).to(ESDataType.DATE)), DATE_FORMAT( @@ -54,7 +54,7 @@ public enum ScalarFunction implements TypeExpression { func(ESDataType.DATE, STRING, STRING).to(STRING) ), DAYOFMONTH(func(ESDataType.DATE).to(INTEGER)), - DEGREES(func(T(NUMBER)).to(T)), + DEGREES(func(T(NUMBER)).to(DOUBLE)), DIVIDE(func(T(NUMBER), NUMBER).to(T)), E(func().to(DOUBLE)), EXP(func(T(NUMBER)).to(T)), @@ -96,7 +96,7 @@ public enum ScalarFunction implements TypeExpression { func(T(NUMBER)).to(T), func(T(NUMBER), NUMBER).to(T) ), - RADIANS(func(T(NUMBER)).to(T)), + RADIANS(func(T(NUMBER)).to(DOUBLE)), RAND( func().to(NUMBER), func(T(NUMBER)).to(T) @@ -108,12 +108,12 @@ public enum ScalarFunction implements TypeExpression { RTRIM(func(T(STRING)).to(T)), SIGN(func(T(NUMBER)).to(T)), SIGNUM(func(T(NUMBER)).to(T)), - SIN(func(T(NUMBER)).to(T)), - SINH(func(T(NUMBER)).to(T)), + SIN(func(T(NUMBER)).to(DOUBLE)), + SINH(func(T(NUMBER)).to(DOUBLE)), SQRT(func(T(NUMBER)).to(T)), SUBSTRING(func(T(STRING), INTEGER, INTEGER).to(T)), SUBTRACT(func(T(NUMBER), NUMBER).to(T)), - TAN(func(T(NUMBER)).to(T)), + TAN(func(T(NUMBER)).to(DOUBLE)), TIMESTAMP(func(ESDataType.DATE).to(ESDataType.DATE)), TRIM(func(T(STRING)).to(T)), UPPER( diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java index 363250753b..827f2ae9e2 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java @@ -25,7 +25,6 @@ import com.amazon.opendistroforelasticsearch.sql.executor.format.DataRows.Row; import com.amazon.opendistroforelasticsearch.sql.executor.format.Schema.Column; import com.amazon.opendistroforelasticsearch.sql.expression.domain.BindingTuple; -import com.amazon.opendistroforelasticsearch.sql.plugin.RestSqlAction; import com.amazon.opendistroforelasticsearch.sql.query.DefaultQueryAction; import com.amazon.opendistroforelasticsearch.sql.query.QueryAction; import com.amazon.opendistroforelasticsearch.sql.query.planner.core.ColumnNode; @@ -58,7 +57,7 @@ public Protocol(Client client, QueryAction queryAction, Object queryResult, Stri this.columnNodeList = ((QueryPlanRequestBuilder) (((QueryPlanQueryAction) queryAction).explain())).outputColumns(); } else if (queryAction instanceof DefaultQueryAction) { - scriptColumnType = RestSqlAction.performAnalysis(queryAction.getSqlRequest().getSql()); + scriptColumnType = queryAction.getScriptColumnType(); } this.formatType = formatType; QueryStatement query = queryAction.getQueryStatement(); diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java index 84183743b1..686fbbe4ba 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java @@ -311,7 +311,7 @@ private String[] emptyArrayIfNull(String typeName) { } } - private Schema.Type fetchMethodReturnType(int fieldIndex, Field field) { + private Schema.Type fetchMethodReturnType(int fieldIndex, MethodField field) { switch (field.getName().toLowerCase()) { case "count": return Schema.Type.LONG; @@ -328,7 +328,7 @@ private Schema.Type fetchMethodReturnType(int fieldIndex, Field field) { if (field.getExpression() instanceof SQLCaseExpr) { return Schema.Type.TEXT; } - return SQLFunctions.getScriptFunctionReturnType(fieldIndex, field, scriptColumnType); + return SQLFunctions.getScriptFunctionReturnType(fieldIndex, field, scriptColumnType); } default: throw new UnsupportedOperationException( @@ -378,7 +378,7 @@ private List populateColumns(Query query, String[] fieldNames, Ma */ if (fieldMap.get(fieldName) instanceof MethodField) { - Field methodField = fieldMap.get(fieldName); + MethodField methodField = (MethodField) fieldMap.get(fieldName); int fieldIndex = fieldNameList.indexOf(fieldName); columns.add( new Schema.Column( diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java index 5501d5d90e..fe2c293e2e 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java @@ -150,6 +150,7 @@ private static QueryAction explainRequest(final NodeClient client, final SqlRequ final QueryAction queryAction = new SearchDao(client) .explain(new QueryActionRequest(sqlRequest.getSql(), typeProvider, format)); queryAction.setSqlRequest(sqlRequest); + queryAction.setColumnTypeProvider(typeProvider); return queryAction; } diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/QueryAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/QueryAction.java index d83031eb25..ed1c02cb5e 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/QueryAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/QueryAction.java @@ -15,6 +15,7 @@ package com.amazon.opendistroforelasticsearch.sql.query; +import com.amazon.opendistroforelasticsearch.sql.domain.ColumnTypeProvider; import com.amazon.opendistroforelasticsearch.sql.domain.Query; import com.amazon.opendistroforelasticsearch.sql.domain.QueryStatement; import com.amazon.opendistroforelasticsearch.sql.domain.Select; @@ -48,6 +49,7 @@ public abstract class QueryAction { protected Query query; protected Client client; protected SqlRequest sqlRequest = SqlRequest.NULL; + protected ColumnTypeProvider scriptColumnType; public QueryAction(Client client, Query query) { this.client = client; @@ -66,10 +68,18 @@ public void setSqlRequest(SqlRequest sqlRequest) { this.sqlRequest = sqlRequest; } + public void setColumnTypeProvider(ColumnTypeProvider scriptColumnType) { + this.scriptColumnType = scriptColumnType; + } + public SqlRequest getSqlRequest() { return sqlRequest; } + public ColumnTypeProvider getScriptColumnType() { + return scriptColumnType; + } + /** * @return List of field names produced by the query */ diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java index 348ab0d55d..5cf0ddf014 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java @@ -975,24 +975,26 @@ public String getCastScriptStatement(String name, String castType, List * it might be safely treated as INTEGER. */ public static Schema.Type getScriptFunctionReturnType( - int fieldIndex, Field field, ColumnTypeProvider scriptColumnType) { - Schema.Type returnType = null; - if (scriptColumnType != null && fieldIndex != -1) { - returnType = scriptColumnType.get(fieldIndex); - } + int fieldIndex, MethodField field, ColumnTypeProvider scriptColumnType) { + Schema.Type returnType; String functionName = ((ScriptMethodField) field).getFunctionName().toLowerCase(); + if (!numberOperators.contains(functionName) && !mathConstants.contains(functionName) + && !trigFunctions.contains(functionName) && !stringOperators.contains(functionName) + && !stringFunctions.contains(functionName) && !binaryOperators.contains(functionName) + && !dateFunctions.contains(functionName) && !conditionalFunctions.contains(functionName) + && !utilityFunctions.contains(functionName)) { + throw new UnsupportedOperationException( + String.format( + "The following method is not supported in Schema: %s", + functionName)); + } if (functionName.equals("cast")) { String castType = ((SQLCastExpr) field.getExpression()).getDataType().getName(); return getCastFunctionReturnType(castType); + } else { + returnType = scriptColumnType.get(fieldIndex); } - if (returnType != null) { - return returnType; - } - - throw new UnsupportedOperationException( - String.format( - "The following method is not supported in Schema: %s", - functionName)); + return returnType; } public static Schema.Type getCastFunctionReturnType(String castType) { @@ -1016,6 +1018,13 @@ public static Schema.Type getCastFunctionReturnType(String castType) { } } + /** + * + * @param field + * @return Schema.Type.TEXT or DOUBLE + * There are only two ORDER BY types (TEXT, NUMBER) in Elasticsearch, so the Type that is returned here essentially + * indicates the category of the function as opposed to the actual return type. + */ public static Schema.Type getOrderByFieldType(Field field) { String functionName = ((ScriptMethodField) field).getFunctionName().toLowerCase(); if (functionName.equals("cast")) { diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java index 0cc307cd0f..81d6198d98 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TypeInformationIT.java @@ -15,13 +15,10 @@ package com.amazon.opendistroforelasticsearch.sql.esintgtest; -import org.json.JSONArray; import org.json.JSONObject; import org.junit.Test; -import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.rows; import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.schema; -import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.verifyDataRows; import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.verifySchema; public class TypeInformationIT extends SQLIntegTestCase { @@ -41,12 +38,6 @@ public void testAbsWithIntFieldReturnsInt() { " ORDER BY age LIMIT 5"); verifySchema(response, schema("ABS(age)", null, "long")); - verifyDataRows(response, - rows(20), - rows(20), - rows(20), - rows(20), - rows(20)); } @Test @@ -55,12 +46,6 @@ public void testCeilWithLongFieldReturnsLong() { " ORDER BY balance LIMIT 5"); verifySchema(response, schema("CEIL(balance)", null, "long")); - verifyDataRows(response, - rows(1011), - rows(1031), - rows(1110), - rows(1133), - rows(1172)); } /* @@ -72,8 +57,6 @@ public void testPiReturnsDouble() { + " LIMIT 1"); verifySchema(response, schema("PI()", null, "double")); - verifyDataRows(response, - rows(3.141592653589793)); } /* @@ -85,9 +68,6 @@ public void testUpperWithStringFieldReturnsString() { TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname_alias LIMIT 2"); verifySchema(response, schema("firstname_alias", null, "text")); - verifyDataRows(response, - rows("ABBOTT"), - rows("ABIGAIL")); } @Test @@ -96,9 +76,6 @@ public void testLowerWithTextFieldReturnsText() { TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); verifySchema(response, schema("LOWER(firstname)", null, "text")); - verifyDataRows(response, - rows("abbott"), - rows("abigail")); } /* @@ -110,34 +87,25 @@ public void testLengthWithTextFieldReturnsInt() { TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); verifySchema(response, schema("length(firstname)", null, "integer")); - verifyDataRows(response, - rows(6), - rows(7)); } /* trigFunctions */ @Test - public void testSinWithLongFieldReturnsLong() { + public void testSinWithLongFieldReturnsDouble() { JSONObject response = executeJdbcRequest("SELECT sin(balance) FROM " + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); - verifySchema(response, schema("sin(balance)", null, "long")); - verifyDataRows(response, - rows(0.544804964572613), - rows(0.5375391881734781)); + verifySchema(response, schema("sin(balance)", null, "double")); } @Test - public void testRadiansWithLongFieldReturnsLong() { + public void testRadiansWithLongFieldReturnsDouble() { JSONObject response = executeJdbcRequest("SELECT radians(balance) FROM " + TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); - verifySchema(response, schema("radians(balance)", null, "long")); - verifyDataRows(response, - rows(192.2480171071754), - rows(235.23547658379573)); + verifySchema(response, schema("radians(balance)", null, "double")); } /* @@ -149,9 +117,6 @@ public void testAddWithIntReturnsInt() { TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); verifySchema(response, schema("balance_add_five", null, "integer")); - verifyDataRows(response, - rows(11020), - rows(13483)); } @Test @@ -160,9 +125,6 @@ public void testSubtractLongWithLongReturnsLong() { TestsConstants.TEST_INDEX_ACCOUNT + " ORDER BY firstname LIMIT 2"); verifySchema(response, schema("subtract(balance, balance)", null, "long")); - verifyDataRows(response, - rows(0), - rows(0)); } /* @@ -175,10 +137,6 @@ public void testDayOfWeekWithKeywordReturnsText() { verifySchema(response, schema("DAY_OF_WEEK(insert_time)", null, "integer")); - - verifyDataRows(response, - rows(7), - rows(7)); } @Test @@ -187,10 +145,6 @@ public void testYearWithKeywordReturnsText() { + TestsConstants.TEST_INDEX_ONLINE + " LIMIT 2"); verifySchema(response, schema("YEAR(insert_time)", null, "integer")); - - verifyDataRows(response, - rows(2014), - rows(2014)); } private JSONObject executeJdbcRequest(String query) { diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java index 74a67948ee..ab353bbf3b 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java @@ -15,18 +15,48 @@ package com.amazon.opendistroforelasticsearch.sql.unittest.utils; +import com.alibaba.druid.sql.ast.SQLDataType; +import com.alibaba.druid.sql.ast.SQLDataTypeImpl; +import com.alibaba.druid.sql.ast.expr.SQLCastExpr; +import com.alibaba.druid.sql.ast.expr.SQLIdentifierExpr; import com.alibaba.druid.sql.ast.expr.SQLIntegerExpr; +import com.alibaba.druid.sql.ast.expr.SQLMethodInvokeExpr; +import com.amazon.opendistroforelasticsearch.sql.antlr.semantic.types.base.ESDataType; +import com.amazon.opendistroforelasticsearch.sql.domain.ColumnTypeProvider; import com.amazon.opendistroforelasticsearch.sql.domain.KVValue; +import com.amazon.opendistroforelasticsearch.sql.domain.MethodField; +import com.amazon.opendistroforelasticsearch.sql.domain.ScriptMethodField; import com.amazon.opendistroforelasticsearch.sql.exception.SqlParseException; +import com.amazon.opendistroforelasticsearch.sql.executor.format.Schema; +import com.amazon.opendistroforelasticsearch.sql.parser.FieldMaker; import com.amazon.opendistroforelasticsearch.sql.utils.SQLFunctions; import com.google.common.collect.ImmutableList; import org.elasticsearch.common.collect.Tuple; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; + +import java.util.ArrayList; +import java.util.List; import static org.junit.Assert.assertTrue; public class SQLFunctionsTest { + private FieldMaker fieldMaker; + private SQLFunctions sqlFunctions; + + @Rule + public ExpectedException exceptionRule = ExpectedException.none(); + + @Before + public void init() { + fieldMaker = new FieldMaker(); + } + public void initSqlFunctions() { sqlFunctions = new SQLFunctions(); } + @Test public void testAssign() throws SqlParseException { SQLFunctions sqlFunctions = new SQLFunctions(); @@ -40,4 +70,48 @@ public void testAssign() throws SqlParseException { assertTrue(assign.v1().matches("assign_[0-9]+")); assertTrue(assign.v2().matches("def assign_[0-9]+ = 10;return assign_[0-9]+;")); } + + @Test + public void testAbsWithIntReturnType() { + final SQLIntegerExpr sqlIntegerExpr = new SQLIntegerExpr(6); + + final SQLMethodInvokeExpr invokeExpr = new SQLMethodInvokeExpr("ABS"); + invokeExpr.addParameter(sqlIntegerExpr); + List params = new ArrayList<>(); + + final MethodField field = new ScriptMethodField("ABS", params, null, null); + field.setExpression(invokeExpr); + ColumnTypeProvider columnTypeProvider = new ColumnTypeProvider(ESDataType.INTEGER); + + final Schema.Type returnType = sqlFunctions.getScriptFunctionReturnType(0, field, columnTypeProvider); + Assert.assertEquals(returnType, Schema.Type.INTEGER); + } + + @Test + public void testCastReturnType() { + final SQLIdentifierExpr identifierExpr = new SQLIdentifierExpr("int_type"); + SQLDataType sqlDataType = new SQLDataTypeImpl("INT"); + final SQLCastExpr castExpr = new SQLCastExpr(); + castExpr.setExpr(identifierExpr); + castExpr.setDataType(sqlDataType); + + List params = new ArrayList<>(); + final MethodField field = new ScriptMethodField("CAST", params, null, null); + field.setExpression(castExpr); + ColumnTypeProvider columnTypeProvider = new ColumnTypeProvider(ESDataType.INTEGER); + + final Schema.Type returnType = sqlFunctions.getScriptFunctionReturnType(0, field, columnTypeProvider); + Assert.assertEquals(returnType, Schema.Type.INTEGER); + } + + @Test + public void testNullScriptReturnTypeThrowsException() throws UnsupportedOperationException { + exceptionRule.expect(UnsupportedOperationException.class); + exceptionRule.expectMessage("The following method is not supported in Schema: lo"); + + List params = new ArrayList<>(); + MethodField field = new ScriptMethodField("LO", params, null, null); + ColumnTypeProvider columnTypeProvider = new ColumnTypeProvider(ESDataType.INTEGER); + final Schema.Type type = sqlFunctions.getScriptFunctionReturnType(0, field, columnTypeProvider); + } } \ No newline at end of file From 6afd24ee177cf4d44162bdea8c1622627a555907 Mon Sep 17 00:00:00 2001 From: David Cui Date: Thu, 20 Feb 2020 14:52:09 -0800 Subject: [PATCH 6/8] addressed comments- refactored columnType variable and removed unsupported case check in SQLFunctions --- .../sql/executor/format/SelectResultSet.java | 10 +++++----- .../sql/plugin/RestSqlAction.java | 2 +- .../sql/utils/SQLFunctions.java | 16 ++-------------- .../sql/unittest/utils/SQLFunctionsTest.java | 18 +++++------------- 4 files changed, 13 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java index 686fbbe4ba..7e2e2e9239 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java @@ -66,19 +66,19 @@ public class SelectResultSet extends ResultSet { private String indexName; private String typeName; private List columns = new ArrayList<>(); - private ColumnTypeProvider scriptColumnType; + private ColumnTypeProvider outputColumnType; private List head; private long size; private long totalHits; private List rows; - public SelectResultSet(Client client, Query query, Object queryResult, ColumnTypeProvider scriptColumnType) { + public SelectResultSet(Client client, Query query, Object queryResult, ColumnTypeProvider outputColumnType) { this.client = client; this.query = query; this.queryResult = queryResult; this.selectAll = false; - this.scriptColumnType = scriptColumnType; + this.outputColumnType = outputColumnType; if (isJoinQuery()) { JoinSelect joinQuery = (JoinSelect) query; @@ -328,7 +328,8 @@ private Schema.Type fetchMethodReturnType(int fieldIndex, MethodField field) { if (field.getExpression() instanceof SQLCaseExpr) { return Schema.Type.TEXT; } - return SQLFunctions.getScriptFunctionReturnType(fieldIndex, field, scriptColumnType); + Schema.Type resolvedType = outputColumnType.get(fieldIndex); + return SQLFunctions.getScriptFunctionReturnType(field, resolvedType); } default: throw new UnsupportedOperationException( @@ -377,7 +378,6 @@ private List populateColumns(Query query, String[] fieldNames, Ma * name instead. */ if (fieldMap.get(fieldName) instanceof MethodField) { - MethodField methodField = (MethodField) fieldMap.get(fieldName); int fieldIndex = fieldNameList.indexOf(fieldName); columns.add( diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java index fe2c293e2e..6cb86b4afc 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java @@ -210,7 +210,7 @@ private boolean isSQLFeatureEnabled() { return allowExplicitIndex && isSqlEnabled; } - public static ColumnTypeProvider performAnalysis(String sql) { + private static ColumnTypeProvider performAnalysis(String sql) { LocalClusterState clusterState = LocalClusterState.state(); SqlAnalysisConfig config = new SqlAnalysisConfig( clusterState.getSettingValue(QUERY_ANALYSIS_ENABLED), diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java index 5cf0ddf014..e8bd017536 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java @@ -26,7 +26,6 @@ import com.alibaba.druid.sql.ast.expr.SQLPropertyExpr; import com.alibaba.druid.sql.ast.expr.SQLTextLiteralExpr; import com.alibaba.druid.sql.ast.expr.SQLVariantRefExpr; -import com.amazon.opendistroforelasticsearch.sql.domain.ColumnTypeProvider; import com.amazon.opendistroforelasticsearch.sql.domain.Field; import com.amazon.opendistroforelasticsearch.sql.domain.KVValue; import com.amazon.opendistroforelasticsearch.sql.domain.MethodField; @@ -974,25 +973,14 @@ public String getCastScriptStatement(String name, String castType, List * approach will return type of result column as DOUBLE, although there is enough information to understand that * it might be safely treated as INTEGER. */ - public static Schema.Type getScriptFunctionReturnType( - int fieldIndex, MethodField field, ColumnTypeProvider scriptColumnType) { + public static Schema.Type getScriptFunctionReturnType(MethodField field, Schema.Type resolvedType) { Schema.Type returnType; String functionName = ((ScriptMethodField) field).getFunctionName().toLowerCase(); - if (!numberOperators.contains(functionName) && !mathConstants.contains(functionName) - && !trigFunctions.contains(functionName) && !stringOperators.contains(functionName) - && !stringFunctions.contains(functionName) && !binaryOperators.contains(functionName) - && !dateFunctions.contains(functionName) && !conditionalFunctions.contains(functionName) - && !utilityFunctions.contains(functionName)) { - throw new UnsupportedOperationException( - String.format( - "The following method is not supported in Schema: %s", - functionName)); - } if (functionName.equals("cast")) { String castType = ((SQLCastExpr) field.getExpression()).getDataType().getName(); return getCastFunctionReturnType(castType); } else { - returnType = scriptColumnType.get(fieldIndex); + returnType = resolvedType; } return returnType; } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java index ab353bbf3b..6b57eadce2 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java @@ -34,6 +34,7 @@ import org.elasticsearch.common.collect.Tuple; import org.junit.Assert; import org.junit.Before; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -83,7 +84,8 @@ public void testAbsWithIntReturnType() { field.setExpression(invokeExpr); ColumnTypeProvider columnTypeProvider = new ColumnTypeProvider(ESDataType.INTEGER); - final Schema.Type returnType = sqlFunctions.getScriptFunctionReturnType(0, field, columnTypeProvider); + Schema.Type resolvedType = columnTypeProvider.get(0); + final Schema.Type returnType = sqlFunctions.getScriptFunctionReturnType(field, resolvedType); Assert.assertEquals(returnType, Schema.Type.INTEGER); } @@ -100,18 +102,8 @@ public void testCastReturnType() { field.setExpression(castExpr); ColumnTypeProvider columnTypeProvider = new ColumnTypeProvider(ESDataType.INTEGER); - final Schema.Type returnType = sqlFunctions.getScriptFunctionReturnType(0, field, columnTypeProvider); + Schema.Type resolvedType = columnTypeProvider.get(0); + final Schema.Type returnType = sqlFunctions.getScriptFunctionReturnType(field, resolvedType); Assert.assertEquals(returnType, Schema.Type.INTEGER); } - - @Test - public void testNullScriptReturnTypeThrowsException() throws UnsupportedOperationException { - exceptionRule.expect(UnsupportedOperationException.class); - exceptionRule.expectMessage("The following method is not supported in Schema: lo"); - - List params = new ArrayList<>(); - MethodField field = new ScriptMethodField("LO", params, null, null); - ColumnTypeProvider columnTypeProvider = new ColumnTypeProvider(ESDataType.INTEGER); - final Schema.Type type = sqlFunctions.getScriptFunctionReturnType(0, field, columnTypeProvider); - } } \ No newline at end of file From 016795ece0b3c434d9cce0d251e054e03216f5ae Mon Sep 17 00:00:00 2001 From: David Cui Date: Thu, 20 Feb 2020 15:03:28 -0800 Subject: [PATCH 7/8] removed unused variables in SQLFunctionsTest --- .../sql/unittest/utils/SQLFunctionsTest.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java index 6b57eadce2..6d48f5c004 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/SQLFunctionsTest.java @@ -46,18 +46,11 @@ public class SQLFunctionsTest { - private FieldMaker fieldMaker; private SQLFunctions sqlFunctions; @Rule public ExpectedException exceptionRule = ExpectedException.none(); - @Before - public void init() { - fieldMaker = new FieldMaker(); - } - public void initSqlFunctions() { sqlFunctions = new SQLFunctions(); } - @Test public void testAssign() throws SqlParseException { SQLFunctions sqlFunctions = new SQLFunctions(); From 32f1fa5054112320146882bc29cc03fa05c193db Mon Sep 17 00:00:00 2001 From: David Cui Date: Thu, 20 Feb 2020 21:06:18 -0800 Subject: [PATCH 8/8] addressed further comments --- .../opendistroforelasticsearch/sql/utils/SQLFunctions.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java index e8bd017536..09b0679997 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java @@ -974,15 +974,12 @@ public String getCastScriptStatement(String name, String castType, List * it might be safely treated as INTEGER. */ public static Schema.Type getScriptFunctionReturnType(MethodField field, Schema.Type resolvedType) { - Schema.Type returnType; String functionName = ((ScriptMethodField) field).getFunctionName().toLowerCase(); if (functionName.equals("cast")) { String castType = ((SQLCastExpr) field.getExpression()).getDataType().getName(); return getCastFunctionReturnType(castType); - } else { - returnType = resolvedType; } - return returnType; + return resolvedType; } public static Schema.Type getCastFunctionReturnType(String castType) {