From 760b65ad268f9b362a9513e224b238e4d067dedd Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Mon, 10 Jun 2019 23:19:16 +0200 Subject: [PATCH 01/14] bugfix for https://github.com/elastic/elasticsearch/issues/42041 --- .../xpack/sql/expression/Expression.java | 3 - .../xpack/sql/expression/NamedExpression.java | 4 +- .../expression/function/aggregate/Count.java | 8 +- .../xpack/sql/planner/QueryTranslator.java | 19 +++-- .../xpack/sql/optimizer/OptimizerTests.java | 2 +- .../sql/planner/QueryTranslatorTests.java | 79 +++++++++++++++++++ 6 files changed, 97 insertions(+), 18 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java index 616c337e64c9a..2dde7e5f97d61 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java @@ -126,9 +126,6 @@ public boolean resolved() { public abstract DataType dataType(); - @Override - public abstract int hashCode(); - @Override public String toString() { return nodeName() + "[" + propertiesToString(false) + "]"; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java index 2331034c10140..417145237ab9f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java @@ -67,7 +67,7 @@ protected Pipe makePipe() { @Override public int hashCode() { - return Objects.hash(id, name, synthetic); + return Objects.hash(super.hashCode(), name, synthetic); } @Override @@ -96,4 +96,4 @@ public boolean equals(Object obj) { public String toString() { return super.toString() + "#" + id(); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Count.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Count.java index 236cf105a4c80..1da2eeb0277ad 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Count.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Count.java @@ -78,11 +78,15 @@ public AggregateFunctionAttribute toAttribute() { @Override public boolean equals(Object obj) { - if (false == super.equals(obj)) { + if (this == obj) { + return true; + } + if (obj == null || obj.getClass() != getClass()) { return false; } Count other = (Count) obj; - return Objects.equals(other.distinct(), distinct()); + return Objects.equals(other.distinct(), distinct()) + && Objects.equals(field(), other.field()); } @Override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java index 25ff9e9879797..6614d98cffbba 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java @@ -12,7 +12,6 @@ import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; import org.elasticsearch.xpack.sql.expression.Attribute; import org.elasticsearch.xpack.sql.expression.Expression; -import org.elasticsearch.xpack.sql.expression.ExpressionId; import org.elasticsearch.xpack.sql.expression.Expressions; import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.expression.Foldables; @@ -210,14 +209,14 @@ static LeafAgg toAgg(String id, Function f) { } static class GroupingContext { - final Map groupMap; + final Map groupMap; final GroupByKey tail; - GroupingContext(Map groupMap) { + GroupingContext(Map groupMap) { this.groupMap = groupMap; GroupByKey lastAgg = null; - for (Entry entry : groupMap.entrySet()) { + for (Entry entry : groupMap.entrySet()) { lastAgg = entry.getValue(); } @@ -232,7 +231,7 @@ GroupByKey groupFor(Expression exp) { GroupByKey matchingGroup = null; // group found - finding the dedicated agg if (f.field() instanceof NamedExpression) { - matchingGroup = groupMap.get(((NamedExpression) f.field()).id()); + matchingGroup = groupMap.get(f.field()); } // return matching group or the tail (last group) return matchingGroup != null ? matchingGroup : tail; @@ -242,7 +241,7 @@ GroupByKey groupFor(Expression exp) { } } if (exp instanceof NamedExpression) { - return groupMap.get(((NamedExpression) exp).id()); + return groupMap.get(exp); } throw new SqlIllegalArgumentException("Don't know how to find group for expression {}", exp); } @@ -261,18 +260,18 @@ static GroupingContext groupBy(List groupings) { return null; } - Map aggMap = new LinkedHashMap<>(); + Map aggMap = new LinkedHashMap<>(); for (Expression exp : groupings) { GroupByKey key = null; - ExpressionId id; + NamedExpression id; String aggId; if (exp instanceof NamedExpression) { NamedExpression ne = (NamedExpression) exp; - id = ne.id(); - aggId = id.toString(); + id = ne; + aggId = ne.id().toString(); // change analyzed to non non-analyzed attributes if (exp instanceof FieldAttribute) { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java index 68dbf0bbc9279..d2eeb98a25bda 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java @@ -236,7 +236,7 @@ public void testDuplicateFunctions() { assertTrue(result instanceof Project); List projections = ((Project) result).projections(); assertEquals(2, projections.size()); - assertSame(projections.get(0), projections.get(1)); + assertEquals(projections.get(0), projections.get(1)); } public void testCombineProjections() { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index df500411926ae..d5d46e7fe312d 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -1132,6 +1132,85 @@ public void testAllCountVariantsWithHavingGenerateCorrectAggregations() { + "\"gap_policy\":\"skip\"}}}}}")); } + public void testGroupSelection() { + { + PhysicalPlan p = optimizeAndPlan("SELECT EXTRACT(MINUTE FROM CONVERT(date, SQL_TIMESTAMP)) x FROM test GROUP BY x"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("x", p.output().get(0).qualifiedName()); + assertEquals(DataType.INTEGER, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeChrono(" + + "InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"MINUTE_OF_HOUR\"}}," + + "\"missing_bucket\":true,\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") + ); + } + { + PhysicalPlan p = optimizeAndPlan("SELECT EXTRACT(HOUR FROM CONVERT(date, SQL_TIMESTAMP)) FROM test GROUP BY " + + "EXTRACT(HOUR FROM CONVERT(date, SQL_TIMESTAMP))"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("EXTRACT(HOUR FROM CONVERT(date, SQL_TIMESTAMP))", p.output().get(0).qualifiedName()); + assertEquals(DataType.INTEGER, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeChrono(" + + "InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"HOUR_OF_DAY\"}},\"missing_bucket\":true," + + "\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") + ); + } + { + PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY 1"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("PI() * int", p.output().get(0).qualifiedName()); + assertEquals(DataType.DOUBLE, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.mul(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))\"," + + "\"lang\":\"painless\",\"params\":{\"v0\":3.141592653589793,\"v1\":\"int\"}}," + + "\"missing_bucket\":true,\"value_type\":\"double\",\"order\":\"asc\"}}}]}}}") + ); + } + { + PhysicalPlan p = optimizeAndPlan("SELECT date + 1 * INTERVAL '1' DAY FROM test GROUP BY 1"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("date + 1 * INTERVAL '1' DAY", p.output().get(0).qualifiedName()); + assertEquals(DataType.DATETIME, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.add(InternalSqlScriptUtils.docValue(doc,params.v0)," + + "InternalSqlScriptUtils.intervalDayTime(params.v1,params.v2))\"," + + "\"lang\":\"painless\",\"params\":{\"v0\":\"date\",\"v1\":\"PT24H\",\"v2\":\"INTERVAL_DAY\"}}," + + "\"missing_bucket\":true,\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") + ); + } + { + PhysicalPlan p = optimizeAndPlan("select (3 < int) as multi_language, count(*) from test group by multi_language"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(2, p.output().size()); + assertEquals("multi_language", p.output().get(0).qualifiedName()); + assertEquals(DataType.BOOLEAN, p.output().get(0).dataType()); + assertEquals("count(*)", p.output().get(1).qualifiedName()); + assertEquals(DataType.LONG, p.output().get(1).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.gt(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1)\"," + + "\"lang\":\"painless\",\"params\":{\"v0\":\"int\",\"v1\":3}}," + + "\"missing_bucket\":true,\"value_type\":\"boolean\",\"order\":\"asc\"}}}]}}}") + ); + } + } + public void testTopHitsAggregationWithOneArg() { { PhysicalPlan p = optimizeAndPlan("SELECT FIRST(keyword) FROM test"); From 7ba3ad51b8a02f3828d29562796ef86c0402f01d Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Sat, 3 Aug 2019 11:39:54 +0200 Subject: [PATCH 02/14] added integration tests added test cases and refactored into multiple unit tests --- .../sql/qa/src/main/resources/agg.csv-spec | 141 +++++++++++++ .../sql/qa/src/main/resources/agg.sql-spec | 4 + .../sql/planner/QueryTranslatorTests.java | 196 ++++++++++++++++-- 3 files changed, 321 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec index e62fda5478b86..7bf8d8d3683d0 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec @@ -246,7 +246,148 @@ TRUNCATE(YEAR("birth_date"), -2) null 1900 ; +// Fails for H2 +groupByCastScalarWithNumericRef +SELECT CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) FROM test_emp GROUP BY 1 ORDER BY 1 NULLS FIRST; +CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT):l +------------------------------------------------------ +null +1952 +1953 +1954 +1955 +1956 +1957 +1958 +1959 +1960 +1961 +1962 +1963 +1964 +1965 +; + +groupByConvertScalar +SELECT CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) FROM test_emp GROUP BY CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) ORDER BY CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) NULLS FIRST; + + +CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT):l +----------------------------------------------------------- +null +1952 +1953 +1954 +1955 +1956 +1957 +1958 +1959 +1960 +1961 +1962 +1963 +1964 +1965 +; + + +groupByConvertScalarWithAlias +SELECT CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) as "convert" FROM test_emp GROUP BY "convert" ORDER BY "convert" NULLS FIRST; + +convert:l +--------- +null +1952 +1953 +1954 +1955 +1956 +1957 +1958 +1959 +1960 +1961 +1962 +1963 +1964 +1965 +; + +groupByConvertScalarWithNumericRef +SELECT CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) FROM test_emp GROUP BY 1 ORDER BY 1 NULLS FIRST; + +CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT):l +----------------------------------------------------------- +null +1952 +1953 +1954 +1955 +1956 +1957 +1958 +1959 +1960 +1961 +1962 +1963 +1964 +1965 +; + +// AwaitsFix https://github.com/elastic/elasticsearch/issues/36074 +//groupByConstantScalar +//SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no LIMIT 10; + +//PI() * emp_no:d +//--------------- +//31419.0681285515 +//31422.2097212051 +//31425.3513138587 +//31428.4929065123 +//31431.6344991659 +//31434.7760918195 +//31437.9176844731 +//31441.0592771266 +//31444.2008697802 +//31447.3424624338 +//; + +groupByConstantScalarWithAlias +SELECT PI() * emp_no AS "value" FROM test_emp GROUP BY value ORDER BY value LIMIT 10; + +value:d +------- +31419.0681285515 +31422.2097212051 +31425.3513138587 +31428.4929065123 +31431.6344991659 +31434.7760918195 +31437.9176844731 +31441.0592771266 +31444.2008697802 +31447.3424624338 +; + +groupByConstantScalarWithNumericRef +SELECT PI() * emp_no FROM test_emp GROUP BY 1 ORDER BY 1 LIMIT 10; + +PI() * emp_no:d +--------------- +31419.0681285515 +31422.2097212051 +31425.3513138587 +31428.4929065123 +31431.6344991659 +31434.7760918195 +31437.9176844731 +31441.0592771266 +31444.2008697802 +31447.3424624338 +; // // Grouping functions // diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.sql-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.sql-spec index 102f884c1b90c..cdd5c2215b6e5 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg.sql-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg.sql-spec @@ -51,6 +51,10 @@ groupByMulScalar SELECT emp_no * 2 AS e FROM test_emp GROUP BY e ORDER BY e; groupByModScalar SELECT (emp_no % 3) + 1 AS e FROM test_emp GROUP BY e ORDER BY e; +groupByCastScalar +SELECT CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) FROM test_emp GROUP BY CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) ORDER BY CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) NULLS FIRST; +groupByCastScalarWithAlias +SELECT CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) as "cast" FROM test_emp GROUP BY "cast" ORDER BY "cast" NULLS FIRST; // group by nested functions with no alias groupByTruncate diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index d5d46e7fe312d..629d888791a92 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -1132,20 +1132,73 @@ public void testAllCountVariantsWithHavingGenerateCorrectAggregations() { + "\"gap_policy\":\"skip\"}}}}}")); } - public void testGroupSelection() { + public void testGroupByCastScalar() { + PhysicalPlan p = optimizeAndPlan("SELECT CAST(ABS(EXTRACT(YEAR FROM date)) AS BIGINT) FROM test " + + "GROUP BY CAST(ABS(EXTRACT(YEAR FROM date)) AS BIGINT) ORDER BY CAST(ABS(EXTRACT(YEAR FROM date)) AS BIGINT) NULLS FIRST"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("CAST(ABS(EXTRACT(YEAR FROM date)) AS BIGINT)", p.output().get(0).qualifiedName()); + assertEquals(DataType.LONG, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" + + "(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," + + "\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") + ); + } + + public void testGroupByCastScalarWithAlias() { + PhysicalPlan p = optimizeAndPlan("SELECT CAST(ABS(EXTRACT(YEAR FROM date)) AS BIGINT) as \"cast\" FROM test " + + "GROUP BY \"cast\" ORDER BY \"cast\" NULLS FIRST"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("cast", p.output().get(0).qualifiedName()); + assertEquals(DataType.LONG, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" + + "(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," + + "\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") + ); + } + + public void testGroupByCastScalarWithNumericRef() { + PhysicalPlan p = optimizeAndPlan("SELECT CAST(ABS(EXTRACT(YEAR FROM date)) AS BIGINT) FROM test " + + "GROUP BY 1 ORDER BY 1 NULLS FIRST"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("CAST(ABS(EXTRACT(YEAR FROM date)) AS BIGINT)", p.output().get(0).qualifiedName()); + assertEquals(DataType.LONG, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" + + "(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," + + "\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") + ); + } + + public void testGroupByConvertScalar() { { - PhysicalPlan p = optimizeAndPlan("SELECT EXTRACT(MINUTE FROM CONVERT(date, SQL_TIMESTAMP)) x FROM test GROUP BY x"); + PhysicalPlan p = optimizeAndPlan("SELECT CONVERT(ABS(EXTRACT(YEAR FROM date)), SQL_BIGINT) FROM test " + + "GROUP BY CONVERT(ABS(EXTRACT(YEAR FROM date)), SQL_BIGINT) ORDER BY CONVERT(ABS(EXTRACT(YEAR FROM date)), SQL_BIGINT) " + + "NULLS FIRST"); assertEquals(EsQueryExec.class, p.getClass()); assertEquals(1, p.output().size()); - assertEquals("x", p.output().get(0).qualifiedName()); - assertEquals(DataType.INTEGER, p.output().get(0).dataType()); + assertEquals("CONVERT(ABS(EXTRACT(YEAR FROM date)), SQL_BIGINT)", p.output().get(0).qualifiedName()); + assertEquals(DataType.LONG, p.output().get(0).dataType()); assertThat( ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() .replaceAll("\\s+", ""), - endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeChrono(" + - "InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)\",\"lang\":\"painless\"," + - "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"MINUTE_OF_HOUR\"}}," + - "\"missing_bucket\":true,\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") + endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" + + "(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," + + "\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") ); } { @@ -1164,35 +1217,91 @@ public void testGroupSelection() { "\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") ); } + } + + public void testGroupByConvertScalarWithAlias() { { - PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY 1"); + PhysicalPlan p = optimizeAndPlan("SELECT CONVERT(ABS(EXTRACT(YEAR FROM date)), SQL_BIGINT) as \"convert\" FROM test " + + "GROUP BY \"convert\" ORDER BY \"convert\" NULLS FIRST"); assertEquals(EsQueryExec.class, p.getClass()); assertEquals(1, p.output().size()); - assertEquals("PI() * int", p.output().get(0).qualifiedName()); - assertEquals(DataType.DOUBLE, p.output().get(0).dataType()); + assertEquals("convert", p.output().get(0).qualifiedName()); + assertEquals(DataType.LONG, p.output().get(0).dataType()); assertThat( ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() .replaceAll("\\s+", ""), - endsWith("{\"source\":\"InternalSqlScriptUtils.mul(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))\"," + - "\"lang\":\"painless\",\"params\":{\"v0\":3.141592653589793,\"v1\":\"int\"}}," + - "\"missing_bucket\":true,\"value_type\":\"double\",\"order\":\"asc\"}}}]}}}") + endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" + + "(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," + + "\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") ); } { - PhysicalPlan p = optimizeAndPlan("SELECT date + 1 * INTERVAL '1' DAY FROM test GROUP BY 1"); + PhysicalPlan p = optimizeAndPlan("SELECT EXTRACT(MINUTE FROM CONVERT(date, SQL_TIMESTAMP)) x FROM test GROUP BY x"); assertEquals(EsQueryExec.class, p.getClass()); assertEquals(1, p.output().size()); - assertEquals("date + 1 * INTERVAL '1' DAY", p.output().get(0).qualifiedName()); - assertEquals(DataType.DATETIME, p.output().get(0).dataType()); + assertEquals("x", p.output().get(0).qualifiedName()); + assertEquals(DataType.INTEGER, p.output().get(0).dataType()); assertThat( ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() .replaceAll("\\s+", ""), - endsWith("{\"source\":\"InternalSqlScriptUtils.add(InternalSqlScriptUtils.docValue(doc,params.v0)," + - "InternalSqlScriptUtils.intervalDayTime(params.v1,params.v2))\"," + - "\"lang\":\"painless\",\"params\":{\"v0\":\"date\",\"v1\":\"PT24H\",\"v2\":\"INTERVAL_DAY\"}}," + + endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeChrono(" + + "InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"MINUTE_OF_HOUR\"}}," + "\"missing_bucket\":true,\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") ); } + } + + public void testGroupByConvertScalarWithNumericRef() { + PhysicalPlan p = optimizeAndPlan("SELECT CONVERT(ABS(EXTRACT(YEAR FROM date)), SQL_BIGINT) FROM test " + + "GROUP BY 1 ORDER BY 1 NULLS FIRST"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("CONVERT(ABS(EXTRACT(YEAR FROM date)), SQL_BIGINT)", p.output().get(0).qualifiedName()); + assertEquals(DataType.LONG, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" + + "(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," + + "\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") + ); + } + + public void testGroupByConstantScalar() { + // FIXME: same query with order by PI() * int still fails + PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY PI() * int LIMIT 10"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("PI() * int", p.output().get(0).qualifiedName()); + assertEquals(DataType.DOUBLE, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"script\":{\"source\":\"InternalSqlScriptUtils.mul(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))\"," + + "\"lang\":\"painless\",\"params\":{\"v0\":3.141592653589793,\"v1\":\"int\"}},\"missing_bucket\":true," + + "\"value_type\":\"double\",\"order\":\"asc\"}}}]}}}") + ); + } + + + public void testGroupByConstantScalarWithAlias() { + { + PhysicalPlan p = optimizeAndPlan("SELECT PI() * int AS \"value\" FROM test GROUP BY \"value\" ORDER BY \"value\" LIMIT 10"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("value", p.output().get(0).qualifiedName()); + assertEquals(DataType.DOUBLE, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"script\":{\"source\":\"InternalSqlScriptUtils.mul(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))" + + "\",\"lang\":\"painless\",\"params\":{\"v0\":3.141592653589793,\"v1\":\"int\"}},\"missing_bucket\":true," + + "\"value_type\":\"double\",\"order\":\"asc\"}}}]}}}") + ); + } { PhysicalPlan p = optimizeAndPlan("select (3 < int) as multi_language, count(*) from test group by multi_language"); assertEquals(EsQueryExec.class, p.getClass()); @@ -1211,6 +1320,53 @@ public void testGroupSelection() { } } + + public void testGroupByConstantScalarWithNumericRef() { + { + PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY 1 ORDER BY 1 LIMIT 10"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("PI() * int", p.output().get(0).qualifiedName()); + assertEquals(DataType.DOUBLE, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"script\":{\"source\":\"InternalSqlScriptUtils.mul(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))" + + "\",\"lang\":\"painless\",\"params\":{\"v0\":3.141592653589793,\"v1\":\"int\"}},\"missing_bucket\":true," + + "\"value_type\":\"double\",\"order\":\"asc\"}}}]}}}") + ); + } + { + PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY 1"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("PI() * int", p.output().get(0).qualifiedName()); + assertEquals(DataType.DOUBLE, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.mul(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))\"," + + "\"lang\":\"painless\",\"params\":{\"v0\":3.141592653589793,\"v1\":\"int\"}}," + + "\"missing_bucket\":true,\"value_type\":\"double\",\"order\":\"asc\"}}}]}}}") + ); + } + { + PhysicalPlan p = optimizeAndPlan("SELECT date + 1 * INTERVAL '1' DAY FROM test GROUP BY 1"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("date + 1 * INTERVAL '1' DAY", p.output().get(0).qualifiedName()); + assertEquals(DataType.DATETIME, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString() + .replaceAll("\\s+", ""), + endsWith("{\"source\":\"InternalSqlScriptUtils.add(InternalSqlScriptUtils.docValue(doc,params.v0)," + + "InternalSqlScriptUtils.intervalDayTime(params.v1,params.v2))\"," + + "\"lang\":\"painless\",\"params\":{\"v0\":\"date\",\"v1\":\"PT24H\",\"v2\":\"INTERVAL_DAY\"}}," + + "\"missing_bucket\":true,\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}") + ); + } + } + public void testTopHitsAggregationWithOneArg() { { PhysicalPlan p = optimizeAndPlan("SELECT FIRST(keyword) FROM test"); From d3d001072ae7ab116f2f5c8be31cb3fe13fa9201 Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Sat, 3 Aug 2019 18:21:16 +0200 Subject: [PATCH 03/14] fix for for query with "group by" and "order by" a constant scalar unified semantic of equals to that of hashCode for NamedExpression changed AttributeMapTests according to the new semantic changed equals semantic of Failure --- .../sql/qa/src/main/resources/agg.csv-spec | 33 +++++++++---------- .../xpack/sql/analysis/analyzer/Analyzer.java | 3 +- .../analyzer/VerificationException.java | 2 +- .../xpack/sql/analysis/analyzer/Verifier.java | 16 ++++----- .../xpack/sql/expression/AttributeMap.java | 6 ++-- .../xpack/sql/expression/NamedExpression.java | 1 - .../function/FunctionAttribute.java | 4 +-- .../sql/expression/gen/script/Param.java | 12 +++++++ .../sql/expression/gen/script/Params.java | 13 +++++++- .../sql/expression/AttributeMapTests.java | 6 ++-- .../sql/planner/QueryTranslatorTests.java | 3 +- 11 files changed, 60 insertions(+), 39 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec index 7bf8d8d3683d0..7c4063852e349 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec @@ -337,23 +337,22 @@ null 1965 ; -// AwaitsFix https://github.com/elastic/elasticsearch/issues/36074 -//groupByConstantScalar -//SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no LIMIT 10; - -//PI() * emp_no:d -//--------------- -//31419.0681285515 -//31422.2097212051 -//31425.3513138587 -//31428.4929065123 -//31431.6344991659 -//31434.7760918195 -//31437.9176844731 -//31441.0592771266 -//31444.2008697802 -//31447.3424624338 -//; +groupByConstantScalar +SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no LIMIT 10; + +PI() * emp_no:d +--------------- +31419.0681285515 +31422.2097212051 +31425.3513138587 +31428.4929065123 +31431.6344991659 +31434.7760918195 +31437.9176844731 +31441.0592771266 +31444.2008697802 +31447.3424624338 +; groupByConstantScalarWithAlias SELECT PI() * emp_no AS "value" FROM test_emp GROUP BY value ORDER BY value LIMIT 10; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index 5fdd1f9124d7b..415b536a6ac93 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -609,8 +609,9 @@ protected LogicalPlan rule(LogicalPlan plan) { .map(or -> tryResolveExpression(or, o.child())) .collect(toList()); - AttributeSet resolvedRefs = Expressions.references(maybeResolved.stream() + AttributeSet resolvedRefs = new AttributeSet(maybeResolved.stream() .filter(Expression::resolved) + .map(exp->Expressions.attribute(exp.child())) .collect(toList())); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java index e7aa0b36f1482..14281a628b163 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java @@ -27,7 +27,7 @@ protected VerificationException(Collection sources) { public String getMessage() { return failures.stream() .map(f -> { - Location l = f.source().source().source(); + Location l = f.node().source().source(); return "line " + l.getLineNumber() + ":" + l.getColumnNumber() + ": " + f.message(); }) .collect(Collectors.joining(StringUtils.NEW_LINE, "Found " + failures.size() + " problem(s)\n", StringUtils.EMPTY)); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index 31636a30c68cb..e73b235c238dc 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -84,16 +84,16 @@ public Verifier(Metrics metrics) { } static class Failure { - private final Node source; + private final Node node; private final String message; - Failure(Node source, String message) { - this.source = source; + Failure(Node node, String message) { + this.node = node; this.message = message; } - Node source() { - return source; + Node node() { + return node; } String message() { @@ -102,7 +102,7 @@ String message() { @Override public int hashCode() { - return source.hashCode(); + return Objects.hash(node.source()); } @Override @@ -116,7 +116,7 @@ public boolean equals(Object obj) { } Verifier.Failure other = (Verifier.Failure) obj; - return Objects.equals(source, other.source); + return Objects.equals(node.source(), other.node.source()); } @Override @@ -131,7 +131,7 @@ private static Failure fail(Node source, String message, Object... args) { public Map, String> verifyFailures(LogicalPlan plan) { Collection failures = verify(plan); - return failures.stream().collect(toMap(Failure::source, Failure::message)); + return failures.stream().collect(toMap(Failure::node, Failure::message)); } Collection verify(LogicalPlan plan) { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/AttributeMap.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/AttributeMap.java index 73092f2ed72da..bb8d373f98bfd 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/AttributeMap.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/AttributeMap.java @@ -32,14 +32,14 @@ static class AttributeWrapper { @Override public int hashCode() { - return attr.semanticHash(); + return attr.hashCode(); } @Override public boolean equals(Object obj) { if (obj instanceof AttributeWrapper) { AttributeWrapper aw = (AttributeWrapper) obj; - return attr.semanticEquals(aw.attr); + return attr.equals(aw.attr); } return false; @@ -368,4 +368,4 @@ public boolean equals(Object obj) { public String toString() { return delegate.toString(); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java index 417145237ab9f..e586621a7ddb3 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java @@ -81,7 +81,6 @@ public boolean equals(Object obj) { NamedExpression other = (NamedExpression) obj; return Objects.equals(synthetic, other.synthetic) - && Objects.equals(id, other.id) /* * It is important that the line below be `name` * and not `name()` because subclasses might override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/FunctionAttribute.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/FunctionAttribute.java index 36ff097bdae8f..962fb010c4820 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/FunctionAttribute.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/FunctionAttribute.java @@ -29,11 +29,11 @@ public String functionId() { @Override public int hashCode() { - return Objects.hash(super.hashCode(), functionId); + return Objects.hash(super.hashCode()); } @Override public boolean equals(Object obj) { - return super.equals(obj) && Objects.equals(functionId, ((FunctionAttribute) obj).functionId()); + return super.equals(obj); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java index e8151ada18a9c..6372263c32a2f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java @@ -24,4 +24,16 @@ T value() { public String toString() { return format(null, "{{}={}}", prefix(), value); } + + + @Override + public int hashCode() { + return this.value.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if(!(obj instanceof Param)) return false; + return this.value.equals(((Param)obj).value); + } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Params.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Params.java index ed00160dbc3d5..e98bd2e0d9568 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Params.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Params.java @@ -124,4 +124,15 @@ else if (p instanceof Var) { public String toString() { return params.toString(); } -} \ No newline at end of file + + @Override + public int hashCode() { + return this.params.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if(!(obj instanceof Params)) return false; + return this.params.equals(((Params)obj).params); + } +} diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/AttributeMapTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/AttributeMapTests.java index c14f15b7b2f1b..f2a6045124e4b 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/AttributeMapTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/AttributeMapTests.java @@ -55,7 +55,7 @@ public void testMapConstructor() { Attribute one = m.keySet().iterator().next(); assertThat(m.containsKey(one), is(true)); - assertThat(m.containsKey(a("one")), is(false)); + assertThat(m.containsKey(a("one")), is(true)); assertThat(m.containsValue("one"), is(true)); assertThat(m.containsValue("on"), is(false)); assertThat(m.attributeNames(), contains("one", "two", "three")); @@ -74,7 +74,7 @@ public void testSingleItemConstructor() { assertThat(m.isEmpty(), is(false)); assertThat(m.containsKey(one), is(true)); - assertThat(m.containsKey(a("one")), is(false)); + assertThat(m.containsKey(a("one")), is(true)); assertThat(m.containsValue("one"), is(true)); assertThat(m.containsValue("on"), is(false)); } @@ -178,4 +178,4 @@ public void testForEach() { assertThat(m, is(copy)); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index 629d888791a92..7d624fb61e6a3 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -1271,8 +1271,7 @@ public void testGroupByConvertScalarWithNumericRef() { } public void testGroupByConstantScalar() { - // FIXME: same query with order by PI() * int still fails - PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY PI() * int LIMIT 10"); + PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY PI() * int ORDER BY PI() * int LIMIT 10"); assertEquals(EsQueryExec.class, p.getClass()); assertEquals(1, p.output().size()); assertEquals("PI() * int", p.output().get(0).qualifiedName()); From 3099b9fc89777b468da8b8e635fcd9cf9e1cfac9 Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Tue, 10 Sep 2019 21:54:40 +0200 Subject: [PATCH 04/14] Changed the behavior of QueryFolder and QueryContainer to use ExpressionId. Minor changes to adapt to the change of NamedExpression.equals and hashCode. Changed unit test because of the same reason. --- .../sql/qa/src/main/resources/agg.csv-spec | 33 ++++++++++--------- .../xpack/sql/analysis/analyzer/Analyzer.java | 3 +- .../xpack/sql/expression/FieldAttribute.java | 5 --- .../aggregate/AggregateFunctionAttribute.java | 6 ++-- .../sql/expression/gen/script/Param.java | 4 +++ .../xpack/sql/planner/QueryFolder.java | 19 ++++++----- .../querydsl/container/QueryContainer.java | 21 ++++++------ .../expression/UnresolvedAttributeTests.java | 2 -- .../sql/planner/QueryTranslatorTests.java | 3 +- 9 files changed, 48 insertions(+), 48 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec index 7c4063852e349..7bf8d8d3683d0 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec @@ -337,22 +337,23 @@ null 1965 ; -groupByConstantScalar -SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no LIMIT 10; - -PI() * emp_no:d ---------------- -31419.0681285515 -31422.2097212051 -31425.3513138587 -31428.4929065123 -31431.6344991659 -31434.7760918195 -31437.9176844731 -31441.0592771266 -31444.2008697802 -31447.3424624338 -; +// AwaitsFix https://github.com/elastic/elasticsearch/issues/36074 +//groupByConstantScalar +//SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no LIMIT 10; + +//PI() * emp_no:d +//--------------- +//31419.0681285515 +//31422.2097212051 +//31425.3513138587 +//31428.4929065123 +//31431.6344991659 +//31434.7760918195 +//31437.9176844731 +//31441.0592771266 +//31444.2008697802 +//31447.3424624338 +//; groupByConstantScalarWithAlias SELECT PI() * emp_no AS "value" FROM test_emp GROUP BY value ORDER BY value LIMIT 10; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index 415b536a6ac93..5fdd1f9124d7b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -609,9 +609,8 @@ protected LogicalPlan rule(LogicalPlan plan) { .map(or -> tryResolveExpression(or, o.child())) .collect(toList()); - AttributeSet resolvedRefs = new AttributeSet(maybeResolved.stream() + AttributeSet resolvedRefs = Expressions.references(maybeResolved.stream() .filter(Expression::resolved) - .map(exp->Expressions.attribute(exp.child())) .collect(toList())); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/FieldAttribute.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/FieldAttribute.java index c0cd9a95eb683..625a679898a93 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/FieldAttribute.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/FieldAttribute.java @@ -102,11 +102,6 @@ private FieldAttribute innerField(EsField type) { return new FieldAttribute(source(), this, name() + "." + type.getName(), type, qualifier(), nullable(), id(), synthetic()); } - @Override - protected Expression canonicalize() { - return new FieldAttribute(source(), null, "", field, null, Nullability.TRUE, id(), false); - } - @Override protected Attribute clone(Source source, String name, DataType type, String qualifier, Nullability nullability, ExpressionId id, boolean synthetic) { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/AggregateFunctionAttribute.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/AggregateFunctionAttribute.java index 0bd0c9199bcb7..463a277a8fa74 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/AggregateFunctionAttribute.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/AggregateFunctionAttribute.java @@ -75,14 +75,14 @@ public AggregateFunctionAttribute withFunctionId(String functionId, String prope @Override public int hashCode() { - return Objects.hash(super.hashCode(), innerId, propertyPath); + return Objects.hash(super.hashCode(), propertyPath); } @Override public boolean equals(Object obj) { if (super.equals(obj)) { AggregateFunctionAttribute other = (AggregateFunctionAttribute) obj; - return Objects.equals(innerId, other.innerId) && Objects.equals(propertyPath, other.propertyPath); + return Objects.equals(propertyPath, other.propertyPath); } return false; } @@ -91,4 +91,4 @@ public boolean equals(Object obj) { protected String label() { return "a->" + innerId(); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java index 6372263c32a2f..ffa0374bde0b9 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java @@ -5,6 +5,8 @@ */ package org.elasticsearch.xpack.sql.expression.gen.script; +import java.util.Objects; + import static org.elasticsearch.common.logging.LoggerMessageFormat.format; abstract class Param { @@ -28,12 +30,14 @@ public String toString() { @Override public int hashCode() { + if(this.value==null) return Objects.hashCode(null); return this.value.hashCode(); } @Override public boolean equals(Object obj) { if(!(obj instanceof Param)) return false; + if(this.value==null) return ((Param)obj).value == null; return this.value.equals(((Param)obj).value); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java index 3931ada383662..57f6de54ed077 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java @@ -72,6 +72,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -122,7 +123,7 @@ protected PhysicalPlan rule(ProjectExec project) { EsQueryExec exec = (EsQueryExec) project.child(); QueryContainer queryC = exec.queryContainer(); - Map aliases = new LinkedHashMap<>(queryC.aliases()); + Map aliases = new LinkedHashMap<>(queryC.aliases()); Map processors = new LinkedHashMap<>(queryC.scalarFunctions()); for (NamedExpression pj : project.projections()) { @@ -132,7 +133,7 @@ protected PhysicalPlan rule(ProjectExec project) { if (e instanceof NamedExpression) { Attribute attr = ((NamedExpression) e).toAttribute(); - aliases.put(aliasAttr, attr); + aliases.put(aliasAttr.id(), attr); // add placeholder for each scalar function if (e instanceof ScalarFunction) { processors.put(attr, Expressions.pipe(e)); @@ -153,7 +154,7 @@ protected PhysicalPlan rule(ProjectExec project) { } QueryContainer clone = new QueryContainer(queryC.query(), queryC.aggs(), queryC.fields(), - new AttributeMap<>(aliases), + new HashMap<>(aliases), queryC.pseudoFunctions(), new AttributeMap<>(processors), queryC.sort(), @@ -234,7 +235,7 @@ static EsQueryExec fold(AggregateExec a, EsQueryExec exec) { queryC = queryC.addGroups(groupingContext.groupMap.values()); } - Map aliases = new LinkedHashMap<>(); + Map aliases = new LinkedHashMap<>(); // tracker for compound aggs seen in a group Map compoundAggMap = new LinkedHashMap<>(); @@ -262,7 +263,7 @@ static EsQueryExec fold(AggregateExec a, EsQueryExec exec) { // record aliases in case they are later referred in the tree if (as != null && as.child() instanceof NamedExpression) { - aliases.put(as.toAttribute(), ((NamedExpression) as.child()).toAttribute()); + aliases.put(as.toAttribute().id(), ((NamedExpression) as.child()).toAttribute()); } // @@ -392,9 +393,9 @@ else if (ne.foldable()) { } if (!aliases.isEmpty()) { - Map newAliases = new LinkedHashMap<>(queryC.aliases()); + Map newAliases = new LinkedHashMap<>(queryC.aliases()); newAliases.putAll(aliases); - queryC = queryC.withAliases(new AttributeMap<>(newAliases)); + queryC = queryC.withAliases(new HashMap<>(newAliases)); } return new EsQueryExec(exec.source(), exec.index(), a.output(), queryC); } @@ -481,7 +482,7 @@ protected PhysicalPlan rule(OrderExec plan) { // check whether sorting is on an group (and thus nested agg) or field Attribute attr = ((NamedExpression) order.child()).toAttribute(); // check whether there's an alias (occurs with scalar functions which are not named) - attr = qContainer.aliases().getOrDefault(attr, attr); + attr = qContainer.aliases().getOrDefault(attr.id(), attr); String lookup = attr.id().toString(); GroupByKey group = qContainer.findGroupForAgg(lookup); @@ -504,7 +505,7 @@ protected PhysicalPlan rule(OrderExec plan) { if (sfa.orderBy() != null) { if (sfa.orderBy() instanceof NamedExpression) { Attribute at = ((NamedExpression) sfa.orderBy()).toAttribute(); - at = qContainer.aliases().getOrDefault(at, at); + at = qContainer.aliases().getOrDefault(at.id(), at); qContainer = qContainer.addSort(new AttributeSort(at, direction, missing)); } else if (!sfa.orderBy().foldable()) { // ignore constant diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java index 9cf79281d59be..ac3253bbd1dd2 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java @@ -39,6 +39,7 @@ import java.util.BitSet; import java.util.Collection; import java.util.Comparator; +import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -70,7 +71,7 @@ public class QueryContainer { private final List> fields; // aliases (maps an alias to its actual resolved attribute) - private final AttributeMap aliases; + private final Map aliases; // pseudo functions (like count) - that are 'extracted' from other aggs private final Map pseudoFunctions; @@ -98,7 +99,7 @@ public QueryContainer(Query query, Aggs aggs, List> fields, - AttributeMap aliases, + Map aliases, Map pseudoFunctions, AttributeMap scalarFunctions, Set sort, @@ -109,7 +110,7 @@ public QueryContainer(Query query, this.query = query; this.aggs = aggs == null ? Aggs.EMPTY : aggs; this.fields = fields == null || fields.isEmpty() ? emptyList() : fields; - this.aliases = aliases == null || aliases.isEmpty() ? AttributeMap.emptyAttributeMap() : aliases; + this.aliases = aliases == null || aliases.isEmpty() ? new HashMap<>() : aliases; this.pseudoFunctions = pseudoFunctions == null || pseudoFunctions.isEmpty() ? emptyMap() : pseudoFunctions; this.scalarFunctions = scalarFunctions == null || scalarFunctions.isEmpty() ? AttributeMap.emptyAttributeMap() : scalarFunctions; this.sort = sort == null || sort.isEmpty() ? emptySet() : sort; @@ -141,7 +142,7 @@ public List> sortingColumns() { if (as.attribute() instanceof AggregateFunctionAttribute) { aggSort = true; AggregateFunctionAttribute afa = (AggregateFunctionAttribute) as.attribute(); - afa = (AggregateFunctionAttribute) aliases.getOrDefault(afa, afa); + afa = (AggregateFunctionAttribute) aliases.getOrDefault(afa.innerId(), afa); int atIndex = -1; for (int i = 0; i < fields.size(); i++) { Tuple field = fields.get(i); @@ -179,7 +180,7 @@ public List> sortingColumns() { public BitSet columnMask(List columns) { BitSet mask = new BitSet(fields.size()); for (Attribute column : columns) { - Attribute alias = aliases.get(column); + Attribute alias = aliases.get(column.id()); // find the column index int index = -1; @@ -217,7 +218,7 @@ public List> fields() { return fields; } - public AttributeMap aliases() { + public Map aliases() { return aliases; } @@ -271,7 +272,7 @@ public QueryContainer withFields(List> f) { minPageSize); } - public QueryContainer withAliases(AttributeMap a) { + public QueryContainer withAliases(Map a) { return new QueryContainer(query, aggs, fields, a, pseudoFunctions, scalarFunctions, sort, limit, trackHits, includeFrozen, minPageSize); } @@ -312,7 +313,7 @@ public QueryContainer addSort(Sort sortable) { } private String aliasName(Attribute attr) { - return aliases.getOrDefault(attr, attr).name(); + return aliases.getOrDefault(attr.id(), attr).name(); } // @@ -397,7 +398,7 @@ static Query rewriteToContainNestedField(@Nullable Query query, Source source, S // replace function/operators's input with references private Tuple resolvedTreeComputingRef(ScalarFunctionAttribute ta) { - Attribute attribute = aliases.getOrDefault(ta, ta); + Attribute attribute = aliases.getOrDefault(ta.id(), ta); Pipe proc = scalarFunctions.get(attribute); // check the attribute itself @@ -419,7 +420,7 @@ private QueryAttributeResolver(QueryContainer container) { @Override public FieldExtraction resolve(Attribute attribute) { - Attribute attr = aliases.getOrDefault(attribute, attribute); + Attribute attr = aliases.getOrDefault(attribute.id(), attribute); Tuple ref = container.toReference(attr); container = ref.v1(); return ref.v2(); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/UnresolvedAttributeTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/UnresolvedAttributeTests.java index 4d35b40a98c71..4deca1d1f6362 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/UnresolvedAttributeTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/UnresolvedAttributeTests.java @@ -54,8 +54,6 @@ protected UnresolvedAttribute mutate(UnresolvedAttribute a) { () -> new UnresolvedAttribute(a.source(), a.name(), randomValueOtherThan(a.qualifier(), UnresolvedAttributeTests::randomQualifier), a.id(), a.unresolvedMessage(), a.resolutionMetadata()), - () -> new UnresolvedAttribute(a.source(), a.name(), a.qualifier(), - new ExpressionId(), a.unresolvedMessage(), a.resolutionMetadata()), () -> new UnresolvedAttribute(a.source(), a.name(), a.qualifier(), a.id(), randomValueOtherThan(a.unresolvedMessage(), () -> randomUnresolvedMessage()), a.resolutionMetadata()), diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index 7d624fb61e6a3..629d888791a92 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -1271,7 +1271,8 @@ public void testGroupByConvertScalarWithNumericRef() { } public void testGroupByConstantScalar() { - PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY PI() * int ORDER BY PI() * int LIMIT 10"); + // FIXME: same query with order by PI() * int still fails + PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY PI() * int LIMIT 10"); assertEquals(EsQueryExec.class, p.getClass()); assertEquals(1, p.output().size()); assertEquals("PI() * int", p.output().get(0).qualifiedName()); From 1f7540b0ba6da80127e838a67b6086c44c5cd48b Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Thu, 19 Sep 2019 16:25:01 +0200 Subject: [PATCH 05/14] fix for order by and group by ScalarFunctionAttribute --- .../sql/qa/src/main/resources/agg.csv-spec | 63 ++++++++++++++----- .../xpack/sql/analysis/analyzer/Analyzer.java | 12 ++-- .../xpack/sql/expression/Expressions.java | 27 ++++++++ .../sql/planner/QueryTranslatorTests.java | 20 +++++- 4 files changed, 102 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec index 7bf8d8d3683d0..09379746fc8fd 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec @@ -337,22 +337,57 @@ null 1965 ; -// AwaitsFix https://github.com/elastic/elasticsearch/issues/36074 -//groupByConstantScalar -//SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no LIMIT 10; +groupByConstantScalar +SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no LIMIT 10; + +PI() * emp_no:d +--------------- +31419.0681285515 +31422.2097212051 +31425.3513138587 +31428.4929065123 +31431.6344991659 +31434.7760918195 +31437.9176844731 +31441.0592771266 +31444.2008697802 +31447.3424624338 +; + +// AwaitsFix +//groupByConstantScalarWithOrderByDesc +//SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY -1 * (PI() * emp_no) LIMIT 10; + +//PI() * emp_no:d +//------- +//31730.0858012569 +//31726.9442086033 +//31723.8026159497 +//31720.6610232961 +//31717.5194306425 +//31714.3778379889 +//31711.2362453353 +//31708.0946526817 +//31704.9530600281 +//31701.8114673746 +//; + +// AwaitsFix +//groupByConstantScalarWithOrderByDesc +//SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no DESC LIMIT 10; //PI() * emp_no:d -//--------------- -//31419.0681285515 -//31422.2097212051 -//31425.3513138587 -//31428.4929065123 -//31431.6344991659 -//31434.7760918195 -//31437.9176844731 -//31441.0592771266 -//31444.2008697802 -//31447.3424624338 +//------- +//31730.0858012569 +//31726.9442086033 +//31723.8026159497 +//31720.6610232961 +//31717.5194306425 +//31714.3778379889 +//31711.2362453353 +//31708.0946526817 +//31704.9530600281 +//31701.8114673746 //; groupByConstantScalarWithAlias diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index 5fdd1f9124d7b..2b267ed9f8b81 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -66,6 +66,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; @@ -609,12 +610,15 @@ protected LogicalPlan rule(LogicalPlan plan) { .map(or -> tryResolveExpression(or, o.child())) .collect(toList()); - AttributeSet resolvedRefs = Expressions.references(maybeResolved.stream() - .filter(Expression::resolved) - .collect(toList())); + Set resolvedRefs = maybeResolved.stream() + .filter(Expression::resolved) + .collect(Collectors.toSet()); - AttributeSet missing = resolvedRefs.subtract(o.child().outputSet()); + AttributeSet missing = Expressions.referencesExcept( + resolvedRefs, + o.child().outputSet() + ); if (!missing.isEmpty()) { // Add missing attributes but project them away afterwards diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java index 0515d4f11b4e0..350d430f9286e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java @@ -16,6 +16,7 @@ import java.util.List; import java.util.Set; import java.util.function.Predicate; +import java.util.stream.Collectors; import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; @@ -102,6 +103,32 @@ public static AttributeSet references(List exps) { return set; } + public static AttributeSet referencesExcept(Set exps, + AttributeSet except) { + AttributeSet ret = new AttributeSet(); + while(exps.size()>0){ + + Set filteredExps = new LinkedHashSet<>(); + for(Expression exp: exps){ + Expression attr = Expressions.attribute(exp); + if(attr==null || !except.contains(attr)){ + filteredExps.add(exp); + } + } + + ret.addAll(new AttributeSet( + filteredExps.stream().filter(c->c.children().isEmpty()) + .flatMap(exp->exp.references().stream()) + .collect(Collectors.toSet()) + )); + + exps = filteredExps.stream() + .flatMap((Expression exp)->exp.children().stream()) + .collect(Collectors.toSet()); + } + return ret; + } + public static String name(Expression e) { return e instanceof NamedExpression ? ((NamedExpression) e).name() : e.nodeName(); } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index 629d888791a92..2b03810b57982 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -1271,8 +1271,8 @@ public void testGroupByConvertScalarWithNumericRef() { } public void testGroupByConstantScalar() { - // FIXME: same query with order by PI() * int still fails - PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test GROUP BY PI() * int LIMIT 10"); + PhysicalPlan p = optimizeAndPlan("SELECT PI() * int FROM test WHERE PI() * int > 5.0 GROUP BY PI() * int " + + "ORDER BY PI() * int LIMIT 10"); assertEquals(EsQueryExec.class, p.getClass()); assertEquals(1, p.output().size()); assertEquals("PI() * int", p.output().get(0).qualifiedName()); @@ -1367,6 +1367,22 @@ public void testGroupByConstantScalarWithNumericRef() { } } + public void testOrderByWithCastWithMissingRefs() { + PhysicalPlan p = optimizeAndPlan("SELECT keyword FROM test ORDER BY date::TIME, int LIMIT 5"); + assertEquals(EsQueryExec.class, p.getClass()); + assertEquals(1, p.output().size()); + assertEquals("test.keyword", p.output().get(0).qualifiedName()); + assertEquals(DataType.KEYWORD, p.output().get(0).dataType()); + assertThat( + ((EsQueryExec) p).queryContainer().toString() + .replaceAll("\\s+", ""), + endsWith("\"sort\":[{\"_script\":{\"script\":{\"source\":\"InternalSqlScriptUtils.nullSafeSortString(InternalSqlScriptUtils" + + ".cast(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1))\",\"lang\":\"painless\"," + + "\"params\":{\"v0\":\"date\",\"v1\":\"TIME\"}},\"type\":\"string\",\"order\":\"asc\"}},{\"int\":{\"order\":\"asc\"," + + "\"missing\":\"_last\",\"unmapped_type\":\"integer\"}}]}") + ); + } + public void testTopHitsAggregationWithOneArg() { { PhysicalPlan p = optimizeAndPlan("SELECT FIRST(keyword) FROM test"); From 34242158dfacf316f6c85c8713e9623aa37a3409 Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Fri, 20 Sep 2019 20:20:04 +0200 Subject: [PATCH 06/14] changed excepted test behavior to be similiar to that of testGeoShapeInWhereClause --- .../elasticsearch/xpack/sql/analysis/analyzer/Verifier.java | 4 ++-- .../sql/analysis/analyzer/VerifierErrorMessagesTests.java | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index e73b235c238dc..3f5caa064a2ed 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -102,7 +102,7 @@ String message() { @Override public int hashCode() { - return Objects.hash(node.source()); + return Objects.hash(node); } @Override @@ -116,7 +116,7 @@ public boolean equals(Object obj) { } Verifier.Failure other = (Verifier.Failure) obj; - return Objects.equals(node.source(), other.node.source()); + return Objects.equals(node, other.node); } @Override diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index cf9cf8d0ea77b..04e58d9fc874c 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -377,7 +377,8 @@ public void testMultiplyIntervalWithDecimalNotAllowed() { } public void testMultipleColumns() { - assertEquals("1:43: Unknown column [xxx]\nline 1:8: Unknown column [xxx]", + // We get only one message back because the messages are grouped by the node that caused the issue + assertEquals("1:43: Unknown column [xxx]", error("SELECT xxx FROM test GROUP BY DAY_oF_YEAR(xxx)")); } From d816f8fad919bc756a4d0882108f0f7fe2f78140 Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Wed, 9 Oct 2019 16:51:31 +0200 Subject: [PATCH 07/14] fix order by grouped ScalarFunctionAttribute rebased --- .../sql/qa/src/main/resources/agg.csv-spec | 106 ++++++++++-------- .../xpack/sql/planner/QueryFolder.java | 12 +- .../xpack/sql/querydsl/agg/Aggs.java | 15 ++- .../querydsl/container/QueryContainer.java | 4 +- .../container/QueryContainerTests.java | 7 +- 5 files changed, 79 insertions(+), 65 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec index 09379746fc8fd..8aa86349ee537 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec @@ -354,41 +354,22 @@ PI() * emp_no:d 31447.3424624338 ; -// AwaitsFix -//groupByConstantScalarWithOrderByDesc -//SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY -1 * (PI() * emp_no) LIMIT 10; - -//PI() * emp_no:d -//------- -//31730.0858012569 -//31726.9442086033 -//31723.8026159497 -//31720.6610232961 -//31717.5194306425 -//31714.3778379889 -//31711.2362453353 -//31708.0946526817 -//31704.9530600281 -//31701.8114673746 -//; - -// AwaitsFix -//groupByConstantScalarWithOrderByDesc -//SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no DESC LIMIT 10; - -//PI() * emp_no:d -//------- -//31730.0858012569 -//31726.9442086033 -//31723.8026159497 -//31720.6610232961 -//31717.5194306425 -//31714.3778379889 -//31711.2362453353 -//31708.0946526817 -//31704.9530600281 -//31701.8114673746 -//; +groupByConstantScalarWithOrderByDesc +SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no DESC LIMIT 10; + +PI() * emp_no:d +------- +31730.0858012569 +31726.9442086033 +31723.8026159497 +31720.6610232961 +31717.5194306425 +31714.3778379889 +31711.2362453353 +31708.0946526817 +31704.9530600281 +31701.8114673746 +; groupByConstantScalarWithAlias SELECT PI() * emp_no AS "value" FROM test_emp GROUP BY value ORDER BY value LIMIT 10; @@ -408,21 +389,52 @@ value:d ; groupByConstantScalarWithNumericRef -SELECT PI() * emp_no FROM test_emp GROUP BY 1 ORDER BY 1 LIMIT 10; +SELECT PI() * emp_no FROM test_emp GROUP BY 1 ORDER BY 1 DESC LIMIT 10; PI() * emp_no:d ---------------- -31419.0681285515 -31422.2097212051 -31425.3513138587 -31428.4929065123 -31431.6344991659 -31434.7760918195 -31437.9176844731 -31441.0592771266 -31444.2008697802 -31447.3424624338 +------- +31730.0858012569 +31726.9442086033 +31723.8026159497 +31720.6610232961 +31717.5194306425 +31714.3778379889 +31711.2362453353 +31708.0946526817 +31704.9530600281 +31701.8114673746 +; + +groupByFieldAndConstantScalarWithMultipleOrderBy +SELECT gender, emp_no % 3 + PI() FROM test_emp GROUP BY gender, emp_no % 3 + PI() ORDER BY gender, emp_no % 3 + PI() DESC LIMIT 8; + +gender:s |emp_no % 3 + PI():d +------------+------------------ +null |5.1415926535 +null |4.1415926535 +null |3.1415926535 +F |5.1415926535 +F |4.1415926535 +F |3.1415926535 +M |5.1415926535 +M |4.1415926535 +; + +groupByFieldAndConstantScalarWithAliasWithOrderByDescWithNullsFirst +SELECT gender, emp_no % 3 + PI() as p FROM test_emp GROUP BY gender, emp_no % 3 + PI() ORDER BY gender DESC, p DESC LIMIT 8; + +gender:s |p:d +------------+------------------ +M |5.1415926535 +M |4.1415926535 +M |3.1415926535 +F |5.1415926535 +F |4.1415926535 +F |3.1415926535 +null |5.1415926535 +null |4.1415926535 ; + // // Grouping functions // diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java index 57f6de54ed077..8dc9b5b595add 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java @@ -483,19 +483,11 @@ protected PhysicalPlan rule(OrderExec plan) { Attribute attr = ((NamedExpression) order.child()).toAttribute(); // check whether there's an alias (occurs with scalar functions which are not named) attr = qContainer.aliases().getOrDefault(attr.id(), attr); - String lookup = attr.id().toString(); - GroupByKey group = qContainer.findGroupForAgg(lookup); + GroupByKey group = qContainer.findGroupForAgg(attr); // TODO: might need to validate whether the target field or group actually exist if (group != null && group != Aggs.IMPLICIT_GROUP_KEY) { - // check whether the lookup matches a group - if (group.id().equals(lookup)) { - qContainer = qContainer.updateGroup(group.with(direction)); - } - // else it's a leafAgg - else { - qContainer = qContainer.updateGroup(group.with(direction)); - } + qContainer = qContainer.updateGroup(group.with(direction)); } else { // scalar functions typically require script ordering diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Aggs.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Aggs.java index 94f854c29f0b8..f16f2e70f7c13 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Aggs.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Aggs.java @@ -10,6 +10,8 @@ import org.elasticsearch.search.aggregations.bucket.composite.CompositeValuesSourceBuilder; import org.elasticsearch.search.aggregations.bucket.filter.FiltersAggregationBuilder; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; +import org.elasticsearch.xpack.sql.expression.Attribute; +import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunctionAttribute; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate; import org.elasticsearch.xpack.sql.querydsl.container.Sort.Direction; import org.elasticsearch.xpack.sql.util.StringUtils; @@ -121,16 +123,23 @@ public Aggs addAgg(PipelineAgg pipelineAgg) { return new Aggs(groups, simpleAggs, combine(pipelineAggs, pipelineAgg)); } - public GroupByKey findGroupForAgg(String groupOrAggId) { + public GroupByKey findGroupForAgg(Attribute attr) { + String id = attr.id().toString(); for (GroupByKey group : this.groups) { - if (groupOrAggId.equals(group.id())) { + if (id.equals(group.id())) { return group; } + if(attr instanceof ScalarFunctionAttribute) { + ScalarFunctionAttribute sfa = (ScalarFunctionAttribute) attr; + if (group.script()!=null && group.script().equals(sfa.script())) { + return group; + } + } } // maybe it's the default group agg ? for (Agg agg : simpleAggs) { - if (groupOrAggId.equals(agg.id())) { + if (id.equals(agg.id())) { return IMPLICIT_GROUP_KEY; } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java index ac3253bbd1dd2..d7235ad506914 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java @@ -487,8 +487,8 @@ public QueryContainer addGroups(Collection values) { return with(aggs.addGroups(values)); } - public GroupByKey findGroupForAgg(String aggId) { - return aggs.findGroupForAgg(aggId); + public GroupByKey findGroupForAgg(Attribute attr) { + return aggs.findGroupForAgg(attr); } public QueryContainer updateGroup(GroupByKey group) { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java index efae2eab2b3e7..bbcf734334417 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java @@ -9,6 +9,7 @@ import org.elasticsearch.xpack.sql.expression.Alias; import org.elasticsearch.xpack.sql.expression.Attribute; import org.elasticsearch.xpack.sql.expression.AttributeMap; +import org.elasticsearch.xpack.sql.expression.ExpressionId; import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.querydsl.query.BoolQuery; import org.elasticsearch.xpack.sql.querydsl.query.MatchAll; @@ -81,11 +82,11 @@ public void testColumnMaskShouldDuplicateSameAttributes() { Attribute fourth = new FieldAttribute(Source.EMPTY, "fourth", esField); Alias firstAliased = new Alias(Source.EMPTY, "firstAliased", first); - Map aliasesMap = new LinkedHashMap<>(); - aliasesMap.put(firstAliased.toAttribute(), first); + Map aliasesMap = new LinkedHashMap<>(); + aliasesMap.put(firstAliased.id(), first); QueryContainer queryContainer = new QueryContainer() - .withAliases(new AttributeMap<>(aliasesMap)) + .withAliases(aliasesMap) .addColumn(third) .addColumn(first) .addColumn(fourth) From cc423eadfa0b74bcf341ba1761a8b758895dcc53 Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Mon, 21 Oct 2019 19:59:13 +0200 Subject: [PATCH 08/14] removed unused import --- .../xpack/sql/querydsl/container/QueryContainerTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java index bbcf734334417..a23dc8a3f2784 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java @@ -8,7 +8,6 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.expression.Alias; import org.elasticsearch.xpack.sql.expression.Attribute; -import org.elasticsearch.xpack.sql.expression.AttributeMap; import org.elasticsearch.xpack.sql.expression.ExpressionId; import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.querydsl.query.BoolQuery; From f64e72009ece2e6f7f852c7dadd943b6d53cf96e Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Mon, 28 Oct 2019 21:10:07 +0100 Subject: [PATCH 09/14] solved recent issue with Pivot and EsRelation --- .../xpack/sql/plan/logical/Pivot.java | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java index 4a0639d8b78b3..b617e97ddbccf 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Objects; +import java.util.stream.Collectors; import static java.util.Collections.singletonList; @@ -47,7 +48,20 @@ protected NodeInfo info() { @Override protected Pivot replaceChild(LogicalPlan newChild) { - return new Pivot(source(), newChild, column, values, aggregates); + + java.util.function.Function withQualifierNull = (Expression e)-> { + if (e instanceof Attribute && newChild instanceof EsRelation) { + Attribute fa = (Attribute) e; + return fa.withQualifier(null); + } + return e; + }; + Expression newColumn = column.transformUp(withQualifierNull); + List newAggregates = aggregates.stream().map((NamedExpression aggregate)-> + (NamedExpression) aggregate.transformUp(withQualifierNull) + ).collect(Collectors.toUnmodifiableList()); + + return new Pivot(source(), newChild, newColumn, values, newAggregates); } public Expression column() { @@ -83,7 +97,7 @@ public AttributeSet valuesOutput() { if (aggregates.size() == 1) { NamedExpression agg = aggregates.get(0); for (NamedExpression value : values) { - ExpressionId id = new ExpressionId(agg.id().hashCode() + value.id().hashCode()); + ExpressionId id = value.id(); out.add(value.toAttribute().withDataType(agg.dataType()).withId(id)); } } @@ -92,7 +106,7 @@ public AttributeSet valuesOutput() { for (NamedExpression agg : aggregates) { String name = agg instanceof Function ? ((Function) agg).functionName() : agg.name(); for (NamedExpression value : values) { - ExpressionId id = new ExpressionId(agg.id().hashCode() + value.id().hashCode()); + ExpressionId id = value.id(); out.add(value.toAttribute().withName(value.name() + "_" + name).withDataType(agg.dataType()).withId(id)); } } @@ -139,4 +153,4 @@ public boolean equals(Object obj) { && Objects.equals(aggregates, other.aggregates) && Objects.equals(child(), other.child()); } -} \ No newline at end of file +} From c98df565808992c1268d8c2c05e77a39fced7a5c Mon Sep 17 00:00:00 2001 From: emasab Date: Tue, 29 Oct 2019 23:27:44 +0100 Subject: [PATCH 10/14] Apply suggestions from code review code style Co-Authored-By: Marios Trivyzas --- .../xpack/sql/expression/gen/script/Param.java | 12 +++++++++--- .../xpack/sql/expression/gen/script/Params.java | 4 +++- .../elasticsearch/xpack/sql/querydsl/agg/Aggs.java | 4 ++-- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java index ffa0374bde0b9..63b92be20a3d6 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Param.java @@ -30,14 +30,20 @@ public String toString() { @Override public int hashCode() { - if(this.value==null) return Objects.hashCode(null); + if (this.value == null) { + return Objects.hashCode(null); + } return this.value.hashCode(); } @Override public boolean equals(Object obj) { - if(!(obj instanceof Param)) return false; - if(this.value==null) return ((Param)obj).value == null; + if ((obj instanceof Param) == false) { + return false; + } + if (this.value == null) { + return ((Param)obj).value == null; + } return this.value.equals(((Param)obj).value); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Params.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Params.java index e98bd2e0d9568..073df0329d9c8 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Params.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Params.java @@ -132,7 +132,9 @@ public int hashCode() { @Override public boolean equals(Object obj) { - if(!(obj instanceof Params)) return false; + if ((obj instanceof Params) == false) { + return false; + } return this.params.equals(((Params)obj).params); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Aggs.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Aggs.java index f16f2e70f7c13..632eb729936fd 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Aggs.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Aggs.java @@ -129,9 +129,9 @@ public GroupByKey findGroupForAgg(Attribute attr) { if (id.equals(group.id())) { return group; } - if(attr instanceof ScalarFunctionAttribute) { + if (attr instanceof ScalarFunctionAttribute) { ScalarFunctionAttribute sfa = (ScalarFunctionAttribute) attr; - if (group.script()!=null && group.script().equals(sfa.script())) { + if (group.script() != null && group.script().equals(sfa.script())) { return group; } } From bafec272cf9b73b0d25f9791d8ea4aa13c2b926e Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Wed, 30 Oct 2019 01:03:05 +0100 Subject: [PATCH 11/14] changes suggested by @matriv --- .../xpack/sql/expression/Expressions.java | 3 +- .../xpack/sql/plan/logical/Pivot.java | 33 ++++++++++++------- .../querydsl/container/QueryContainer.java | 4 +-- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java index 350d430f9286e..198ca991b8b11 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java @@ -103,8 +103,7 @@ public static AttributeSet references(List exps) { return set; } - public static AttributeSet referencesExcept(Set exps, - AttributeSet except) { + public static AttributeSet referencesExcept(Set exps, AttributeSet except) { AttributeSet ret = new AttributeSet(); while(exps.size()>0){ diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java index b617e97ddbccf..1b4c1edcdae7e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java @@ -41,6 +41,14 @@ public Pivot(Source source, LogicalPlan child, Expression column, List info() { return NodeInfo.create(this, Pivot::new, child(), column, values, aggregates); @@ -49,17 +57,20 @@ protected NodeInfo info() { @Override protected Pivot replaceChild(LogicalPlan newChild) { - java.util.function.Function withQualifierNull = (Expression e)-> { - if (e instanceof Attribute && newChild instanceof EsRelation) { - Attribute fa = (Attribute) e; - return fa.withQualifier(null); - } - return e; - }; - Expression newColumn = column.transformUp(withQualifierNull); - List newAggregates = aggregates.stream().map((NamedExpression aggregate)-> - (NamedExpression) aggregate.transformUp(withQualifierNull) - ).collect(Collectors.toUnmodifiableList()); + Expression newColumn = column; + List newAggregates = aggregates; + + if(newChild instanceof EsRelation) { + // when changing from a SubQueryAlias to EsRelation + // the qualifier of the column and aggregates needs + // to be changed to null like the attributes of EsRelation + // otherwise they don't equal and aren't removed + // when calculating the groupingSet + newColumn = column.transformUp(Pivot::withQualifierNull); + newAggregates = aggregates.stream().map((NamedExpression aggregate) -> + (NamedExpression) aggregate.transformUp(Pivot::withQualifierNull) + ).collect(Collectors.toUnmodifiableList()); + } return new Pivot(source(), newChild, newColumn, values, newAggregates); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java index d7235ad506914..3dd1a2ac10834 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java @@ -38,8 +38,8 @@ import java.util.ArrayList; import java.util.BitSet; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; -import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -110,7 +110,7 @@ public QueryContainer(Query query, this.query = query; this.aggs = aggs == null ? Aggs.EMPTY : aggs; this.fields = fields == null || fields.isEmpty() ? emptyList() : fields; - this.aliases = aliases == null || aliases.isEmpty() ? new HashMap<>() : aliases; + this.aliases = aliases == null || aliases.isEmpty() ? Collections.emptyMap() : aliases; this.pseudoFunctions = pseudoFunctions == null || pseudoFunctions.isEmpty() ? emptyMap() : pseudoFunctions; this.scalarFunctions = scalarFunctions == null || scalarFunctions.isEmpty() ? AttributeMap.emptyAttributeMap() : scalarFunctions; this.sort = sort == null || sort.isEmpty() ? emptySet() : sort; From 5c53d91993ee9767761ee52707b20e87f30708d0 Mon Sep 17 00:00:00 2001 From: emasab Date: Wed, 30 Oct 2019 18:57:15 +0100 Subject: [PATCH 12/14] Apply suggestions from code review code style Co-Authored-By: Marios Trivyzas --- .../org/elasticsearch/xpack/sql/expression/Expressions.java | 6 +++--- .../org/elasticsearch/xpack/sql/plan/logical/Pivot.java | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java index 198ca991b8b11..6741600cbd961 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java @@ -105,12 +105,12 @@ public static AttributeSet references(List exps) { public static AttributeSet referencesExcept(Set exps, AttributeSet except) { AttributeSet ret = new AttributeSet(); - while(exps.size()>0){ + while (exps.size() > 0) { Set filteredExps = new LinkedHashSet<>(); - for(Expression exp: exps){ + for (Expression exp : exps) { Expression attr = Expressions.attribute(exp); - if(attr==null || !except.contains(attr)){ + if (attr == null || (except.contains(attr) == false)) { filteredExps.add(exp); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java index 1b4c1edcdae7e..fe06e1bb01869 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java @@ -41,7 +41,7 @@ public Pivot(Source source, LogicalPlan child, Expression column, List info() { @Override protected Pivot replaceChild(LogicalPlan newChild) { - Expression newColumn = column; List newAggregates = aggregates; - if(newChild instanceof EsRelation) { + if (newChild instanceof EsRelation) { // when changing from a SubQueryAlias to EsRelation // the qualifier of the column and aggregates needs // to be changed to null like the attributes of EsRelation From b68db36832ba6a43eb605b22ff5bb38fac910c82 Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Wed, 30 Oct 2019 19:20:00 +0100 Subject: [PATCH 13/14] change test case name --- x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec index 8aa86349ee537..19ee3d260b83c 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec @@ -420,7 +420,7 @@ M |5.1415926535 M |4.1415926535 ; -groupByFieldAndConstantScalarWithAliasWithOrderByDescWithNullsFirst +groupByFieldAndConstantScalarWithAliasWithOrderByDesc SELECT gender, emp_no % 3 + PI() as p FROM test_emp GROUP BY gender, emp_no % 3 + PI() ORDER BY gender DESC, p DESC LIMIT 8; gender:s |p:d From b442b6a5b6c0da9f63fd0a689fe76b0eb7d7f8a7 Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Wed, 30 Oct 2019 23:02:24 +0100 Subject: [PATCH 14/14] referencesExcept changed to filterReferences --- .../elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java | 2 +- .../org/elasticsearch/xpack/sql/expression/Expressions.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index 2b267ed9f8b81..c790626c5fbcb 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -615,7 +615,7 @@ protected LogicalPlan rule(LogicalPlan plan) { .filter(Expression::resolved) .collect(Collectors.toSet()); - AttributeSet missing = Expressions.referencesExcept( + AttributeSet missing = Expressions.filterReferences( resolvedRefs, o.child().outputSet() ); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java index 6741600cbd961..3e5450f01ac30 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java @@ -103,14 +103,14 @@ public static AttributeSet references(List exps) { return set; } - public static AttributeSet referencesExcept(Set exps, AttributeSet except) { + public static AttributeSet filterReferences(Set exps, AttributeSet excluded) { AttributeSet ret = new AttributeSet(); while (exps.size() > 0) { Set filteredExps = new LinkedHashSet<>(); for (Expression exp : exps) { Expression attr = Expressions.attribute(exp); - if (attr == null || (except.contains(attr) == false)) { + if (attr == null || (excluded.contains(attr) == false)) { filteredExps.add(exp); } }