SQL: Allow sorting of groups by aggregates #38042

costin · 2019-01-30T16:24:27Z

Introduce client-side, custom sorting of groups based on aggregate
functions. To allow this, the Analyzer has been extended to push down
to underlying Aggregate, aggregate function and the Querier has been
extended to identify the case and consume the results in order and sort
them based on the given columns.
The underlying QueryContainer has been slightly modified to allow a view
of the underlying values being extracted as the columns used for sorting
might not be requested by the user.

The PR also adds minor tweaks, mainly related to tree output.

Close #35118

Introduce client-side, custom sorting of groups based on aggregate functions. To allow this, the Analyzer has been extended to push down to underlying Aggregate, aggregate function and the Querier has been extended to identify the case and consume the results in order and sort them based on the given columns. The underlying QueryContainer has been slightly modified to allow a view of the underlying values being extracted as the columns used for sorting might not be requested by the user. The PR also adds minor tweaks, mainly related to tree output. Close elastic#35118

elasticmachine · 2019-01-30T16:24:31Z

Pinging @elastic/es-search

costin · 2019-01-30T16:29:35Z

There are still some things that need to be added in a future PR, mainly:

move the metrics from the transport layer to the planExecutor so in case of sorting, the stats are properly incremented (since multiple requests are done).
figure out the policy for establishing the maximum number of items to be sorted. Currently the PR uses an arbitrary number (100) however this requires investigation.
While initially I was favoring the idea of the user being required to specify a limit - LIMIT 100 I think that's quite annoying and in most cases we'll have an upper limit anyway.
So another variant is to always allocate the higher limit in case no LIMIT is specified and throw an exception at runtime if the limit is reached to tell the user that they have to specify a manual LIMIT.

matriv

Looks good overall, Really important feature!
Left some comments and additionally:

Please add tests with ordering on multiple columns (tie breaker)
Please add tests with ordering using number indexes (e.g.: ORDER BY 1, 2)
Please add tests with descending ordering
Please remove the paragraph from the limitations doc page.

matriv · 2019-01-31T10:21:21Z

x-pack/plugin/sql/qa/src/main/resources/agg-ordering.sql-spec

+aggWithAlias
+SELECT MAX(salary) AS m FROM test_emp GROUP BY gender ORDER BY m;
+
+multipleAggsThatGetRewrittenWithoutAlias


What do you mean Rewritten?

max and avg and count get rewritten/replaced with a Stats agg.

matriv · 2019-01-31T10:23:27Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

+            }
+
+            // 2. find first Aggregate child and update it
+


you can remove empty line

matriv · 2019-01-31T10:29:59Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

@@ -310,11 +310,31 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set<Failure> localFailur
                Aggregate a = (Aggregate) child;

                Map<Expression, Node<?>> missing = new LinkedHashMap<>();
+
+                // track aggs and non-aggs - to keep the final modifier, use an array
+                final Expression[] aggAndNonAgg = new Expression[2];


You can use a boolean[] instead here, no?

No, because if found, the expression is used in the validation message.

sorry, missed that part

matriv · 2019-01-31T10:34:42Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

+
+            // 2. find first Aggregate child and update it
+
+            final AtomicBoolean found = new AtomicBoolean(false);


Why is Atomic needed here? is there some kind of concurrency?

Not it's an utility class for storing state outside a lamba expression. Since the class is already part of the JDK (so it's likely to be inlined) it's quite convenient. The alternative is to create our own Holder class.

got it thx!

I would opt for a boolean[] found = new boolean[] {false} instead, to avoid confusion and because AtomicBoolean has special semantics for the compiler to achieve the atomic operations in CPU level which is a bit too much, imho, just for convenience.

matriv · 2019-01-31T10:36:18Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java

+    /**
+     * Listener used for local sorting (typically due to aggregations used inside `ORDER BY`).
+     * 
+     * This listener consumes the whole result set, sorts it in memory then send the paginates


...then sends the paginated results back to the client

matriv · 2019-01-31T13:15:39Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

-            // if the sort points to an agg, consider it only if there's grouping
-            if (ob.child() instanceof Aggregate) {
-                Aggregate a = (Aggregate) ob.child();
+            AtomicBoolean foundAggregate = new AtomicBoolean(false);


here too? why AtomicBoolean?

matriv · 2019-01-31T13:16:26Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/physical/EsQueryExec.java

-        Querier scroller = new Querier(session.client(), session.configuration());
-        scroller.query(Rows.schema(output), queryContainer, index, listener);
+        Querier scroller = new Querier(session);
+        //        List<ExpressionId> ids = queryContainer.columns();


matriv · 2019-01-31T13:21:47Z

.../src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java

-    public void testGroupByOrderByNonKey() {
-        assertEquals("1:52: Cannot order by non-grouped column [a], expected [bool]",
-                error("SELECT AVG(int) a FROM test GROUP BY bool ORDER BY a"));
+    //    public void testGroupByOrderByNonKey() {


leftover, please remove.

matriv · 2019-01-31T13:25:34Z

...ql/qa/single-node/src/test/java/org/elasticsearch/xpack/sql/qa/single_node/CliExplainIT.java

@@ -64,22 +64,22 @@ public void testExplainWithWhere() throws IOException {
        assertThat(readLine(), startsWith("----------"));
        assertThat(readLine(), startsWith("With[{}]"));
        assertThat(readLine(), startsWith("\\_Project[[?*]]"));
-        assertThat(readLine(), startsWith("  \\_Filter[i = 2#"));
-        assertThat(readLine(), startsWith("    \\_UnresolvedRelation[[][index=test],null,Unknown index [test]]"));
+        assertThat(readLine(), startsWith("  \\_Filter[Equals[?i"));


Here and in the other places: Can't we also still check the 2?

We can for unresolved items but for resolved ones not really since the tests are ran in random order and the ids change between runs. Fwiw, the final generated query is tested and the value checked.

Not important at all but can't we use endsWith?

No because 2 is a literal with its own id.

matriv · 2019-01-31T13:26:56Z

x-pack/plugin/sql/qa/src/main/resources/agg-ordering.sql-spec

+// Custom sorting/ordering on aggregates
+//
+
+countWithImplicitGroupBy


I don't find the value of having all these different cases without group by, as only 1 value is returned and the ORDER BY doesn't really apply, or am I missing something?

Indeed but there's a dedicated rule in the optimizer for this case that excludes the order (as there's no physical group for it).

astefan

Great stuff. Left few minor comments

astefan · 2019-01-31T09:40:26Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

@@ -310,11 +310,31 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set<Failure> localFailur
                Aggregate a = (Aggregate) child;

                Map<Expression, Node<?>> missing = new LinkedHashMap<>();
+
+                // track aggs and non-aggs - to keep the final modifier, use an array
+                final Expression[] aggAndNonAgg = new Expression[2];


Wouldn't have been more clear/easier to manage with two separate Expressions named accordingly? (ie agg/nonAgg)

I don't think so because one would have two arrays - this is really a hack for the setting the state outside a lamba expression. I'll probably go for a Holder class which be more meaningful in intent.

astefan · 2019-01-31T09:47:55Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java

+    /**
+     * Listener used for local sorting (typically due to aggregations used inside `ORDER BY`).
+     * 
+     * This listener consumes the whole result set, sorts it in memory then send the paginates


"send AND paginates..." you meant?

"sends the paginated" response. Fixed.

astefan · 2019-01-31T09:49:50Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java

+        LocalAggregationSorterListener(ActionListener<SchemaRowSet> listener, List<Tuple<Integer, Comparator>> sortingColumns, int limit) {
+            this.listener = listener;
+
+            this.data = new PriorityQueue<Tuple<List<?>, Integer>>(Math.max(limit, 100)) {


Maybe extract the 100 value as a constant to be easier to change in the future? (very likely we will change it or allow the user to configure a default value)

astefan · 2019-01-31T12:39:04Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/physical/EsQueryExec.java

-        Querier scroller = new Querier(session.client(), session.configuration());
-        scroller.query(Rows.schema(output), queryContainer, index, listener);
+        Querier scroller = new Querier(session);
+        //        List<ExpressionId> ids = queryContainer.columns();


Are these still needed?

No, I've removed them

astefan · 2019-01-31T12:47:39Z

.../src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java

    }

-    public void testGroupByOrderByFunctionOverKey() {


Why removing this method? Why not only adding the new one?

astefan · 2019-01-31T12:49:03Z

.../src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java

@@ -312,14 +312,19 @@ public void testUnsupportedTypeInOrder() {
                error("SELECT * FROM test ORDER BY unsupported"));
    }

-    public void testGroupByOrderByNonKey() {


Why removing this test?

This needs a bit of reworking.

astefan · 2019-01-31T13:30:15Z

One idea for a test (from the odbc tests): select gender as g, sum(salary) as s from test_emp group by 1 order by 2 desc limit 3;

astefan · 2019-01-31T13:40:48Z

sql> select gender as g, min(salary) as min, max(salary) as max from test_emp group by 1 order by 2, 3, 2, 4 desc limit 3;
Bad request [line 1:103: Invalid %d specified in OrderBy (valid range is [1, %d])]

astefan · 2019-01-31T13:45:52Z

.../src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java

-        assertEquals("1:44: Cannot order by non-grouped column [MAX(int)], expected [int]",
-                error("SELECT int FROM test GROUP BY int ORDER BY MAX(int)"));
+    public void testGroupByOrderByAggAndGroupedColumn() {
+        assertEquals("1:49: Cannot order by aggregated [MAX(int)] and non-aggregated [int] columns"


Can you add a test where ORDER BY has more than two elements?

matriv · 2019-01-31T13:50:08Z

I would also add the 100 implicit limit to the docs.

costin · 2019-01-31T18:01:53Z

@matriv see number 2) in my initial comment.

matriv · 2019-01-31T21:04:36Z

@costin yep read the comment, I was just thinking to have like an autonomous complete PR. But I'm totally fine to add the proper docs once this bit is figured out and implemented correctly. Personally, if no LIMIT is provided by the user, I'd favour to have a documented implicit limit rather than exhausting available memory and throw an Exception. On the other hand if the latter can implemented easily with the use of a CircuitBreaker maybe it's a nice option too.

costin · 2019-02-01T16:49:51Z

sql> select gender as g, min(salary) as min, max(salary) as max from test_emp group by 1 order by 2, 3, 2, 4 desc limit 3;
Bad request [line 1:103: Invalid %d specified in OrderBy (valid range is [1, %d])]

I've created a separate issue for this since it needs backporting to 6.x. It's not just the message but also the fact that the Analyzer rule throws an exception (with an incorrect message).
See #38208

costin · 2019-02-01T16:52:19Z

@costin yep read the comment, I was just thinking to have like an autonomous complete PR. But I'm totally fine to add the proper docs once this bit is figured out and implemented correctly. Personally, if no LIMIT is provided by the user, I'd favour to have a documented implicit limit rather than exhausting available memory and throw an Exception. On the other hand if the latter can implemented easily with the use of a CircuitBreaker maybe it's a nice option too.

In case the LIMIT is not specified, the hard limit (256) is enforced however the moment an entry is dropped, an exception is thrown.
The idea being that we can sort 256 entries however more than that and we lose some records - and appropriately tell the user instead of silently applying that.

costin · 2019-02-01T16:53:58Z

@astefan @matriv I've addressed more comments, beefed up the test suite (with some tie breakers) and addressed point 2).
There will be a follow-up PR regarding the move of metrics and one for the docs to add some examples.

matriv

LGTM!!!

astefan

LGTM. Great feature.

costin · 2019-02-01T21:12:27Z

@elasticmachine run elasticsearch-ci/packaging-sample

Introduce client-side sorting of groups based on aggregate functions. To allow this, the Analyzer has been extended to push down to underlying Aggregate, aggregate function and the Querier has been extended to identify the case and consume the results in order and sort them based on the given columns. The underlying QueryContainer has been slightly modified to allow a view of the underlying values being extracted as the columns used for sorting might not be requested by the user. The PR also adds minor tweaks, mainly related to tree output. Close elastic#35118 (cherry picked from commit 783c9ed)

Introduce client-side sorting of groups based on aggregate functions. To allow this, the Analyzer has been extended to push down to underlying Aggregate, aggregate function and the Querier has been extended to identify the case and consume the results in order and sort them based on the given columns. The underlying QueryContainer has been slightly modified to allow a view of the underlying values being extracted as the columns used for sorting might not be requested by the user. The PR also adds minor tweaks, mainly related to tree output. Close #35118 (cherry picked from commit 783c9ed)

costin added >feature v7.0.0 :Analytics/SQL SQL querying v6.7.0 labels Jan 30, 2019

costin requested review from astefan and matriv January 30, 2019 16:24

Pass column size explicitly since the schema is lost on paging calls

d3e3174

matriv reviewed Jan 31, 2019

View reviewed changes

astefan reviewed Jan 31, 2019

View reviewed changes

costin added 8 commits February 1, 2019 01:14

First round of feedback

4e69a52

Merge remote-tracking branch 'remotes/upstream/master' into fix-35118

19cc344

Address feedback

a745da8

wip

a4d3434

Merge branch 'master' into fix-35118

b4dc577

wip

c772e69

Merge branch 'master' into fix-35118

8b4792a

Address feedback

292cdef

costin force-pushed the fix-35118 branch from 4520316 to 292cdef Compare February 1, 2019 16:43

matriv approved these changes Feb 1, 2019

View reviewed changes

astefan approved these changes Feb 1, 2019

View reviewed changes

costin added 2 commits February 1, 2019 20:11

Tie breaker tests

492b3d3

Merge remote-tracking branch 'remotes/upstream/master' into fix-35118

cd2239f

costin merged commit 783c9ed into elastic:master Feb 1, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

matriv mentioned this pull request May 9, 2019

SQL: Suggesting a different approach to error messages #31860

Closed

costin deleted the fix-35118 branch July 6, 2020 16:33


		// 2. find first Aggregate child and update it

		final AtomicBoolean found = new AtomicBoolean(false);

SQL: Allow sorting of groups by aggregates #38042

SQL: Allow sorting of groups by aggregates #38042

Conversation

costin commented Jan 30, 2019

elasticmachine commented Jan 30, 2019

costin commented Jan 30, 2019

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

costin Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Jan 31, 2019

astefan commented Jan 31, 2019

Choose a reason for hiding this comment

matriv commented Jan 31, 2019

costin commented Jan 31, 2019

matriv commented Jan 31, 2019

costin commented Feb 1, 2019

costin commented Feb 1, 2019

costin commented Feb 1, 2019

matriv left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

costin commented Feb 1, 2019

matriv Jan 31, 2019 •

edited

Loading

costin Jan 31, 2019 •

edited

Loading