From b49a23ce56483f7199be7cb2641910915036c741 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Tue, 4 Feb 2020 23:09:43 +0100 Subject: [PATCH 1/7] SQL: Fix ORDER BY on aggregates and GROUPed BY fields Previously, in the in-memory sorting module `LocalAggregationSorterListener` only the aggregate functions where used (grabbed by the `sortingColumns`). As a consequence, if the ORDER BY was also using columns of the GROUP BY clause, (especially in the case of higher priority - before the aggregate functions) wrong results were produced. E.g.: ``` SELECT gender, MAX(salary) AS max FROM test_emp GROUP BY gender ORDER BY gender, max ``` Add all columns of the ORDER BY to the `sortingColumns` so that the `LocalAggregationSorterListener` can use the correct comparators in the underlying PriorityQueue used to implement the in-memory sorting. Fixes: #50355 --- .../src/main/resources/agg-ordering.sql-spec | 27 ++++++++- .../xpack/sql/planner/QueryFolder.java | 56 +++++++++--------- .../sql/querydsl/container/AggregateSort.java | 6 ++ .../sql/querydsl/container/AttributeSort.java | 6 ++ .../container/GroupingFunctionSort.java | 44 ++++++++++++++ .../querydsl/container/QueryContainer.java | 59 ++++++++----------- .../sql/querydsl/container/ScoreSort.java | 16 ++++- .../sql/querydsl/container/ScriptSort.java | 16 +++-- .../xpack/sql/querydsl/container/Sort.java | 2 + .../search/SourceGeneratorTests.java | 4 +- 10 files changed, 162 insertions(+), 74 deletions(-) create mode 100644 x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/GroupingFunctionSort.java diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.sql-spec b/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.sql-spec index 087dbdad1d18d..2937b34f0a539 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.sql-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.sql-spec @@ -80,7 +80,7 @@ aggNotSpecifiedInTheAggregateAndGroupWithHavingWithLimitAndDirection SELECT gender, MIN(salary) AS min, COUNT(*) AS c FROM test_emp GROUP BY gender HAVING c > 1 ORDER BY MAX(salary) ASC, c DESC LIMIT 5; groupAndAggNotSpecifiedInTheAggregateWithHaving -SELECT gender, MIN(salary) AS min, COUNT(*) AS c FROM test_emp GROUP BY gender HAVING c > 1 ORDER BY gender, MAX(salary); +SELECT gender, MIN(salary) AS min, COUNT(*) AS c FROM test_emp GROUP BY gender HAVING c > 1 ORDER BY gender NULLS FIRST, MAX(salary); multipleAggsThatGetRewrittenWithAliasOnAMediumGroupBy SELECT languages, MAX(salary) AS max, MIN(salary) AS min FROM test_emp GROUP BY languages ORDER BY max; @@ -136,5 +136,26 @@ SELECT gender AS g, first_name AS f, last_name AS l FROM test_emp GROUP BY f, ge multipleGroupingsAndOrderingByGroups_8 SELECT gender AS g, first_name, last_name FROM test_emp GROUP BY g, last_name, first_name ORDER BY gender ASC, first_name DESC, last_name ASC; -multipleGroupingsAndOrderingByGroupsWithFunctions -SELECT first_name f, last_name l, gender g, CONCAT(first_name, last_name) c FROM test_emp GROUP BY gender, l, f, c ORDER BY gender, c DESC, first_name, last_name ASC; +multipleGroupingsAndOrderingByGroupsAndAggs_1 +SELECT gender, MIN(salary) AS min, COUNT(*) AS c, MAX(salary) AS max FROM test_emp GROUP BY gender HAVING c > 1 ORDER BY gender ASC NULLS FIRST, MAX(salary) DESC; + +multipleGroupingsAndOrderingByGroupsAndAggs_2 +SELECT gender, MIN(salary) AS min, COUNT(*) AS c, MAX(salary) AS max FROM test_emp GROUP BY gender HAVING c > 1 ORDER BY gender DESC NULLS LAST, MAX(salary) ASC; + +multipleGroupingsAndOrderingByGroupsWithFunctions_1 +SELECT first_name f, last_name l, gender g, CONCAT(first_name, last_name) c FROM test_emp GROUP BY gender, l, f, c ORDER BY gender NULLS FIRST, c DESC, first_name, last_name ASC; + +multipleGroupingsAndOrderingByGroupsWithFunctions_2 +SELECT first_name f, last_name l, gender g, CONCAT(first_name, last_name) c FROM test_emp GROUP BY gender, l, f, c ORDER BY c DESC, gender DESC NULLS LAST, first_name, last_name ASC; + +multipleGroupingsAndOrderingByGroupsAndAggregatesWithFunctions_1 +SELECT CONCAT('foo', gender) g, MAX(salary) AS max, MIN(salary) AS min FROM test_emp GROUP BY g ORDER BY 1 NULLS FIRST, 2, 3; + +multipleGroupingsAndOrderingByGroupsAndAggregatesWithFunctions_2 +SELECT CONCAT('foo', gender) g, MAX(salary) AS max, MIN(salary) AS min FROM test_emp GROUP BY g ORDER BY 1 DESC NULLS LAST, 2, 3; + +multipleGroupingsAndOrderingByGroupsAndAggregatesWithFunctions_3 +SELECT CONCAT('foo', gender) g, MAX(salary) AS max, MIN(salary) AS min FROM test_emp GROUP BY g ORDER BY 2, 1 NULLS FIRST, 3; + +multipleGroupingsAndOrderingByGroupsAndAggregatesWithFunctions_4 +SELECT CONCAT('foo', gender) g, MAX(salary) AS max, MIN(salary) AS min FROM test_emp GROUP BY g ORDER BY 3 DESC, 1 NULLS FIRST, 2; 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 f426cb6dfdfad..1fa874ea4e481 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 @@ -66,6 +66,7 @@ import org.elasticsearch.xpack.sql.querydsl.container.GlobalCountRef; import org.elasticsearch.xpack.sql.querydsl.container.GroupByRef; import org.elasticsearch.xpack.sql.querydsl.container.GroupByRef.Property; +import org.elasticsearch.xpack.sql.querydsl.container.GroupingFunctionSort; import org.elasticsearch.xpack.sql.querydsl.container.MetricAggRef; import org.elasticsearch.xpack.sql.querydsl.container.PivotColumnRef; import org.elasticsearch.xpack.sql.querydsl.container.QueryContainer; @@ -682,37 +683,34 @@ protected PhysicalPlan rule(OrderExec plan) { // 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)); + } + + // field + if (orderExpression instanceof FieldAttribute) { + qContainer = qContainer.addSort(new AttributeSort((FieldAttribute) orderExpression, direction, missing)); + } + // scalar functions typically require script ordering + else if (orderExpression instanceof ScalarFunction) { + ScalarFunction sf = (ScalarFunction) orderExpression; + // nope, use scripted sorting + qContainer = qContainer.addSort(new ScriptSort(Expressions.id(sf), sf.asScript(), direction, missing)); + } + // histogram + else if (orderExpression instanceof Histogram) { + qContainer = qContainer.addSort(new GroupingFunctionSort(Expressions.id(orderExpression), direction, missing)); } + // score + else if (orderExpression instanceof Score) { + qContainer = qContainer.addSort(new ScoreSort(Expressions.id(orderExpression), direction, missing)); + } + // agg function + else if (orderExpression instanceof AggregateFunction) { + qContainer = qContainer.addSort(new AggregateSort((AggregateFunction) orderExpression, direction, missing)); + } + // unknown else { - // scalar functions typically require script ordering - if (orderExpression instanceof ScalarFunction) { - ScalarFunction sf = (ScalarFunction) orderExpression; - // nope, use scripted sorting - qContainer = qContainer.addSort(new ScriptSort(sf.asScript(), direction, missing)); - } - // score - else if (orderExpression instanceof Score) { - qContainer = qContainer.addSort(new ScoreSort(direction, missing)); - } - // field - else if (orderExpression instanceof FieldAttribute) { - qContainer = qContainer.addSort(new AttributeSort((FieldAttribute) orderExpression, direction, missing)); - } - // agg function - else if (orderExpression instanceof AggregateFunction) { - qContainer = qContainer.addSort(new AggregateSort((AggregateFunction) orderExpression, direction, missing)); - } else { - // unknown - throw new SqlIllegalArgumentException("unsupported sorting expression {}", orderExpression); - } + throw new SqlIllegalArgumentException("unsupported sorting expression {}", orderExpression); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AggregateSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AggregateSort.java index 50eb7efb4b1bc..b42bb53f98eb6 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AggregateSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AggregateSort.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.querydsl.container; +import org.elasticsearch.xpack.ql.expression.Expressions; import org.elasticsearch.xpack.ql.expression.function.aggregate.AggregateFunction; import java.util.Objects; @@ -23,6 +24,11 @@ public AggregateFunction agg() { return agg; } + @Override + public String id() { + return Expressions.id(agg); + } + @Override public int hashCode() { return Objects.hash(agg, direction(), missing()); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AttributeSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AttributeSort.java index cb6a42745b580..1f7742dc2a500 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AttributeSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AttributeSort.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.querydsl.container; import org.elasticsearch.xpack.ql.expression.Attribute; +import org.elasticsearch.xpack.ql.expression.Expressions; import java.util.Objects; @@ -22,6 +23,11 @@ public Attribute attribute() { return attribute; } + @Override + public String id() { + return Expressions.id(attribute); + } + @Override public int hashCode() { return Objects.hash(attribute, direction(), missing()); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/GroupingFunctionSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/GroupingFunctionSort.java new file mode 100644 index 0000000000000..56ed691444fb3 --- /dev/null +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/GroupingFunctionSort.java @@ -0,0 +1,44 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.sql.querydsl.container; + +import java.util.Objects; + +public class GroupingFunctionSort extends Sort { + + private final String id; + + public GroupingFunctionSort(String id, Direction direction, Missing missing) { + super(direction, missing); + this.id = id; + } + + @Override + public String id() { + return id; + } + + @Override + public int hashCode() { + return Objects.hash(direction(), missing(), id); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + + if (obj == null || getClass() != obj.getClass()) { + return false; + } + + GroupingFunctionSort other = (GroupingFunctionSort) obj; + return Objects.equals(direction(), other.direction()) + && Objects.equals(missing(), other.missing()) + && Objects.equals(id(), other.id()); + } +} 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 0279dd49180e9..57ab5bc8aaf6c 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 @@ -17,7 +17,6 @@ import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.Expressions; import org.elasticsearch.xpack.ql.expression.FieldAttribute; -import org.elasticsearch.xpack.ql.expression.function.aggregate.AggregateFunction; import org.elasticsearch.xpack.ql.expression.function.scalar.ScalarFunction; import org.elasticsearch.xpack.ql.expression.gen.pipeline.ConstantInput; import org.elasticsearch.xpack.ql.expression.gen.pipeline.Pipe; @@ -134,45 +133,39 @@ public List> sortingColumns() { return emptyList(); } - List> sortingColumns = new ArrayList<>(sort.size()); - - boolean aggSort = false; for (Sort s : sort) { - Tuple tuple = new Tuple<>(Integer.valueOf(-1), null); - if (s instanceof AggregateSort) { - AggregateSort as = (AggregateSort) s; - // find the relevant column of each aggregate function - AggregateFunction af = as.agg(); - - aggSort = true; - int atIndex = -1; - String id = Expressions.id(af); - - for (int i = 0; i < fields.size(); i++) { - Tuple field = fields.get(i); - if (field.v2().equals(id)) { - atIndex = i; - break; - } - } - if (atIndex == -1) { - throw new SqlIllegalArgumentException("Cannot find backing column for ordering aggregation [{}]", s); - } - // assemble a comparator for it - Comparator comp = s.direction() == Sort.Direction.ASC ? Comparator.naturalOrder() : Comparator.reverseOrder(); - comp = s.missing() == Sort.Missing.FIRST ? Comparator.nullsFirst(comp) : Comparator.nullsLast(comp); - - tuple = new Tuple<>(Integer.valueOf(atIndex), comp); + customSort = Boolean.TRUE; } - sortingColumns.add(tuple); } - + + // If no custom sort is used break early if (customSort == null) { - customSort = Boolean.valueOf(aggSort); + customSort = Boolean.FALSE; + return emptyList(); } - return aggSort ? sortingColumns : emptyList(); + List> sortingColumns = new ArrayList<>(sort.size()); + for (Sort s: sort) { + int atIndex = -1; + for (int i = 0; i < fields.size(); i++) { + Tuple field = fields.get(i); + if (field.v2().equals(s.id())) { + atIndex = i; + break; + } + } + if (atIndex==-1) { + throw new SqlIllegalArgumentException("Cannot find backing column for ordering aggregation [{}]", s); + } + // assemble a comparator for it + Comparator comp = s.direction()==Sort.Direction.ASC ? Comparator.naturalOrder():Comparator.reverseOrder(); + comp = s.missing()==Sort.Missing.FIRST ? Comparator.nullsFirst(comp):Comparator.nullsLast(comp); + + sortingColumns.add(new Tuple<>(Integer.valueOf(atIndex), comp)); + } + + return sortingColumns; } /** diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java index 21b4621d0d460..c2192b2e977aa 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java @@ -8,13 +8,22 @@ import java.util.Objects; public class ScoreSort extends Sort { - public ScoreSort(Direction direction, Missing missing) { + + final String id; + + public ScoreSort(String id, Direction direction, Missing missing) { super(direction, missing); + this.id = id; + } + + @Override + public String id() { + return id; } @Override public int hashCode() { - return Objects.hash(direction(), missing()); + return Objects.hash(direction(), missing(), id()); } @Override @@ -29,6 +38,7 @@ public boolean equals(Object obj) { ScriptSort other = (ScriptSort) obj; return Objects.equals(direction(), other.direction()) - && Objects.equals(missing(), other.missing()); + && Objects.equals(missing(), other.missing()) + && Objects.equals(id(), other.id()); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java index 284b60f1c1478..be4d4dca55a75 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java @@ -12,20 +12,27 @@ public class ScriptSort extends Sort { + private final String id; private final ScriptTemplate script; - public ScriptSort(ScriptTemplate script, Direction direction, Missing missing) { + public ScriptSort(String id, ScriptTemplate script, Direction direction, Missing missing) { super(direction, missing); + this.id = id; this.script = Scripts.nullSafeSort(script); } + @Override + public String id() { + return id; + } + public ScriptTemplate script() { return script; } @Override public int hashCode() { - return Objects.hash(direction(), missing(), script); + return Objects.hash(direction(), missing(), id(), script()); } @Override @@ -37,10 +44,11 @@ public boolean equals(Object obj) { if (obj == null || getClass() != obj.getClass()) { return false; } - + ScriptSort other = (ScriptSort) obj; return Objects.equals(direction(), other.direction()) && Objects.equals(missing(), other.missing()) - && Objects.equals(script, other.script); + && Objects.equals(id(), other.id()) + && Objects.equals(script(), other.script()); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/Sort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/Sort.java index c37677cb74f97..11f844fb9b892 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/Sort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/Sort.java @@ -49,6 +49,8 @@ protected Sort(Direction direction, Missing nulls) { this.missing = nulls; } + public abstract String id(); + public Direction direction() { return direction; } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java index 3fc5d96765ad9..0b90b815f5f73 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java @@ -97,7 +97,7 @@ public void testSelectScoreForcesTrackingScore() { public void testSortScoreSpecified() { QueryContainer container = new QueryContainer() - .addSort(new ScoreSort(Direction.DESC, null)); + .addSort(new ScoreSort("id", Direction.DESC, null)); SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, null, randomIntBetween(1, 10)); assertEquals(singletonList(scoreSort()), sourceBuilder.sorts()); } @@ -137,4 +137,4 @@ public void testNoSortIfAgg() { SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, null, randomIntBetween(1, 10)); assertNull(sourceBuilder.sorts()); } -} \ No newline at end of file +} From 90cb862906c76bb26f78862da76f6fe0471bc853 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 5 Feb 2020 13:00:51 +0100 Subject: [PATCH 2/7] make field private --- .../elasticsearch/xpack/sql/querydsl/container/ScoreSort.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java index c2192b2e977aa..ca1522ff48ceb 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java @@ -9,7 +9,7 @@ public class ScoreSort extends Sort { - final String id; + private final String id; public ScoreSort(String id, Direction direction, Missing missing) { super(direction, missing); From 9d25c93fda2f51bf9b343e8ced01ab724a18c47d Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 5 Feb 2020 13:12:02 +0100 Subject: [PATCH 3/7] fix formatting --- .../xpack/sql/querydsl/container/QueryContainer.java | 6 +++--- 1 file changed, 3 insertions(+), 3 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 57ab5bc8aaf6c..fa4696f768df3 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 @@ -155,12 +155,12 @@ public List> sortingColumns() { break; } } - if (atIndex==-1) { + if (atIndex == -1) { throw new SqlIllegalArgumentException("Cannot find backing column for ordering aggregation [{}]", s); } // assemble a comparator for it - Comparator comp = s.direction()==Sort.Direction.ASC ? Comparator.naturalOrder():Comparator.reverseOrder(); - comp = s.missing()==Sort.Missing.FIRST ? Comparator.nullsFirst(comp):Comparator.nullsLast(comp); + Comparator comp = s.direction() == Sort.Direction.ASC ? Comparator.naturalOrder() : Comparator.reverseOrder(); + comp = s.missing() == Sort.Missing.FIRST ? Comparator.nullsFirst(comp) : Comparator.nullsLast(comp); sortingColumns.add(new Tuple<>(Integer.valueOf(atIndex), comp)); } From aaf2bd3805852294ebfc8be12a613b4f44c82776 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 5 Feb 2020 13:14:47 +0100 Subject: [PATCH 4/7] break from loop once custom sort is recognized --- .../xpack/sql/querydsl/container/QueryContainer.java | 1 + 1 file changed, 1 insertion(+) 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 fa4696f768df3..2362591ae9870 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 @@ -136,6 +136,7 @@ public List> sortingColumns() { for (Sort s : sort) { if (s instanceof AggregateSort) { customSort = Boolean.TRUE; + break; } } From a1ec293d57854afa1cdf46cac39d3a78549e0b12 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Tue, 11 Feb 2020 19:07:02 +0100 Subject: [PATCH 5/7] change approach - add more tests --- .../src/main/resources/agg-ordering.csv-spec | 48 +++++++++++++- .../xpack/sql/execution/search/Querier.java | 47 ++++++++----- .../sql/execution/search/SourceGenerator.java | 2 +- .../xpack/sql/planner/QueryFolder.java | 12 ++-- .../sql/querydsl/container/AggregateSort.java | 6 -- .../sql/querydsl/container/AttributeSort.java | 6 -- .../container/GroupingFunctionSort.java | 15 +---- .../querydsl/container/QueryContainer.java | 39 ++++++----- .../sql/querydsl/container/ScoreSort.java | 15 +---- .../sql/querydsl/container/ScriptSort.java | 12 +--- .../xpack/sql/querydsl/container/Sort.java | 2 - .../sql/execution/search/QuerierTests.java | 66 ++++++++++++++++++- .../search/SourceGeneratorTests.java | 10 +-- 13 files changed, 186 insertions(+), 94 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 ce96c34344ab8..070abdb68b3fb 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 @@ -23,4 +23,50 @@ 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 +; + +histogramDateTimeWithCountAndOrder_1 +schema::h:ts|c:l +SELECT HISTOGRAM(birth_date, INTERVAL 1 YEAR) AS h, COUNT(*) as c FROM test_emp GROUP BY h ORDER BY h DESC, c ASC; + + h | c +------------------------+--------------- +1965-01-01T00:00:00.000Z|1 +1964-01-01T00:00:00.000Z|4 +1963-01-01T00:00:00.000Z|7 +1962-01-01T00:00:00.000Z|6 +1961-01-01T00:00:00.000Z|8 +1960-01-01T00:00:00.000Z|8 +1959-01-01T00:00:00.000Z|9 +1958-01-01T00:00:00.000Z|7 +1957-01-01T00:00:00.000Z|4 +1956-01-01T00:00:00.000Z|5 +1955-01-01T00:00:00.000Z|4 +1954-01-01T00:00:00.000Z|8 +1953-01-01T00:00:00.000Z|11 +1952-01-01T00:00:00.000Z|8 +null |10 +; + +histogramDateTimeWithCountAndOrder_2 +schema::h:ts|c:l +SELECT HISTOGRAM(birth_date, INTERVAL 1 YEAR) AS h, COUNT(*) as c FROM test_emp GROUP BY h ORDER BY c DESC, h ASC; + + h | c +------------------------+--------------- +1953-01-01T00:00:00.000Z|11 +null |10 +1959-01-01T00:00:00.000Z|9 +1952-01-01T00:00:00.000Z|8 +1954-01-01T00:00:00.000Z|8 +1960-01-01T00:00:00.000Z|8 +1961-01-01T00:00:00.000Z|8 +1958-01-01T00:00:00.000Z|7 +1963-01-01T00:00:00.000Z|7 +1962-01-01T00:00:00.000Z|6 +1956-01-01T00:00:00.000Z|5 +1955-01-01T00:00:00.000Z|4 +1957-01-01T00:00:00.000Z|4 +1964-01-01T00:00:00.000Z|4 +1965-01-01T00:00:00.000Z|1 +; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java index b84b75cbdcfda..a5c8e7ba13d3f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java @@ -594,36 +594,51 @@ static class AggSortingQueue extends PriorityQueue, Integer>> { this.sortingColumns = sortingColumns; } - // compare row based on the received attribute sort - // if a sort item is not in the list, it is assumed the sorting happened in ES - // and the results are left as is (by using the row ordering), otherwise it is sorted based on the given criteria. - // - // Take for example ORDER BY a, x, b, y - // a, b - are sorted in ES - // x, y - need to be sorted client-side - // sorting on x kicks in, only if the values for a are equal. - + /** + * Compare row based on the received attribute sort + *
    + *
  • + * If a tuple in {@code sortingColumns} has a null comparator, it is assumed the sorting + * happened in ES and the results are left as is (by using the row ordering), otherwise it is + * sorted based on the given criteria. + *
    • + *
  • + * If no tuple exists in {@code sortingColumns} for an output column, it means this column + * is not included at all in the ORDER BY + *
    • + *
+ * + * Take for example ORDER BY a, x, b, y + * a, b - are sorted in ES + * x, y - need to be sorted client-side + * sorting on x kicks in only if the values for a are equal. + * sorting on y kicks in only if the values for a, x and b are all equal + * + */ // thanks to @jpountz for the row ordering idea as a way to preserve ordering @SuppressWarnings("unchecked") @Override protected boolean lessThan(Tuple, Integer> l, Tuple, Integer> r) { for (Tuple tuple : sortingColumns) { - int i = tuple.v1().intValue(); + int columnIdx = tuple.v1().intValue(); Comparator comparator = tuple.v2(); - Object vl = l.v1().get(i); - Object vr = r.v1().get(i); + // Get the values for left and right rows at the current column index + Object vl = l.v1().get(columnIdx); + Object vr = r.v1().get(columnIdx); if (comparator != null) { int result = comparator.compare(vl, vr); - // if things are equals, move to the next comparator + // if things are not equal: return the comparison result, + // or else: move to the next comparator to solve the tie. if (result != 0) { return result > 0; } } - // no comparator means the existing order needs to be preserved + // no comparator means the rows are pre-ordered by ES for the column at + // the current index and the existing order needs to be preserved else { - // check the values - if they are equal move to the next comparator - // otherwise return the row order + // check the values - if they are equal not equal return the row order + // otherwise: move to the next comparator to solve the tie. if (Objects.equals(vl, vr) == false) { return l.v2().compareTo(r.v2()) > 0; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java index 453cdcde377ec..4fb4684e640b1 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java @@ -110,7 +110,7 @@ private static void sorting(QueryContainer container, SearchSourceBuilder source source.sort("_doc"); return; } - for (Sort sortable : container.sort()) { + for (Sort sortable : container.sort().values()) { SortBuilder sortBuilder = null; if (sortable instanceof AttributeSort) { 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 1fa874ea4e481..2838f7274122d 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 @@ -688,25 +688,27 @@ protected PhysicalPlan rule(OrderExec plan) { // field if (orderExpression instanceof FieldAttribute) { - qContainer = qContainer.addSort(new AttributeSort((FieldAttribute) orderExpression, direction, missing)); + qContainer = qContainer.addSort(lookup, + new AttributeSort((FieldAttribute) orderExpression, direction, missing)); } // scalar functions typically require script ordering else if (orderExpression instanceof ScalarFunction) { ScalarFunction sf = (ScalarFunction) orderExpression; // nope, use scripted sorting - qContainer = qContainer.addSort(new ScriptSort(Expressions.id(sf), sf.asScript(), direction, missing)); + qContainer = qContainer.addSort(lookup, new ScriptSort(sf.asScript(), direction, missing)); } // histogram else if (orderExpression instanceof Histogram) { - qContainer = qContainer.addSort(new GroupingFunctionSort(Expressions.id(orderExpression), direction, missing)); + qContainer = qContainer.addSort(lookup, new GroupingFunctionSort(direction, missing)); } // score else if (orderExpression instanceof Score) { - qContainer = qContainer.addSort(new ScoreSort(Expressions.id(orderExpression), direction, missing)); + qContainer = qContainer.addSort(lookup, new ScoreSort(direction, missing)); } // agg function else if (orderExpression instanceof AggregateFunction) { - qContainer = qContainer.addSort(new AggregateSort((AggregateFunction) orderExpression, direction, missing)); + qContainer = qContainer.addSort(lookup, + new AggregateSort((AggregateFunction) orderExpression, direction, missing)); } // unknown else { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AggregateSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AggregateSort.java index b42bb53f98eb6..50eb7efb4b1bc 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AggregateSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AggregateSort.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.querydsl.container; -import org.elasticsearch.xpack.ql.expression.Expressions; import org.elasticsearch.xpack.ql.expression.function.aggregate.AggregateFunction; import java.util.Objects; @@ -24,11 +23,6 @@ public AggregateFunction agg() { return agg; } - @Override - public String id() { - return Expressions.id(agg); - } - @Override public int hashCode() { return Objects.hash(agg, direction(), missing()); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AttributeSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AttributeSort.java index 1f7742dc2a500..cb6a42745b580 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AttributeSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/AttributeSort.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.querydsl.container; import org.elasticsearch.xpack.ql.expression.Attribute; -import org.elasticsearch.xpack.ql.expression.Expressions; import java.util.Objects; @@ -23,11 +22,6 @@ public Attribute attribute() { return attribute; } - @Override - public String id() { - return Expressions.id(attribute); - } - @Override public int hashCode() { return Objects.hash(attribute, direction(), missing()); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/GroupingFunctionSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/GroupingFunctionSort.java index 56ed691444fb3..f32c3548d03a7 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/GroupingFunctionSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/GroupingFunctionSort.java @@ -9,21 +9,13 @@ public class GroupingFunctionSort extends Sort { - private final String id; - - public GroupingFunctionSort(String id, Direction direction, Missing missing) { + public GroupingFunctionSort(Direction direction, Missing missing) { super(direction, missing); - this.id = id; - } - - @Override - public String id() { - return id; } @Override public int hashCode() { - return Objects.hash(direction(), missing(), id); + return Objects.hash(direction(), missing()); } @Override @@ -38,7 +30,6 @@ public boolean equals(Object obj) { GroupingFunctionSort other = (GroupingFunctionSort) obj; return Objects.equals(direction(), other.direction()) - && Objects.equals(missing(), other.missing()) - && Objects.equals(id(), other.id()); + && Objects.equals(missing(), other.missing()); } } 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 2362591ae9870..d3285dd65de22 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 @@ -42,15 +42,12 @@ import java.util.Collection; import java.util.Comparator; import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Set; import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; -import static java.util.Collections.emptySet; import static java.util.Collections.singletonMap; import static org.elasticsearch.xpack.ql.util.CollectionUtils.combine; @@ -81,7 +78,7 @@ public class QueryContainer { // at scrolling, their inputs (leaves) get updated private final AttributeMap scalarFunctions; - private final Set sort; + private final Map sort; private final int limit; private final boolean trackHits; private final boolean includeFrozen; @@ -105,7 +102,7 @@ public QueryContainer(Query query, AttributeMap aliases, Map pseudoFunctions, AttributeMap scalarFunctions, - Set sort, + Map sort, int limit, boolean trackHits, boolean includeFrozen, @@ -116,7 +113,7 @@ public QueryContainer(Query query, this.aliases = aliases == null || aliases.isEmpty() ? AttributeMap.emptyAttributeMap() : 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; + this.sort = sort == null || sort.isEmpty() ? emptyMap() : sort; this.limit = limit; this.trackHits = trackHits; this.includeFrozen = includeFrozen; @@ -133,7 +130,7 @@ public List> sortingColumns() { return emptyList(); } - for (Sort s : sort) { + for (Sort s : sort.values()) { if (s instanceof AggregateSort) { customSort = Boolean.TRUE; break; @@ -147,11 +144,14 @@ public List> sortingColumns() { } List> sortingColumns = new ArrayList<>(sort.size()); - for (Sort s: sort) { + for (Map.Entry entry : sort.entrySet()) { + String expressionId = entry.getKey(); + Sort s = entry.getValue(); + int atIndex = -1; for (int i = 0; i < fields.size(); i++) { Tuple field = fields.get(i); - if (field.v2().equals(s.id())) { + if (field.v2().equals(expressionId)) { atIndex = i; break; } @@ -159,9 +159,14 @@ public List> sortingColumns() { if (atIndex == -1) { throw new SqlIllegalArgumentException("Cannot find backing column for ordering aggregation [{}]", s); } - // assemble a comparator for it - Comparator comp = s.direction() == Sort.Direction.ASC ? Comparator.naturalOrder() : Comparator.reverseOrder(); - comp = s.missing() == Sort.Missing.FIRST ? Comparator.nullsFirst(comp) : Comparator.nullsLast(comp); + + // assemble a comparator for it, if it's not an AggregateSort + // then it's pre-sorted by ES so use null + Comparator comp = null; + if (s instanceof AggregateSort) { + comp = s.direction() == Sort.Direction.ASC ? Comparator.naturalOrder() : Comparator.reverseOrder(); + comp = s.missing() == Sort.Missing.FIRST ? Comparator.nullsFirst(comp) : Comparator.nullsLast(comp); + } sortingColumns.add(new Tuple<>(Integer.valueOf(atIndex), comp)); } @@ -224,7 +229,7 @@ public Map pseudoFunctions() { return pseudoFunctions; } - public Set sort() { + public Map sort() { return sort; } @@ -298,10 +303,10 @@ public QueryContainer withScalarProcessors(AttributeMap procs) { return new QueryContainer(query, aggs, fields, aliases, pseudoFunctions, procs, sort, limit, trackHits, includeFrozen, minPageSize); } - public QueryContainer addSort(Sort sortable) { - Set sort = new LinkedHashSet<>(this.sort); - sort.add(sortable); - return new QueryContainer(query, aggs, fields, aliases, pseudoFunctions, scalarFunctions, sort, limit, trackHits, includeFrozen, + public QueryContainer addSort(String expressionId, Sort sortable) { + Map newSort = new LinkedHashMap<>(this.sort); + newSort.put(expressionId, sortable); + return new QueryContainer(query, aggs, fields, aliases, pseudoFunctions, scalarFunctions, newSort, limit, trackHits, includeFrozen, minPageSize); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java index ca1522ff48ceb..c915f3609d4bd 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java @@ -9,21 +9,13 @@ public class ScoreSort extends Sort { - private final String id; - - public ScoreSort(String id, Direction direction, Missing missing) { + public ScoreSort(Direction direction, Missing missing) { super(direction, missing); - this.id = id; - } - - @Override - public String id() { - return id; } @Override public int hashCode() { - return Objects.hash(direction(), missing(), id()); + return Objects.hash(direction(), missing()); } @Override @@ -38,7 +30,6 @@ public boolean equals(Object obj) { ScriptSort other = (ScriptSort) obj; return Objects.equals(direction(), other.direction()) - && Objects.equals(missing(), other.missing()) - && Objects.equals(id(), other.id()); + && Objects.equals(missing(), other.missing()); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java index be4d4dca55a75..e54dd99f4e8f7 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java @@ -12,27 +12,20 @@ public class ScriptSort extends Sort { - private final String id; private final ScriptTemplate script; - public ScriptSort(String id, ScriptTemplate script, Direction direction, Missing missing) { + public ScriptSort(ScriptTemplate script, Direction direction, Missing missing) { super(direction, missing); - this.id = id; this.script = Scripts.nullSafeSort(script); } - @Override - public String id() { - return id; - } - public ScriptTemplate script() { return script; } @Override public int hashCode() { - return Objects.hash(direction(), missing(), id(), script()); + return Objects.hash(direction(), missing(), script()); } @Override @@ -48,7 +41,6 @@ public boolean equals(Object obj) { ScriptSort other = (ScriptSort) obj; return Objects.equals(direction(), other.direction()) && Objects.equals(missing(), other.missing()) - && Objects.equals(id(), other.id()) && Objects.equals(script(), other.script()); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/Sort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/Sort.java index 11f844fb9b892..c37677cb74f97 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/Sort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/Sort.java @@ -49,8 +49,6 @@ protected Sort(Direction direction, Missing nulls) { this.missing = nulls; } - public abstract String id(); - public Direction direction() { return direction; } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/QuerierTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/QuerierTests.java index a30a89addb816..2269aca7d8a9e 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/QuerierTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/QuerierTests.java @@ -66,6 +66,70 @@ public void testAggSorting_TwoFields() { } } + @SuppressWarnings("rawtypes") + public void testAggSorting_TwoFields_One_Presorted() { + List> tuples = new ArrayList<>(2); + tuples.add(new Tuple<>(0, null)); + tuples.add(new Tuple<>(1, Comparator.reverseOrder())); + Querier.AggSortingQueue queue = new AggSortingQueue(20, tuples); + + for (int i = 1; i <= 100; i++) { + queue.insertWithOverflow(new Tuple<>(Arrays.asList(i <= 5 ? null : 100 - i + 1, i), i)); + } + List> results = queue.asList(); + + assertEquals(20, results.size()); + for (int i = 0; i < 20; i++) { + assertEquals(i < 5 ? null : 100 - i, results.get(i).get(0)); + assertEquals(i < 5 ? 5 - i : i + 1, results.get(i).get(1)); + } + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + public void testAggSorting_FourFields() { + List comparators = Arrays.asList( + Comparator.naturalOrder(), + Comparator.naturalOrder(), + Comparator.reverseOrder(), + Comparator.naturalOrder() + ); + List> tuples = new ArrayList<>(4); + tuples.add(new Tuple<>(0, null)); + tuples.add(new Tuple<>(1, comparators.get(1))); + tuples.add(new Tuple<>(2, null)); + tuples.add(new Tuple<>(3, comparators.get(3))); + Querier.AggSortingQueue queue = new AggSortingQueue(35, tuples); + + List> expected = new ArrayList<>(128); + for (int i = 0; i < 128; i++) { + int col1 = i / 16; + int col2 = 15 - (i / 8); + int col3 = 32 - (i / 4); + int col4 = 127 - i; + + expected.add(Arrays.asList(col1, col2, col3, col4)); + queue.insertWithOverflow(new Tuple<>(Arrays.asList(col1, col2, col3, col4), i)); + } + + expected.sort((o1, o2) -> { + for (int i = 0; i < 4; i++) { + int result = comparators.get(i).compare(o1.get(i), o2.get(i)); + if (result != 0) { + return result; + } + } + return 0; + }); + List> results = queue.asList(); + + assertEquals(35, results.size()); + for (int i = 0; i < 35; i++) { + for (int j = 0; j < 4; j++) { + assertEquals(expected.get(i).get(j), results.get(i).get(j)); + } + } + } + @SuppressWarnings("rawtypes") public void testAggSorting_Randomized() { // Initialize comparators for fields (columns) @@ -76,7 +140,7 @@ public void testAggSorting_Randomized() { boolean order = randomBoolean(); ordering[j] = order; Comparator comp = order ? Comparator.naturalOrder() : Comparator.reverseOrder(); - tuples.add(new Tuple(j, comp)); + tuples.add(new Tuple<>(j, comp)); } // Insert random no of documents (rows) with random 0/1 values for each field diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java index 0b90b815f5f73..3bbfafab07401 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java @@ -97,7 +97,7 @@ public void testSelectScoreForcesTrackingScore() { public void testSortScoreSpecified() { QueryContainer container = new QueryContainer() - .addSort(new ScoreSort("id", Direction.DESC, null)); + .addSort("id", new ScoreSort(Direction.DESC, null)); SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, null, randomIntBetween(1, 10)); assertEquals(singletonList(scoreSort()), sourceBuilder.sorts()); } @@ -106,14 +106,14 @@ public void testSortFieldSpecified() { FieldSortBuilder sortField = fieldSort("test").unmappedType("keyword"); QueryContainer container = new QueryContainer() - .addSort(new AttributeSort(new FieldAttribute(Source.EMPTY, "test", new KeywordEsField("test")), Direction.ASC, - Missing.LAST)); + .addSort("id", new AttributeSort(new FieldAttribute(Source.EMPTY, "test", new KeywordEsField("test")), + Direction.ASC, Missing.LAST)); SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, null, randomIntBetween(1, 10)); assertEquals(singletonList(sortField.order(SortOrder.ASC).missing("_last")), sourceBuilder.sorts()); container = new QueryContainer() - .addSort(new AttributeSort(new FieldAttribute(Source.EMPTY, "test", new KeywordEsField("test")), Direction.DESC, - Missing.FIRST)); + .addSort("id", new AttributeSort(new FieldAttribute(Source.EMPTY, "test", new KeywordEsField("test")), + Direction.DESC, Missing.FIRST)); sourceBuilder = SourceGenerator.sourceBuilder(container, null, randomIntBetween(1, 10)); assertEquals(singletonList(sortField.order(SortOrder.DESC).missing("_first")), sourceBuilder.sorts()); } From 46b9700ef80131a3cc88022991c6c9983d294ca9 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Tue, 11 Feb 2020 19:10:16 +0100 Subject: [PATCH 6/7] revert changes to untouched files --- .../xpack/sql/querydsl/container/ScoreSort.java | 1 - .../xpack/sql/querydsl/container/ScriptSort.java | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java index c915f3609d4bd..21b4621d0d460 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScoreSort.java @@ -8,7 +8,6 @@ import java.util.Objects; public class ScoreSort extends Sort { - public ScoreSort(Direction direction, Missing missing) { super(direction, missing); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java index e54dd99f4e8f7..284b60f1c1478 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/ScriptSort.java @@ -25,7 +25,7 @@ public ScriptTemplate script() { @Override public int hashCode() { - return Objects.hash(direction(), missing(), script()); + return Objects.hash(direction(), missing(), script); } @Override @@ -37,10 +37,10 @@ public boolean equals(Object obj) { if (obj == null || getClass() != obj.getClass()) { return false; } - + ScriptSort other = (ScriptSort) obj; return Objects.equals(direction(), other.direction()) && Objects.equals(missing(), other.missing()) - && Objects.equals(script(), other.script()); + && Objects.equals(script, other.script); } } From 637a9f9591542e9bb23a1d443550b104e43bec41 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Tue, 11 Feb 2020 21:13:56 +0100 Subject: [PATCH 7/7] Address comment --- .../org/elasticsearch/xpack/sql/execution/search/Querier.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java index a5c8e7ba13d3f..25de08ef81694 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java @@ -629,7 +629,7 @@ protected boolean lessThan(Tuple, Integer> l, Tuple, Integer> r) if (comparator != null) { int result = comparator.compare(vl, vr); // if things are not equal: return the comparison result, - // or else: move to the next comparator to solve the tie. + // otherwise: move to the next comparator to solve the tie. if (result != 0) { return result > 0; } @@ -637,7 +637,7 @@ protected boolean lessThan(Tuple, Integer> l, Tuple, Integer> r) // no comparator means the rows are pre-ordered by ES for the column at // the current index and the existing order needs to be preserved else { - // check the values - if they are equal not equal return the row order + // check the values - if they are not equal return the row order // otherwise: move to the next comparator to solve the tie. if (Objects.equals(vl, vr) == false) { return l.v2().compareTo(r.v2()) > 0;