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

Bug fix, support long type for aggregation #529

Merged
merged 1 commit into from
Jun 22, 2020
Merged

Bug fix, support long type for aggregation #529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 35 additions & 0 deletions ...c/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/AggregationExpressionIT.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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