diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/AggregationQueryAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/AggregationQueryAction.java index 442d38ec4c..28dc6e3b37 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/AggregationQueryAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/AggregationQueryAction.java @@ -76,24 +76,33 @@ public SqlElasticSearchRequestBuilder explain() throws SqlParseException { if (!groupBy.isEmpty()) { Field field = groupBy.get(0); - //make groupby can reference to field alias lastAgg = getGroupAgg(field, select); - if (lastAgg != null && lastAgg instanceof TermsAggregationBuilder && !(field instanceof MethodField)) { - //if limit size is too small, increasing shard size is required - if (select.getRowCount() < 200) { - ((TermsAggregationBuilder) lastAgg).shardSize(2000); - for (Hint hint : select.getHints()) { - if (hint.getType() == HintType.SHARD_SIZE) { - if (hint.getParams() != null && hint.getParams().length != 0 && hint.getParams()[0] != null) { - ((TermsAggregationBuilder) lastAgg).shardSize((Integer) hint.getParams()[0]); + if (lastAgg instanceof TermsAggregationBuilder) { + + // TODO: Consider removing that condition + // in theory we should be able to apply this for all types of fiels, but + // this change requires too much of related integration tests (e.g. there are comparisons against + // raw javascript dsl, so I'd like to scope the changes as of now to one particular fix for + // scripted functions + + if (!(field instanceof MethodField) || field instanceof ScriptMethodField) { + //if limit size is too small, increasing shard size is required + if (select.getRowCount() < 200) { + ((TermsAggregationBuilder) lastAgg).shardSize(2000); + for (Hint hint : select.getHints()) { + if (hint.getType() == HintType.SHARD_SIZE) { + if (hint.getParams() != null && hint.getParams().length != 0 && hint.getParams()[0] != null) { + ((TermsAggregationBuilder) lastAgg).shardSize((Integer) hint.getParams()[0]); + } } } } - } - if(select.getRowCount()>0) { - ((TermsAggregationBuilder) lastAgg).size(select.getRowCount()); + + if (select.getRowCount() > 0) { + ((TermsAggregationBuilder) lastAgg).size(select.getRowCount()); + } } } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/DateFormatTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/DateFormatTest.java index 4c235b9706..4a253e4319 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/DateFormatTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/DateFormatTest.java @@ -34,6 +34,7 @@ import org.elasticsearch.index.query.RangeQueryBuilder; import org.hamcrest.*; import org.json.JSONObject; +import org.junit.Ignore; import org.junit.Test; import java.util.List; @@ -97,8 +98,9 @@ public void groupByWithDescOrder() throws SqlParseException { "GROUP BY date_format(utc_time, 'dd-MM-YYYY') " + "ORDER BY date_format(utc_time, 'dd-MM-YYYY') DESC"; - JSONObject sort = getAggregationOrder(query); - assertThat(sort.getString("_key"), is("desc")); + JSONObject aggregation = getAggregation(query); + assertThat(aggregation.getInt("size"), is(getSelect(query).getRowCount())); + assertThat(aggregation.getJSONObject("order").getString("_key"), is("desc")); } @Test @@ -108,11 +110,24 @@ public void groupByWithAscOrder() throws SqlParseException { "GROUP BY date_format(utc_time, 'dd-MM-YYYY') " + "ORDER BY date_format(utc_time, 'dd-MM-YYYY')"; - JSONObject sort = getAggregationOrder(query); - assertThat(sort.getString("_key"), is("asc")); + JSONObject aggregation = getAggregation(query); + + assertThat(aggregation.getJSONObject("order").getString("_key"), is("asc")); + } + + @Test + @Ignore("https://github.com/opendistro-for-elasticsearch/sql/issues/158") + public void groupByWithAndAlias() throws SqlParseException { + String query = "SELECT date_format(utc_time, 'dd-MM-YYYY') x, count(*) " + + "FROM kibana_sample_data_logs " + + "GROUP BY x " + + "ORDER BY x"; + + JSONObject aggregation = getAggregation(query); + assertThat(aggregation.getJSONObject("order").getString("_key"), is("asc")); } - public JSONObject getAggregationOrder(String query) throws SqlParseException { + public JSONObject getAggregation(String query) throws SqlParseException { Select select = getSelect(query); Client client = mock(Client.class); @@ -124,7 +139,7 @@ public JSONObject getAggregationOrder(String query) throws SqlParseException { JSONObject aggregations = elasticQuery.getJSONObject("aggregations"); String dateFormatAggregationKey = getScriptAggregationKey(aggregations, "date_format"); - return aggregations.getJSONObject(dateFormatAggregationKey).getJSONObject("terms").getJSONObject("order"); + return aggregations.getJSONObject(dateFormatAggregationKey).getJSONObject("terms"); } public static String getScriptAggregationKey(JSONObject aggregation, String prefix) {