From e014b22649ab34ded2c2098a6802cf3b592f2089 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 8 Oct 2020 14:44:57 -0700 Subject: [PATCH 1/4] Add return statement --- .../sql/legacy/parser/FieldMaker.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java index 5195d48ff6..aef51c5441 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java @@ -347,6 +347,14 @@ public MethodField makeMethodField(String name, List arguments, SQLAggr methodParameters.add(new KVValue(((SQLCastExpr) object).getExpr().toString())); String castType = ((SQLCastExpr) object).getDataType().getName(); String scriptCode = sqlFunctions.getCastScriptStatement(castName, castType, methodParameters); + + // Parameter "first" indicates if return statement is required. Take CAST statement nested in + // aggregate function SUM(CAST...) for example, return statement is required in this case. + // Otherwise DSL with metric aggregation always returns 0 as result. And also the caller + // makeFieldImpl(SQLExpr("SUM...")) does pass first=true to here. + if (first) { + scriptCode += "; return " + castName; + } methodParameters.add(new KVValue(scriptCode)); paramers.add(new KVValue("script", new SQLCharExpr(scriptCode))); } else if (object instanceof SQLAggregateExpr) { From ce729ca4fb66d4a8fc8065ddbafec32af67472bc Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 8 Oct 2020 14:57:31 -0700 Subject: [PATCH 2/4] Add return statement for built-in functions too --- .../sql/legacy/parser/FieldMaker.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java index aef51c5441..2b0f54ec77 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java @@ -332,7 +332,7 @@ public MethodField makeMethodField(String name, List arguments, SQLAggr paramers.add(new KVValue("children", childrenType)); } else if (SQLFunctions.isFunctionTranslatedToScript(methodName)) { //throw new SqlParseException("only support script/nested as inner functions"); - MethodField abc = makeMethodField(methodName, mExpr.getParameters(), null, null, tableAlias, false); + MethodField abc = makeMethodField(methodName, mExpr.getParameters(), null, null, tableAlias, first); paramers.add(new KVValue(abc.getParams().get(0).toString(), new SQLCharExpr(abc.getParams().get(1).toString()))); } else { @@ -350,8 +350,8 @@ public MethodField makeMethodField(String name, List arguments, SQLAggr // Parameter "first" indicates if return statement is required. Take CAST statement nested in // aggregate function SUM(CAST...) for example, return statement is required in this case. - // Otherwise DSL with metric aggregation always returns 0 as result. And also the caller - // makeFieldImpl(SQLExpr("SUM...")) does pass first=true to here. + // Otherwise DSL with metric aggregation always returns 0 as result. And this works also because + // the caller makeFieldImpl(SQLExpr("SUM...")) does pass first=true to here. if (first) { scriptCode += "; return " + castName; } From 76f606f59b764544b2401e5c5202d37a7b45c9ff Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 8 Oct 2020 15:19:15 -0700 Subject: [PATCH 3/4] Revert changes for other built-in functions --- .../sql/legacy/parser/FieldMaker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java index 2b0f54ec77..f56b0126c6 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/FieldMaker.java @@ -332,7 +332,7 @@ public MethodField makeMethodField(String name, List arguments, SQLAggr paramers.add(new KVValue("children", childrenType)); } else if (SQLFunctions.isFunctionTranslatedToScript(methodName)) { //throw new SqlParseException("only support script/nested as inner functions"); - MethodField abc = makeMethodField(methodName, mExpr.getParameters(), null, null, tableAlias, first); + MethodField abc = makeMethodField(methodName, mExpr.getParameters(), null, null, tableAlias, false); paramers.add(new KVValue(abc.getParams().get(0).toString(), new SQLCharExpr(abc.getParams().get(1).toString()))); } else { From c346207e5b4c05a7f580dc552ae4cf757eff1b23 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 8 Oct 2020 15:33:58 -0700 Subject: [PATCH 4/4] Add IT --- .../sql/legacy/AggregationExpressionIT.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/AggregationExpressionIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/AggregationExpressionIT.java index 5f6310fa7f..0888bbb560 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/AggregationExpressionIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/AggregationExpressionIT.java @@ -252,6 +252,16 @@ public void groupByDateWithAliasShouldPass() { rows("2018-06-23 00:00:00.000", 1)); } + @Test + public void aggregateCastStatementShouldNotReturnZero() { + JSONObject response = executeJdbcRequest(String.format( + "SELECT SUM(CAST(male AS INT)) AS male_sum FROM %s", + Index.BANK.getName())); + + verifySchema(response, schema("male_sum", "male_sum", "double")); + verifyDataRows(response, rows(4)); + } + private JSONObject executeJdbcRequest(String query) { return new JSONObject(executeQuery(query, "jdbc")); }