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

add fix to handle functions applied to literal #913

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,15 @@ public void testDateInGroupBy() throws IOException{
rows("2018-08-11"));
}

@Test
public void testDateWithHavingClauseOnly() throws IOException {
dai-chen marked this conversation as resolved.
Show resolved Hide resolved
JSONObject result =
executeQuery(String.format("SELECT (TO_DAYS(DATE('2050-01-01')) - 693961) FROM %s HAVING (COUNT(1) > 0)",TEST_INDEX_BANK) );
verifySchema(result,
schema("(TO_DAYS(DATE('2050-01-01')) - 693961)", null, "long"));
verifyDataRows(result, rows(54787));
}

@Test
public void testAddDate() throws IOException {
JSONObject result =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

package com.amazon.opendistroforelasticsearch.sql.sql;

import static com.amazon.opendistroforelasticsearch.sql.legacy.TestsConstants.TEST_INDEX_BANK;
import static com.amazon.opendistroforelasticsearch.sql.legacy.plugin.RestSqlAction.QUERY_API_ENDPOINT;
import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.rows;
import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.schema;
Expand All @@ -38,6 +39,16 @@ public class MathematicalFunctionIT extends SQLIntegTestCase {
public void init() throws Exception {
super.init();
TestUtils.enableNewQueryEngine(client());
loadIndex(Index.BANK);
}

@Test
public void testPI() throws IOException {
JSONObject result =
executeQuery(String.format("SELECT PI() FROM %s HAVING (COUNT(1) > 0)",TEST_INDEX_BANK) );
verifySchema(result,
schema("PI()", null, "double"));
verifyDataRows(result, rows(3.141592653589793));
}

@Test
Expand Down
4 changes: 4 additions & 0 deletions integ-test/src/test/resources/correctness/bugfixes/899.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
SELECT 'hello' FROM kibana_sample_data_flights HAVING (COUNT(1) > 0)
SELECT UPPER('hello') AS `literal` FROM kibana_sample_data_flights HAVING (COUNT(1) > 0)
SELECT UPPER(UPPER('hello')) AS `literal` FROM kibana_sample_data_flights HAVING (COUNT(1) > 0)
SELECT SUBSTRING(CONCAT(UPPER('hello'), 'world'), 1, 6) AS `literal` FROM kibana_sample_data_flights HAVING (COUNT(1) > 0)
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.amazon.opendistroforelasticsearch.sql.ast.Node;
import com.amazon.opendistroforelasticsearch.sql.ast.expression.AggregateFunction;
import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias;
import com.amazon.opendistroforelasticsearch.sql.ast.expression.Function;
import com.amazon.opendistroforelasticsearch.sql.ast.expression.Literal;
import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression;
import com.amazon.opendistroforelasticsearch.sql.ast.tree.Aggregation;
Expand Down Expand Up @@ -124,17 +125,26 @@ private List<UnresolvedExpression> replaceGroupByItemIfAliasOrOrdinal() {
*/
private Optional<UnresolvedExpression> findNonAggregatedItemInSelect() {
return querySpec.getSelectItems().stream()
.filter(this::isNonLiteral)
.filter(this::isNonAggregatedExpression)
.filter(this::isNonLiteralFunction)
.findFirst();
}

private boolean isAggregatorNotFoundAnywhere() {
return querySpec.getAggregators().isEmpty();
}

private boolean isNonLiteral(UnresolvedExpression expr) {
return !(expr instanceof Literal);
private boolean isNonLiteralFunction(UnresolvedExpression expr) {
// The base case for recursion
if (expr instanceof Literal) {
return false;
}
if (expr instanceof Function) {
List<? extends Node> children = expr.getChild();
return children.stream().anyMatch(child ->
isNonLiteralFunction((UnresolvedExpression) child));
}
return true;
}

private boolean isNonAggregatedExpression(UnresolvedExpression expr) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,27 @@ void can_build_implicit_group_by_for_aggregator_in_having_clause() {
hasGroupByItems(),
hasAggregators(
alias("AVG(age)", aggregate("AVG", qualifiedName("age"))))));

assertThat(
buildAggregation("SELECT PI() FROM test HAVING AVG(age) > 30"),
allOf(
hasGroupByItems(),
hasAggregators(
alias("AVG(age)", aggregate("AVG", qualifiedName("age"))))));

assertThat(
buildAggregation("SELECT ABS(1.5) FROM test HAVING AVG(age) > 30"),
allOf(
hasGroupByItems(),
hasAggregators(
alias("AVG(age)", aggregate("AVG", qualifiedName("age"))))));

assertThat(
buildAggregation("SELECT ABS(ABS(1.5)) FROM test HAVING AVG(age) > 30"),
allOf(
hasGroupByItems(),
hasAggregators(
alias("AVG(age)", aggregate("AVG", qualifiedName("age"))))));
}

@Test
Expand Down