SQL: Enable the InnerAggregates inside PIVOT #65792

palesz · 2020-12-02T23:26:41Z

Remove the limitation of not being able use InnerAggregate inside
PIVOT (aggregations using extended and matrix stats)
Integration tests that cover the extended stats and the matrix stats aggregations

* Remove the limitation of not being able use `InnerAggregate` inside PIVOT (aggregations using extended and matrix stats)

elasticmachine · 2020-12-02T23:26:43Z

Pinging @elastic/es-ql (Team:QL)

astefan · 2020-12-03T06:18:11Z

Why were the aggregates not supported inside PIVOT and what changed that made them now useable there?

palesz · 2020-12-03T15:50:54Z

The limitation was introduced as part of the original PIVOT implementation: #46489 (comment) , d668751 .
The comment from @costin mentions that after the NamedExpression refactor (#49693), probably should not be issues with the InnerAggregates inside PIVOT.

Note: I will beef up the tests on this, and add extra, more complicated test-cases specifically mentioned in the original comment by @costin. For example: ROUND(SUM_OF_SQUARES(...)).

costin

The reviewers list is incomplete

costin · 2020-12-03T16:34:45Z

Note: I will beef up the tests on this, and add extra, more complicated test-cases specifically mentioned in the original comment by @costin. For example: ROUND(SUM_OF_SQUARES(...)).

I'm in favor of removing code however when dealing with known issues, the tests are critical to assess that there are no regressions. In fact, for this type of PR they become the main part - without them this PR is incomplete.

costin · 2020-12-03T16:31:49Z

x-pack/plugin/sql/qa/server/src/main/resources/pivot.csv-spec

@@ -201,6 +201,29 @@ null                 |10044          |Mingsen        |F              |1994-05-21
 // end::sumWithoutSubquery
 ;

+sumWithInnerAggregateSumOfSquares
+schema::birth_date:ts|emp_no:i|first_name:s|gender:s|hire_date:ts|last_name:s|1:d|2:d
+SELECT * FROM test_emp PIVOT (SUM_OF_SQUARES(salary) FOR languages IN (1, 2)) LIMIT 5;


I'd like to see a test with multiple arguments that get folded into compound aggs, such as sum / min / max.

Do you mean multiple aggregations inside the PIVOT? Currently there is a check that we can only have a single aggregation inside the PIVOT, so we can't have PIVOT(MIN(salary), MAX(salary) FOR ...).

This change only lifts the limitation on the type of that single aggregation within the PIVOT, so aggregations that are translated into an InnerAggregate will be allowed.

Is there any validation or check for this (now that PostOptimizerTest has been removed)? this goes beyond the scope of this ticket so please raise a ticket if this functionality is missing or needs to be further investigated.

The PostOptimizerTests checked that the type of the aggregation cannot be one that results in an InnerAggregate vs the number of the aggregations inside the PIVOT.

There is already a check in the LogicalPlanBuilder that only allows one aggregation inside the PIVOT:

elasticsearch/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java

Line 186 in 2f5ab81

throw new ParsingException(source(pivotClause.aggs), "PIVOT currently supports only one aggregation, found [{}]",

I cannot see it tested in unit tests, but it is there and works. Added: #65894

Test that the `PIVOT` results in the same query as the `GROUP BY`. This should hold across all the `AggregateFunction`s we have.

palesz · 2020-12-03T23:35:06Z

x-pack/plugin/sql/qa/server/src/main/resources/pivot.csv-spec

+
+  client_port  |       'OK'       |    'Error'    
+---------------+------------------+---------------
+null           |2.002             |-0.0            


Created #65863

…aggregates

costin

Left another round of comments.

costin · 2020-12-04T11:33:36Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java

        String q = ee.toString().replaceAll("\\s+", "");
        assertThat(q, containsString("\"query\":{\"terms\":{\"keyword\":[\"A\",\"B\"]"));
        String a = ee.queryContainer().aggs().asAggBuilder().toString().replaceAll("\\s+", "");
        assertThat(a, containsString("\"terms\":{\"field\":\"bool\""));
        assertThat(a, containsString("\"terms\":{\"field\":\"keyword\""));
        assertThat(a, containsString("{\"avg\":{\"field\":\"int\"}"));
    }
+


It's better to get the list from the Function registry so that all existing aggs and future ones are picked up without having to manually update this test (since it is unlikely it will happen).

👍 Yep, that sounds like a better solution (was thinking about possible going with reflection, but this is better).

costin · 2020-12-04T11:36:47Z

x-pack/plugin/sql/qa/server/src/main/resources/pivot.csv-spec

@@ -201,6 +201,29 @@ null                 |10044          |Mingsen        |F              |1994-05-21
 // end::sumWithoutSubquery
 ;

+sumWithInnerAggregateSumOfSquares
+schema::birth_date:ts|emp_no:i|first_name:s|gender:s|hire_date:ts|last_name:s|1:d|2:d
+SELECT * FROM test_emp PIVOT (SUM_OF_SQUARES(salary) FOR languages IN (1, 2)) LIMIT 5;


Is there any validation or check for this (now that PostOptimizerTest has been removed)? this goes beyond the scope of this ticket so please raise a ticket if this functionality is missing or needs to be further investigated.

…aggregates

costin

LGTM

costin · 2020-12-07T12:13:06Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java

        String q = ee.toString().replaceAll("\\s+", "");
        assertThat(q, containsString("\"query\":{\"terms\":{\"keyword\":[\"A\",\"B\"]"));
        String a = ee.queryContainer().aggs().asAggBuilder().toString().replaceAll("\\s+", "");
        assertThat(a, containsString("\"terms\":{\"field\":\"bool\""));
        assertThat(a, containsString("\"terms\":{\"field\":\"keyword\""));
        assertThat(a, containsString("{\"avg\":{\"field\":\"int\"}"));
    }
+
+    public void testPivotHasSameQueryAsGroupBy() {
+        final Map<String, String> aggFnsWithMultipleArguments = Map.of(


You might want to use a different construct to ease backporting to 7.x due to the JDK difference

I was thinking about this. I would prefer using the new APIs on master and change during backporting to Java 8 compatible, so moving forward we use the newer APIs.

bpintea

LGTM

astefan

There is one issue that needs further analysis (added a comment for pivot.csv-spec). Still reviewing.

astefan · 2020-12-07T17:43:28Z

x-pack/plugin/sql/qa/server/src/main/resources/pivot.csv-spec

@@ -201,6 +201,51 @@ null                 |10044          |Mingsen        |F              |1994-05-21
 // end::sumWithoutSubquery
 ;

+sumWithInnerAggregateSumOfSquares
+schema::birth_date:ts|emp_no:i|first_name:s|gender:s|hire_date:ts|last_name:s|1:d|2:d
+SELECT * FROM test_emp PIVOT (SUM_OF_SQUARES(salary) FOR languages IN (1, 2)) LIMIT 5;


This query fails for me in a REST test (using a REST client): it's only returning 3 rows, whereas it should have returned 5. We need to look into why the test passes and/or why the REST call with the same query fails. @palesz please, open an issue for this investigation.

The JDBC integration tests fetch all the rows and follow the cursor (hence 5 rows shows up there). The tests are valid and successful, no issue there. Created #65982 to investigate this unexpected behaviour related to PIVOT+LIMIT.

astefan · 2020-12-07T21:21:12Z

LGTM otherwise.

* Remove the limitation of not being able to use `InnerAggregate` inside PIVOTs (aggregations using extended and matrix stats) * The limitation was introduced as part of the original `PIVOT` implementation in elastic#46489, but after elastic#49693 it could be lifted. * Test that the `PIVOT` results in the same query as the `GROUP BY`. This should hold across all the `AggregateFunction`s we have. (cherry-pick 67704b0)

* Remove the limitation of not being able to use `InnerAggregate` inside PIVOTs (aggregations using extended and matrix stats) * The limitation was introduced as part of the original `PIVOT` implementation in #46489, but after #49693 it could be lifted. * Test that the `PIVOT` results in the same query as the `GROUP BY`. This should hold across all the `AggregateFunction`s we have. (cherry-picked from 67704b0)

SQL: Enable the InnerAggregates inside PIVOT

63ffe2e

* Remove the limitation of not being able use `InnerAggregate` inside PIVOT (aggregations using extended and matrix stats)

palesz added >feature :Analytics/SQL SQL querying v8.0.0 Team:QL (Deprecated) Meta label for query languages team v7.11.0 labels Dec 2, 2020

palesz requested review from costin and astefan December 2, 2020 23:26

palesz mentioned this pull request Dec 3, 2020

SQL: Fix SUM(all zeroes) to return 0 instead of NULL #65796

Merged

Additional, more complicated testcases

a6208fb

costin requested changes Dec 3, 2020

View reviewed changes

palesz requested review from matriv and bpintea December 3, 2020 20:05

Test that PIVOT and GROUP BY has same query

19dc1c6

Test that the `PIVOT` results in the same query as the `GROUP BY`. This should hold across all the `AggregateFunction`s we have.

palesz commented Dec 3, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into feature/pivot-inner…

3867593

…aggregates

costin reviewed Dec 4, 2020

View reviewed changes

Andras Palinkas added 2 commits December 4, 2020 13:11

Utilized SQLFunctionRegistry to look up the AggregateFunctions

cca0ed7

Merge remote-tracking branch 'origin/master' into feature/pivot-inner…

8480e39

…aggregates

palesz requested a review from costin December 4, 2020 18:32

costin approved these changes Dec 7, 2020

View reviewed changes

bpintea approved these changes Dec 7, 2020

View reviewed changes

palesz merged commit 67704b0 into elastic:master Dec 7, 2020

palesz added the backport pending label Dec 7, 2020

astefan reviewed Dec 7, 2020

View reviewed changes

palesz deleted the feature/pivot-inneraggregates branch December 7, 2020 22:34

palesz removed the backport pending label Dec 9, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Enable the InnerAggregates inside PIVOT #65792

SQL: Enable the InnerAggregates inside PIVOT #65792

palesz commented Dec 2, 2020

elasticmachine commented Dec 2, 2020

astefan commented Dec 3, 2020

palesz commented Dec 3, 2020

costin left a comment

costin commented Dec 3, 2020

costin Dec 3, 2020

palesz Dec 3, 2020

costin Dec 4, 2020

palesz Dec 4, 2020

palesz Dec 3, 2020

costin left a comment

costin Dec 4, 2020

palesz Dec 4, 2020

costin Dec 4, 2020

costin left a comment

costin Dec 7, 2020

palesz Dec 7, 2020

bpintea left a comment

astefan left a comment

astefan Dec 7, 2020

palesz Dec 7, 2020

astefan commented Dec 7, 2020

SQL: Enable the InnerAggregates inside PIVOT #65792

SQL: Enable the InnerAggregates inside PIVOT #65792

Conversation

palesz commented Dec 2, 2020

elasticmachine commented Dec 2, 2020

astefan commented Dec 3, 2020

palesz commented Dec 3, 2020

costin left a comment

Choose a reason for hiding this comment

costin commented Dec 3, 2020

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

costin 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

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea left a 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

astefan commented Dec 7, 2020