From 8e8310208f14a2b0a541f155151e765741ef14f4 Mon Sep 17 00:00:00 2001 From: penghuo Date: Tue, 10 Mar 2020 16:11:19 -0700 Subject: [PATCH 1/2] Bug fix, return object type for field which has implicit object datatype when describe the table --- .../executor/format/DescribeResultSet.java | 8 +++++- .../sql/esintgtest/MetaDataQueriesIT.java | 28 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/DescribeResultSet.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/DescribeResultSet.java index 66a5e9f8b5..04044f8360 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/DescribeResultSet.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/DescribeResultSet.java @@ -36,6 +36,12 @@ public class DescribeResultSet extends ResultSet { private static final int DEFAULT_NUM_PREC_RADIX = 10; private static final String IS_AUTOINCREMENT = "NO"; + /** + * You are not required to set the field type to object explicitly, as this is the default value. + * https://www.elastic.co/guide/en/elasticsearch/reference/current/object.html + */ + private static final String DEFAULT_OBJECT_DATATYPE = "object"; + private IndexStatement statement; private Object queryResult; @@ -159,7 +165,7 @@ private Map flattenMappingMetaData(Map mappingMe Map metaData = (Map) entry.getValue(); String fullPath = addToPath(currPath, entry.getKey()); - flattenedMapping.put(fullPath, (String) metaData.get("type")); + flattenedMapping.put(fullPath, (String) metaData.getOrDefault("type", DEFAULT_OBJECT_DATATYPE)); if (metaData.containsKey("properties")) { flattenedMapping = flattenMappingMetaData(metaData, fullPath, flattenedMapping); } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/MetaDataQueriesIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/MetaDataQueriesIT.java index cee43fcc06..7f91d04e42 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/MetaDataQueriesIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/MetaDataQueriesIT.java @@ -290,6 +290,34 @@ public void describeSingleIndex() throws IOException { assertThat(row.get(5), not(equalTo(JSONObject.NULL))); } + @Test + public void describeSingleIndexWithObjectFieldShouldPass() throws IOException { + JSONObject response = + executeQuery(String.format("DESCRIBE TABLES LIKE %s", TestsConstants.TEST_INDEX_GAME_OF_THRONES)); + + // Schema for DESCRIBE is filled with a lot of fields that aren't used so only the important + // ones are checked for here + String[] fields = {"TABLE_NAME", "COLUMN_NAME", "TYPE_NAME"}; + checkContainsColumns(getSchema(response), fields); + + JSONArray dataRows = getDataRows(response); + assertThat(dataRows.length(), greaterThan(0)); + assertThat(dataRows.getJSONArray(0).length(), equalTo(DESCRIBE_FIELD_LENGTH)); + + /* + * Assumed indices of fields in dataRows based on "schema" output for DESCRIBE given above: + * "TABLE_NAME" : 2 + * "COLUMN_NAME" : 3 + * "TYPE_NAME" : 5 + */ + for (int i = 0; i < dataRows.length(); i++) { + JSONArray row = dataRows.getJSONArray(i); + assertThat(row.get(2), equalTo(TestsConstants.TEST_INDEX_GAME_OF_THRONES)); + assertThat(row.get(3), not(equalTo(JSONObject.NULL))); + assertThat(row.get(5), not(equalTo(JSONObject.NULL))); + } + } + @Test public void describeCaseSensitivityCheck() throws IOException { JSONObject response = executeQuery(String.format("describe tables like %s", TestsConstants.TEST_INDEX_ACCOUNT)); From d7c4f2c29fd9aa498ed53109c0d3dd13b5b04467 Mon Sep 17 00:00:00 2001 From: penghuo Date: Thu, 12 Mar 2020 14:11:53 -0700 Subject: [PATCH 2/2] address comments --- .../sql/esintgtest/MetaDataQueriesIT.java | 49 ++++++++++++++----- .../sql/util/MatcherUtils.java | 13 ++++- 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/MetaDataQueriesIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/MetaDataQueriesIT.java index 7f91d04e42..221e306760 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/MetaDataQueriesIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/MetaDataQueriesIT.java @@ -16,14 +16,19 @@ package com.amazon.opendistroforelasticsearch.sql.esintgtest; import org.elasticsearch.client.Request; +import org.hamcrest.Description; +import org.hamcrest.TypeSafeMatcher; import org.json.JSONArray; import org.json.JSONObject; import org.junit.Test; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; +import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_GAME_OF_THRONES; +import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.verifySome; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasItems; @@ -293,7 +298,7 @@ public void describeSingleIndex() throws IOException { @Test public void describeSingleIndexWithObjectFieldShouldPass() throws IOException { JSONObject response = - executeQuery(String.format("DESCRIBE TABLES LIKE %s", TestsConstants.TEST_INDEX_GAME_OF_THRONES)); + executeQuery(String.format("DESCRIBE TABLES LIKE %s", TEST_INDEX_GAME_OF_THRONES)); // Schema for DESCRIBE is filled with a lot of fields that aren't used so only the important // ones are checked for here @@ -304,18 +309,15 @@ public void describeSingleIndexWithObjectFieldShouldPass() throws IOException { assertThat(dataRows.length(), greaterThan(0)); assertThat(dataRows.getJSONArray(0).length(), equalTo(DESCRIBE_FIELD_LENGTH)); - /* - * Assumed indices of fields in dataRows based on "schema" output for DESCRIBE given above: - * "TABLE_NAME" : 2 - * "COLUMN_NAME" : 3 - * "TYPE_NAME" : 5 - */ - for (int i = 0; i < dataRows.length(); i++) { - JSONArray row = dataRows.getJSONArray(i); - assertThat(row.get(2), equalTo(TestsConstants.TEST_INDEX_GAME_OF_THRONES)); - assertThat(row.get(3), not(equalTo(JSONObject.NULL))); - assertThat(row.get(5), not(equalTo(JSONObject.NULL))); - } + verifySome(dataRows, + describeRow(TEST_INDEX_GAME_OF_THRONES, "nickname", "text"), + describeRow(TEST_INDEX_GAME_OF_THRONES, "name", "object"), + describeRow(TEST_INDEX_GAME_OF_THRONES, "name.firstname", "text"), + describeRow(TEST_INDEX_GAME_OF_THRONES, "name.lastname", "text"), + describeRow(TEST_INDEX_GAME_OF_THRONES, "name.ofHerName", "integer"), + describeRow(TEST_INDEX_GAME_OF_THRONES, "name.ofHisName", "integer"), + describeRow(TEST_INDEX_GAME_OF_THRONES, "house", "text"), + describeRow(TEST_INDEX_GAME_OF_THRONES, "gender", "text")); } @Test @@ -402,4 +404,25 @@ private String getClusterName() throws IOException { String response = executeRequest(new Request("GET", "_cluster/health")); return new JSONObject(response).optString("cluster_name", ""); } + + public static TypeSafeMatcher describeRow(Object... expectedObjects) { + return new TypeSafeMatcher() { + @Override + public void describeTo(Description description) { + description.appendText(String.join(",", Arrays.asList(expectedObjects).toString())); + } + + @Override + protected boolean matchesSafely(JSONArray array) { + List actualObjects = new ArrayList<>(); + // TABLE_NAME + actualObjects.add(array.get(2)); + // COLUMN_NAME + actualObjects.add(array.get(3)); + // TYPE_NAME + actualObjects.add(array.get(5)); + return Arrays.asList(expectedObjects).equals(actualObjects); + } + }; + } } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/MatcherUtils.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/MatcherUtils.java index 03a118797a..2f4d526b15 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/MatcherUtils.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/MatcherUtils.java @@ -34,9 +34,9 @@ import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.arrayContainingInAnyOrder; -import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.emptyArray; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.hasItems; import static org.junit.Assert.assertEquals; @@ -148,6 +148,17 @@ public static void verify(JSONArray array, Matcher... matchers) { assertThat(objects, containsInAnyOrder(matchers)); } + @SuppressWarnings("unchecked") + public static void verifySome(JSONArray array, Matcher... matchers) { + List objects = new ArrayList<>(); + array.iterator().forEachRemaining(o -> objects.add((T) o)); + + assertThat(matchers.length, greaterThan(0)); + for (Matcher matcher : matchers) { + assertThat(objects, hasItems(matcher)); + } + } + public static TypeSafeMatcher schema(String expectedName, String expectedAlias, String expectedType) { return new TypeSafeMatcher() { @Override