From 0016c23987c74c99bb6d32361cd22775bbe4d808 Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Wed, 12 Jun 2019 17:58:38 +0200 Subject: [PATCH 1/4] bugfix for https://github.com/elastic/elasticsearch/issues/41811 --- .../querydsl/container/QueryContainer.java | 6 +- .../container/QueryContainerTests.java | 55 +++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) 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 c75a200820273..bbdf0465f0ba9 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 @@ -188,7 +188,9 @@ public BitSet columnMask(List columns) { .innerId() : alias.id()) : null; for (int i = 0; i < fields.size(); i++) { Tuple tuple = fields.get(i); - if (tuple.v2().equals(id) || (aliasId != null && tuple.v2().equals(aliasId))) { + // if the index is already set there is a collision, + // so continue searching for the other tuple with the same id + if ((tuple.v2().equals(id) || (aliasId != null && tuple.v2().equals(aliasId))) && !mask.get(i)) { index = i; break; } @@ -532,4 +534,4 @@ public String toString() { throw new RuntimeException("error rendering", e); } } -} \ No newline at end of file +} 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 424964bdbd9f3..4921e0b2c7e82 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 @@ -6,6 +6,10 @@ package org.elasticsearch.xpack.sql.querydsl.container; 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.FieldAttribute; import org.elasticsearch.xpack.sql.querydsl.query.BoolQuery; import org.elasticsearch.xpack.sql.querydsl.query.MatchAll; import org.elasticsearch.xpack.sql.querydsl.query.NestedQuery; @@ -13,8 +17,14 @@ import org.elasticsearch.xpack.sql.querydsl.query.RangeQuery; import org.elasticsearch.xpack.sql.tree.Source; import org.elasticsearch.xpack.sql.tree.SourceTests; +import org.elasticsearch.xpack.sql.type.DataType; +import org.elasticsearch.xpack.sql.type.EsField; import java.util.AbstractMap.SimpleImmutableEntry; +import java.util.Arrays; +import java.util.BitSet; +import java.util.LinkedHashMap; +import java.util.Map; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; @@ -60,4 +70,49 @@ public void testRewriteToContainsNestedFieldWhenDoesNotContainNestedFieldAndCant new MatchAll(source))); assertEquals(expected, QueryContainer.rewriteToContainNestedField(original, source, path, name, format, hasDocValues)); } + + public void testColumnMaskShouldDuplicateSameAttributes() { + + EsField esField = new EsField("str", DataType.TEXT, emptyMap(), true); + + Attribute first = new FieldAttribute(Source.EMPTY, "first", esField); + Attribute second = new FieldAttribute(Source.EMPTY, "second", esField); + Attribute third = new FieldAttribute(Source.EMPTY, "third", esField); + Attribute fourth = new FieldAttribute(Source.EMPTY, "third", esField); + Alias firstAliased = new Alias(Source.EMPTY, "firstAliased", first); + + Map aliasesMap = new LinkedHashMap<>(){{ + this.put(firstAliased.toAttribute(), first); + }}; + + QueryContainer queryContainer = new QueryContainer() + .withAliases(new AttributeMap<>(aliasesMap)) + .addColumn(third) + .addColumn(first) + .addColumn(fourth) + .addColumn(firstAliased.toAttribute()) + .addColumn(second) + .addColumn(first) + .addColumn(fourth); + + BitSet result = queryContainer.columnMask(Arrays.asList( + first, + first, + second, + third, + firstAliased.toAttribute() + )); + + BitSet expected = new BitSet(); + expected.set(0, true); + expected.set(1, true); + expected.set(2, false); + expected.set(3, true); + expected.set(4, true); + expected.set(5, true); + expected.set(6, false); + + + assertEquals(expected, result); + } } From 04cad42380034c4e82404173847f0669858e0cbc Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Wed, 11 Sep 2019 22:00:00 +0200 Subject: [PATCH 2/4] code style and integration tests --- x-pack/plugin/sql/qa/src/main/resources/agg.sql-spec | 6 ++++++ x-pack/plugin/sql/qa/src/main/resources/select.sql-spec | 6 ++++++ .../xpack/sql/querydsl/container/QueryContainer.java | 2 +- .../xpack/sql/querydsl/container/QueryContainerTests.java | 7 +++---- 4 files changed, 16 insertions(+), 5 deletions(-) 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 8ebdcf88e8543..2bb31e05f5a69 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 @@ -369,6 +369,12 @@ aggSumWithCastAndCountWithFilterAndLimit SELECT gender g, CAST(SUM(emp_no) AS BIGINT) s, COUNT(1) c FROM "test_emp" WHERE emp_no > 10000 GROUP BY g ORDER BY g LIMIT 1; aggSumWithAlias SELECT gender g, CAST(SUM(emp_no) AS BIGINT) s FROM "test_emp" GROUP BY g ORDER BY gender; +aggSumWithColumnRepeated +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY gender; +aggSumWithAliasWithColumnRepeated +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY g; +aggSumWithNumericRefWithColumnRepeated +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY 2; // Conditional SUM aggSumWithHaving diff --git a/x-pack/plugin/sql/qa/src/main/resources/select.sql-spec b/x-pack/plugin/sql/qa/src/main/resources/select.sql-spec index 9f9731efcc5b9..54694a4513615 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/select.sql-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/select.sql-spec @@ -33,6 +33,12 @@ multipleColumnsNoAliasWithLimit SELECT first_name, last_name FROM "test_emp" ORDER BY emp_no LIMIT 5; multipleColumnWithAliasWithAndWithoutAsWithLimit SELECT first_name f, last_name AS l FROM "test_emp" ORDER BY emp_no LIMIT 5; +multipleColumnNoAliasWithColumnRepeatedWithLimit +SELECT salary, first_name, salary FROM test_emp ORDER BY salary LIMIT 3; +multipleColumnWithAliasWithAsWithColumnRepeatedWithLimit +SELECT salary, first_name, salary AS x FROM test_emp ORDER BY x LIMIT 3; +multipleColumnWithAliasWithAndWithoutAsWithColumnRepeatedWithLimit +SELECT salary, first_name, salary AS x, salary y FROM test_emp ORDER BY y LIMIT 3; // // SELECT constant literals with FROM 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 bbdf0465f0ba9..9cf79281d59be 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 @@ -190,7 +190,7 @@ public BitSet columnMask(List columns) { Tuple tuple = fields.get(i); // if the index is already set there is a collision, // so continue searching for the other tuple with the same id - if ((tuple.v2().equals(id) || (aliasId != null && tuple.v2().equals(aliasId))) && !mask.get(i)) { + if (mask.get(i)==false && (tuple.v2().equals(id) || (aliasId != null && tuple.v2().equals(aliasId)))) { index = i; break; } 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 4921e0b2c7e82..efae2eab2b3e7 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 @@ -78,12 +78,11 @@ public void testColumnMaskShouldDuplicateSameAttributes() { Attribute first = new FieldAttribute(Source.EMPTY, "first", esField); Attribute second = new FieldAttribute(Source.EMPTY, "second", esField); Attribute third = new FieldAttribute(Source.EMPTY, "third", esField); - Attribute fourth = new FieldAttribute(Source.EMPTY, "third", esField); + Attribute fourth = new FieldAttribute(Source.EMPTY, "fourth", esField); Alias firstAliased = new Alias(Source.EMPTY, "firstAliased", first); - Map aliasesMap = new LinkedHashMap<>(){{ - this.put(firstAliased.toAttribute(), first); - }}; + Map aliasesMap = new LinkedHashMap<>(); + aliasesMap.put(firstAliased.toAttribute(), first); QueryContainer queryContainer = new QueryContainer() .withAliases(new AttributeMap<>(aliasesMap)) From 11ac9c1992849c17fa96be55fd40f4a54ed4cf67 Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Fri, 13 Sep 2019 00:19:05 +0200 Subject: [PATCH 3/4] converted aggregation tests to csv specifications added aggregation ordering csv specifications --- .../src/main/resources/agg-ordering.csv-spec | 26 ++++++++++++++++++ .../sql/qa/src/main/resources/agg.csv-spec | 27 +++++++++++++++++++ .../sql/qa/src/main/resources/agg.sql-spec | 6 ----- 3 files changed, 53 insertions(+), 6 deletions(-) create mode 100644 x-pack/plugin/sql/qa/src/main/resources/agg-ordering.csv-spec diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.csv-spec new file mode 100644 index 0000000000000..f570f7f57ae58 --- /dev/null +++ b/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.csv-spec @@ -0,0 +1,26 @@ +aggSumWithColumnRepeatedWithOrderAsc +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY gender ORDER BY gender; + +g:s | gender:s | s3:i | SUM(salary):i | s5:i +null |null |487605 |487605 |487605 +F |F |1666196|1666196 |1666196 +M |M |2671054|2671054 |2671054 +; + +aggSumWithAliasWithColumnRepeatedWithOrderDesc +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY g ORDER BY g DESC; + +g:s | gender:s | s3:i | SUM(salary):i | s5:i +M |M |2671054|2671054 |2671054 +F |F |1666196|1666196 |1666196 +null |null |487605 |487605 |487605 +; + +aggSumWithNumericRefWithColumnRepeatedWithOrderDesc +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY 2 ORDER BY 2 DESC; + +g:s | gender:s | s3:i | SUM(salary):i | s5:i +M |M |2671054|2671054 |2671054 +F |F |1666196|1666196 |1666196 +null |null |487605 |487605 |487605 +; \ No newline at end of file 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 5cc70a8cb5ef0..947b712145b6e 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 @@ -146,6 +146,33 @@ null |null |null |null |null |n 4 |4 |72 |4 |4.0 |0.0 |NaN |NaN ; +aggSumWithColumnRepeated +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY gender; + +g:s | gender:s | s3:i | SUM(salary):i | s5:i +null |null |487605 |487605 |487605 +F |F |1666196|1666196 |1666196 +M |M |2671054|2671054 |2671054 +; + +aggSumWithAliasWithColumnRepeated +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY g; + +g:s | gender:s | s3:i | SUM(salary):i | s5:i +null |null |487605 |487605 |487605 +F |F |1666196|1666196 |1666196 +M |M |2671054|2671054 |2671054 +; + +aggSumWithNumericRefWithColumnRepeated +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY 2; + +g:s | gender:s | s3:i | SUM(salary):i | s5:i +null |null |487605 |487605 |487605 +F |F |1666196|1666196 |1666196 +M |M |2671054|2671054 |2671054 +; + aggByComplexCastedValue SELECT CONVERT(CONCAT(LTRIM(CONVERT("emp_no", SQL_VARCHAR)), LTRIM(CONVERT("languages", SQL_VARCHAR))), SQL_BIGINT) AS "TEMP" FROM "test_emp" GROUP BY "TEMP" ORDER BY "TEMP" LIMIT 20; 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 2bb31e05f5a69..8ebdcf88e8543 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 @@ -369,12 +369,6 @@ aggSumWithCastAndCountWithFilterAndLimit SELECT gender g, CAST(SUM(emp_no) AS BIGINT) s, COUNT(1) c FROM "test_emp" WHERE emp_no > 10000 GROUP BY g ORDER BY g LIMIT 1; aggSumWithAlias SELECT gender g, CAST(SUM(emp_no) AS BIGINT) s FROM "test_emp" GROUP BY g ORDER BY gender; -aggSumWithColumnRepeated -SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY gender; -aggSumWithAliasWithColumnRepeated -SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY g; -aggSumWithNumericRefWithColumnRepeated -SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY 2; // Conditional SUM aggSumWithHaving From a305938067dffafb98c891d3bd38eae33ce2c87d Mon Sep 17 00:00:00 2001 From: Emanuele Sabellico Date: Wed, 25 Sep 2019 18:46:02 +0200 Subject: [PATCH 4/4] integration tests order by aggregation function --- .../plugin/sql/qa/src/main/resources/agg-ordering.csv-spec | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.csv-spec index f570f7f57ae58..ce96c34344ab8 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.csv-spec @@ -1,5 +1,5 @@ aggSumWithColumnRepeatedWithOrderAsc -SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY gender ORDER BY gender; +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY gender ORDER BY SUM(salary); g:s | gender:s | s3:i | SUM(salary):i | s5:i null |null |487605 |487605 |487605 @@ -8,7 +8,7 @@ M |M |2671054|2671054 |2671054 ; aggSumWithAliasWithColumnRepeatedWithOrderDesc -SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY g ORDER BY g DESC; +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY g ORDER BY s5 DESC; g:s | gender:s | s3:i | SUM(salary):i | s5:i M |M |2671054|2671054 |2671054 @@ -17,7 +17,7 @@ null |null |487605 |487605 |487605 ; aggSumWithNumericRefWithColumnRepeatedWithOrderDesc -SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY 2 ORDER BY 2 DESC; +SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY 2 ORDER BY 3 DESC; g:s | gender:s | s3:i | SUM(salary):i | s5:i M |M |2671054|2671054 |2671054