From fb2ed912c8bfe50abed8fba182f2125905220cb4 Mon Sep 17 00:00:00 2001 From: Peng Huo Date: Wed, 17 Jun 2020 09:17:36 -0700 Subject: [PATCH] Bug fix, support long type for aggregation (#522) * Bug fix, support long type for aggregation * change to datetime to JDBC format --- .../format/BindingTupleResultSet.java | 16 ++++-- .../executor/format/DateFieldFormatter.java | 2 +- .../expression/model/ExprValueFactory.java | 6 ++- .../sql/query/planner/core/ColumnNode.java | 5 ++ .../esintgtest/AggregationExpressionIT.java | 35 +++++++++++++ .../format/BindingTupleResultSetTest.java | 52 +++++++++++++++---- .../expression/model/ExprValueUtilsTest.java | 5 ++ 7 files changed, 105 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/BindingTupleResultSet.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/BindingTupleResultSet.java index a028db1ebc..39044b1b0c 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/BindingTupleResultSet.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/BindingTupleResultSet.java @@ -20,11 +20,14 @@ import com.amazon.opendistroforelasticsearch.sql.query.planner.core.ColumnNode; import com.google.common.annotations.VisibleForTesting; +import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.stream.Collectors; +import static com.amazon.opendistroforelasticsearch.sql.executor.format.DateFieldFormatter.FORMAT_JDBC; + /** * The definition of BindingTuple ResultSet. */ @@ -32,7 +35,7 @@ public class BindingTupleResultSet extends ResultSet { public BindingTupleResultSet(List columnNodes, List bindingTuples) { this.schema = buildSchema(columnNodes); - this.dataRows = buildDataRows(bindingTuples); + this.dataRows = buildDataRows(columnNodes, bindingTuples); } @VisibleForTesting @@ -47,12 +50,17 @@ public static Schema buildSchema(List columnNodes) { } @VisibleForTesting - public static DataRows buildDataRows(List bindingTuples) { + public static DataRows buildDataRows(List columnNodes, List bindingTuples) { List rowList = bindingTuples.stream().map(tuple -> { Map bindingMap = tuple.getBindingMap(); Map rowMap = new HashMap<>(); - for (String s : bindingMap.keySet()) { - rowMap.put(s, bindingMap.get(s).value()); + for (ColumnNode column : columnNodes) { + String columnName = column.columnName(); + Object value = bindingMap.get(columnName).value(); + if (column.getType() == Schema.Type.DATE) { + value = DateFormat.getFormattedDate(new Date((Long) value), FORMAT_JDBC); + } + rowMap.put(columnName, value); } return new DataRows.Row(rowMap); }).collect(Collectors.toList()); diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/DateFieldFormatter.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/DateFieldFormatter.java index 30994b089d..8775e53d2e 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/DateFieldFormatter.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/DateFieldFormatter.java @@ -39,7 +39,7 @@ */ public class DateFieldFormatter { private static final Logger LOG = LogManager.getLogger(DateFieldFormatter.class); - private static final String FORMAT_JDBC = "yyyy-MM-dd HH:mm:ss.SSS"; + public static final String FORMAT_JDBC = "yyyy-MM-dd HH:mm:ss.SSS"; private static final String FORMAT_DELIMITER = "\\|\\|"; private static final String FORMAT_DOT_DATE_AND_TIME = "yyyy-MM-dd'T'HH:mm:ss.SSSZ"; diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/model/ExprValueFactory.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/model/ExprValueFactory.java index 7254e4f3bf..87d9cbb241 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/model/ExprValueFactory.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/model/ExprValueFactory.java @@ -41,6 +41,10 @@ public static ExprValue stringValue(String value) { return new ExprStringValue(value); } + public static ExprValue longValue(Long value) { + return new ExprLongValue(value); + } + public static ExprValue tupleValue(Map map) { Map valueMap = new HashMap<>(); map.forEach((k, v) -> valueMap.put(k, from(v))); @@ -61,7 +65,7 @@ public static ExprValue from(Object o) { } else if (o instanceof Integer) { return integerValue((Integer) o); } else if (o instanceof Long) { - return integerValue(((Long) o).intValue()); + return longValue(((Long) o)); } else if (o instanceof Boolean) { return booleanValue((Boolean) o); } else if (o instanceof Double) { diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/planner/core/ColumnNode.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/planner/core/ColumnNode.java index 4e5755aab1..911a695efd 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/planner/core/ColumnNode.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/planner/core/ColumnNode.java @@ -17,6 +17,7 @@ import com.amazon.opendistroforelasticsearch.sql.executor.format.Schema; import com.amazon.opendistroforelasticsearch.sql.expression.core.Expression; +import com.google.common.base.Strings; import lombok.Builder; import lombok.Getter; import lombok.Setter; @@ -34,4 +35,8 @@ public class ColumnNode { private String alias; private Schema.Type type; private Expression expr; + + public String columnName() { + return Strings.isNullOrEmpty(alias) ? name : alias; + } } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/AggregationExpressionIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/AggregationExpressionIT.java index 7d345cbf56..9a52341105 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/AggregationExpressionIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/AggregationExpressionIT.java @@ -216,6 +216,41 @@ public void logWithAddLiteralOnGroupKeyAndMaxSubtractLiteralShouldPass() { rows("m", 3.4339872044851463d, 49333)); } + /** + * The date is in JDBC format. + */ + @Test + public void groupByDateShouldPass() { + JSONObject response = executeJdbcRequest(String.format( + "SELECT birthdate, count(*) as count " + + "FROM %s " + + "WHERE age < 30 " + + "GROUP BY birthdate ", + Index.BANK.getName())); + + verifySchema(response, + schema("birthdate", null, "date"), + schema("count", "count", "integer")); + verifyDataRows(response, + rows("2018-06-23 00:00:00.000", 1)); + } + + @Test + public void groupByDateWithAliasShouldPass() { + JSONObject response = executeJdbcRequest(String.format( + "SELECT birthdate as birth, count(*) as count " + + "FROM %s " + + "WHERE age < 30 " + + "GROUP BY birthdate ", + Index.BANK.getName())); + + verifySchema(response, + schema("birth", "birth", "date"), + schema("count", "count", "integer")); + verifyDataRows(response, + rows("2018-06-23 00:00:00.000", 1)); + } + private JSONObject executeJdbcRequest(String query) { return new JSONObject(executeQuery(query, "jdbc")); } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/executor/format/BindingTupleResultSetTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/executor/format/BindingTupleResultSetTest.java index cf51d5cd44..fc9b33f72b 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/executor/format/BindingTupleResultSetTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/executor/format/BindingTupleResultSetTest.java @@ -17,7 +17,9 @@ import com.amazon.opendistroforelasticsearch.sql.executor.format.BindingTupleResultSet; import com.amazon.opendistroforelasticsearch.sql.executor.format.DataRows; +import com.amazon.opendistroforelasticsearch.sql.executor.format.Schema; import com.amazon.opendistroforelasticsearch.sql.expression.domain.BindingTuple; +import com.amazon.opendistroforelasticsearch.sql.query.planner.core.ColumnNode; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import org.hamcrest.Matcher; @@ -37,21 +39,51 @@ public class BindingTupleResultSetTest { @Test public void buildDataRowsFromBindingTupleShouldPass() { - assertThat(row(Arrays.asList(BindingTuple.from(ImmutableMap.of("age", 31, "gender", "m")), - BindingTuple.from(ImmutableMap.of("age", 31, "gender", "f")), - BindingTuple.from(ImmutableMap.of("age", 39, "gender", "m")), - BindingTuple.from(ImmutableMap.of("age", 39, "gender", "f")))), - containsInAnyOrder(rowContents(allOf(hasEntry("age", 31), hasEntry("gender", (Object) "m"))), - rowContents(allOf(hasEntry("age", 31), hasEntry("gender", (Object) "f"))), - rowContents(allOf(hasEntry("age", 39), hasEntry("gender", (Object) "m"))), - rowContents(allOf(hasEntry("age", 39), hasEntry("gender", (Object) "f"))))); + assertThat(row( + Arrays.asList( + ColumnNode.builder().name("age").type(Schema.Type.INTEGER).build(), + ColumnNode.builder().name("gender").type(Schema.Type.TEXT).build()), + Arrays.asList(BindingTuple.from(ImmutableMap.of("age", 31, "gender", "m")), + BindingTuple.from(ImmutableMap.of("age", 31, "gender", "f")), + BindingTuple.from(ImmutableMap.of("age", 39, "gender", "m")), + BindingTuple.from(ImmutableMap.of("age", 39, "gender", "f")))), + containsInAnyOrder(rowContents(allOf(hasEntry("age", 31), hasEntry("gender", (Object) "m"))), + rowContents(allOf(hasEntry("age", 31), hasEntry("gender", (Object) "f"))), + rowContents(allOf(hasEntry("age", 39), hasEntry("gender", (Object) "m"))), + rowContents(allOf(hasEntry("age", 39), hasEntry("gender", (Object) "f"))))); + } + + @Test + public void buildDataRowsFromBindingTupleIncludeLongValueShouldPass() { + assertThat(row( + Arrays.asList( + ColumnNode.builder().name("longValue").type(Schema.Type.LONG).build(), + ColumnNode.builder().name("gender").type(Schema.Type.TEXT).build()), + Arrays.asList( + BindingTuple.from(ImmutableMap.of("longValue", Long.MAX_VALUE, "gender", "m")), + BindingTuple.from(ImmutableMap.of("longValue", Long.MIN_VALUE, "gender", "f")))), + containsInAnyOrder( + rowContents(allOf(hasEntry("longValue", Long.MAX_VALUE), hasEntry("gender", (Object) "m"))), + rowContents(allOf(hasEntry("longValue", Long.MIN_VALUE), hasEntry("gender", (Object) "f"))))); + } + + @Test + public void buildDataRowsFromBindingTupleIncludeDateShouldPass() { + assertThat(row( + Arrays.asList( + ColumnNode.builder().alias("dateValue").type(Schema.Type.DATE).build(), + ColumnNode.builder().alias("gender").type(Schema.Type.TEXT).build()), + Arrays.asList( + BindingTuple.from(ImmutableMap.of("dateValue", 1529712000000L, "gender", "m")))), + containsInAnyOrder( + rowContents(allOf(hasEntry("dateValue", "2018-06-23 00:00:00.000"), hasEntry("gender", (Object) "m"))))); } private static Matcher rowContents(Matcher> matcher) { return featureValueOf("DataRows.Row", matcher, DataRows.Row::getContents); } - private List row(List bindingTupleList) { - return ImmutableList.copyOf(BindingTupleResultSet.buildDataRows(bindingTupleList).iterator()); + private List row(List columnNodes, List bindingTupleList) { + return ImmutableList.copyOf(BindingTupleResultSet.buildDataRows(columnNodes, bindingTupleList).iterator()); } } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/expression/model/ExprValueUtilsTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/expression/model/ExprValueUtilsTest.java index d350e8281c..7456f48e99 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/expression/model/ExprValueUtilsTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/expression/model/ExprValueUtilsTest.java @@ -46,6 +46,11 @@ public void getIntegerWithDoubleExprValueShouldPass() { assertThat(ExprValueUtils.getIntegerValue(ExprValueFactory.doubleValue(1d)), equalTo(1)); } + @Test + public void getLongValueFromLongExprValueShouldPass() { + assertThat(ExprValueUtils.getLongValue(ExprValueFactory.from(1L)), equalTo(1L)); + } + @Test public void getIntegerValueFromStringExprValueShouldThrowException() { exceptionRule.expect(IllegalStateException.class);