Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Commit

Permalink
Bug fix, support long type for aggregation (#529)
Browse files Browse the repository at this point in the history
  • Loading branch information
@penghuo
penghuo committed Jun 22, 2020
1 parent 88a9c5d commit dc02438
Show file tree
Hide file tree
Showing 7 changed files with 105 additions and 16 deletions.
35 changes: 35 additions & 0 deletions ...c/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/AggregationExpressionIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,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"));
}
Expand Down
16 changes: 12 additions & 4 deletions ...m/amazon/opendistroforelasticsearch/sql/legacy/executor/format/BindingTupleResultSet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@

package com.amazon.opendistroforelasticsearch.sql.legacy.executor.format;

import static com.amazon.opendistroforelasticsearch.sql.legacy.executor.format.DateFieldFormatter.FORMAT_JDBC;

import com.amazon.opendistroforelasticsearch.sql.legacy.expression.domain.BindingTuple;
import com.amazon.opendistroforelasticsearch.sql.legacy.expression.model.ExprValue;
import com.amazon.opendistroforelasticsearch.sql.legacy.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;
Expand All @@ -32,7 +35,7 @@ public class BindingTupleResultSet extends ResultSet {

public BindingTupleResultSet(List<ColumnNode> columnNodes, List<BindingTuple> bindingTuples) {
this.schema = buildSchema(columnNodes);
this.dataRows = buildDataRows(bindingTuples);
this.dataRows = buildDataRows(columnNodes, bindingTuples);
}

@VisibleForTesting
Expand All @@ -47,12 +50,17 @@ public static Schema buildSchema(List<ColumnNode> columnNodes) {
}

@VisibleForTesting
public static DataRows buildDataRows(List<BindingTuple> bindingTuples) {
public static DataRows buildDataRows(List<ColumnNode> columnNodes, List<BindingTuple> bindingTuples) {
List<DataRows.Row> rowList = bindingTuples.stream().map(tuple -> {
Map<String, ExprValue> bindingMap = tuple.getBindingMap();
Map<String, Object> 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());
Expand Down
2 changes: 1 addition & 1 deletion .../com/amazon/opendistroforelasticsearch/sql/legacy/executor/format/DateFieldFormatter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
6 changes: 5 additions & 1 deletion ...a/com/amazon/opendistroforelasticsearch/sql/legacy/expression/model/ExprValueFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, Object> map) {
Map<String, ExprValue> valueMap = new HashMap<>();
map.forEach((k, v) -> valueMap.put(k, from(v)));
Expand All @@ -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) {
Expand Down
5 changes: 5 additions & 0 deletions .../java/com/amazon/opendistroforelasticsearch/sql/legacy/query/planner/core/ColumnNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.amazon.opendistroforelasticsearch.sql.legacy.executor.format.Schema;
import com.amazon.opendistroforelasticsearch.sql.legacy.expression.core.Expression;
import com.google.common.base.Strings;
import lombok.Builder;
import lombok.Getter;
import lombok.Setter;
Expand All @@ -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;
}
}
52 changes: 42 additions & 10 deletions ...distroforelasticsearch/sql/legacy/unittest/executor/format/BindingTupleResultSetTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@

import com.amazon.opendistroforelasticsearch.sql.legacy.executor.format.BindingTupleResultSet;
import com.amazon.opendistroforelasticsearch.sql.legacy.executor.format.DataRows;
import com.amazon.opendistroforelasticsearch.sql.legacy.executor.format.Schema;
import com.amazon.opendistroforelasticsearch.sql.legacy.expression.domain.BindingTuple;
import com.amazon.opendistroforelasticsearch.sql.legacy.query.planner.core.ColumnNode;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import org.hamcrest.Matcher;
Expand All @@ -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<DataRows.Row> rowContents(Matcher<Map<String, Object>> matcher) {
return featureValueOf("DataRows.Row", matcher, DataRows.Row::getContents);
}

private List<DataRows.Row> row(List<BindingTuple> bindingTupleList) {
return ImmutableList.copyOf(BindingTupleResultSet.buildDataRows(bindingTupleList).iterator());
private List<DataRows.Row> row(List<ColumnNode> columnNodes, List<BindingTuple> bindingTupleList) {
return ImmutableList.copyOf(BindingTupleResultSet.buildDataRows(columnNodes, bindingTupleList).iterator());
}
}
5 changes: 5 additions & 0 deletions ...n/opendistroforelasticsearch/sql/legacy/unittest/expression/model/ExprValueUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit dc02438

Please sign in to comment.