SQL: Add PIVOT support #46489

costin · 2019-09-09T13:07:51Z

Add initial PIVOT support for transforming a regular table into a
statistics table around an arbitrary pivoting column:

SELECT * FROM
(SELECT languages, country, salary, FROM mp)
PIVOT (AVG(salary) FOR countries IN ('NL', 'DE', 'ES', 'RO', 'US'))

In the current implementation PIVOT allows only one aggregation however
this restriction is likely to be lifted in the future.

Add initial PIVOT support for transforming a regular table into a statistics table around an arbitrary pivoting column: SELECT * FROM (SELECT languages, country, salary, FROM mp) PIVOT (AVG(salary) FOR countries IN ('NL', 'DE', 'ES', 'RO', 'US')) In the current implementation PIVOT allows only one aggregation however this restriction is likely to be lifted in the future.

elasticmachine · 2019-09-09T13:07:53Z

Pinging @elastic/es-search

costin · 2019-09-09T14:57:23Z

@elasticmachine run elasticsearch-ci/2

costin · 2019-09-09T15:05:07Z

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

@@ -3,7 +3,8 @@
 //


This file shouldn't be here - I'll remove it on the next commit once the feedback lands to avoid an extra build.

matriv

Great work overall!

I left some comments and here are a couple of more general ones:

Should we have some test similar to https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/cli/FetchSizeTestCase.java#L56
to double check that the paging works for large row numbers
Could you please add some tests in OptimizerTests to validate the QueryFolder & Optimizer folding changes?

matriv · 2019-09-09T14:31:05Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java

@@ -294,7 +294,7 @@ public void testConstantFolding() {
        // check now with an alias
        result = new ConstantFolding().rule(new Alias(EMPTY, "a", exp));
        assertEquals("a", Expressions.name(result));
-        assertEquals(5, ((Literal) result).value());
+        //assertEquals(5, ((Literal) result).value());


Remove comment please.

matriv · 2019-09-09T14:48:51Z

...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java

@@ -76,6 +76,9 @@ public static SearchSourceBuilder sourceBuilder(QueryContainer container, QueryB
        // set page size
        if (size != null) {
            int sz = container.limit() > 0 ? Math.min(container.limit(), size) : size;
+            // now take into account the the minimum page (if set)
+            int minSize = container.minPageSize();
+            sz = minSize > 0 ? (Math.max(sz / minSize, 1) * minSize) : sz;


Could you please add some comment which explains this calculation?

matriv · 2019-09-10T08:46:12Z

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

+;
+
+
+averageWithOneValueAndAlias


I think this is the same as: https://github.com/elastic/elasticsearch/pull/46489/files#diff-7cd262e8f087be8a976b37e385749ccdR29

matriv · 2019-09-10T08:49:17Z

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

+
+averageWithScalarOverAggregateAndFoldedValue
+schema::status:s|client_ip:s
+SELECT status, client_ip FROM logs 


Why don't we include the pivoted value here?

matriv · 2019-09-10T14:00:14Z

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

-                Alias a = (Alias) e;
-                return a.child().foldable() ? Literal.of(a.name(), a.child()) : a;
-            }
+            //            if (e instanceof Alias) {


Should be removed?

matriv · 2019-09-10T14:04:37Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java

+        if (groupingSet == null) {
+            AttributeSet columnSet = Expressions.references(singletonList(column));
+            // grouping can happen only on "primitive" fields, thus exclude multi-fields or nested docs
+            // the verifier enforces this rule so it does not catches folks by surprise


catches -> catch

matriv · 2019-09-10T14:10:50Z

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

+            UnresolvedAttribute column = new UnresolvedAttribute(source(pivotClause.column), visitQualifiedName(pivotClause.column));
+            List<NamedExpression> values = namedValues(pivotClause.aggs);
+            if (values.size() > 1) {
+                throw new ParsingException(source(pivotClause.aggs), "PIVOT currently supports only one aggregation, found [{}]",


I think ParsingException at this point is "late", but on the other hand SqlIllegalArgumentException doesn't seem appropriate. Should we introduce a new one, something with 'InvalidorRestricted` in the name?

Not necessary to do as part of this PR though.

Considering there's no typed consumer of these exceptions, the fact that it's a parsing exception vs restricted or whatever we get to call it, is about irrelevant semantics.
Further more if the client sees the message wrapped (inside the UI for example), the exception type is even further minimized.
My point is, the message sent to the user is important, not the exception in which is wrapped.

matriv · 2019-09-10T14:17:29Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java

+                    out.add(value.toAttribute().withDataType(agg.dataType()).withId(id));
+                }
+            }
+            // for multiple args, concat the function and the value


Shouldn't we do that also for one aggregate function?

No - this follows the Oracle convention which is the only one that supports multiple aggs inside the same pivot. Currently this branch is disabled due to a corner-case optimization that results in missing IDs.

costin · 2019-09-18T15:02:18Z

@astefan @matriv I've updated the PR, please take another look at it.

Thanks to a number of queries suggested by @astefan I concluded that supporting folding expressions inside values or functions inside columns leads to invalid queries due to the optimizer kicking in and creating different naming expressions which, due to the difference in ID, stops matching.

As such I've beefed up the Verifier and to prevent such cases - once the NamedExpression/ExpressionId refactoring will take place things, I'll revisit this.
Currently the folding of expressions leads to bugs since despite the name not changing, since most expressions are named, different ids are created leading to different identities.

costin · 2019-09-18T16:02:42Z

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

+    }
+
+    public void testPivotWithNull() {
+        assertEquals("1:85: Null not allowed as a PIVOT value",


@astefan Added test for forbidding null - it looks like it doesn't work since In doesn't allow it and it just removes it. Which kinda makes sense (null means the thing is missing).
This could be improved by either handling nulls as a separate filter or, preferably inside In directly.
@matriv thoughts?

When null is inside the value list of IN it will always yield NULL as a result which in the case of IN used as a filter in a normal WHERE clause means false and therefore it's eliminated. e.g.:

postgres=# select null in (null); ?column? ---------- (1 row)

If for pivot this case should yield true It should be treated differently but in my opinion not inside IN but somewhere else (e.g.: QueryFolder where we have the context information of IN used in a PIVOT query).

costin · 2019-09-18T16:03:08Z

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

+    }
+
+    public void testPivotWithFunctionInput() {
+        assertEquals("1:37: No functions allowed (yet); encountered [YEAR(date)]",


No functions allowed due to namedExpression issues.

costin · 2019-09-18T16:47:53Z

Added a minor update essentially preventing InnerAggs to be used. While the simple case can be addressed - SELECT .. PIVOT (SUM_OF_SQUARES(x)...) - the issue appears when the InnerAgg is hidden:
SELECT .. PIVOT (ROUND(SUM_OF_SQUARES(x))...).
Again, I'm expecting the NamedExpression refactor to address this by using function equality instead of ids.

Until then, I've prevented the use of such aggs inside the verifier.

matriv

LGTM. Thank you for the feedback addressing and once again great work!

matriv · 2019-09-23T07:18:29Z

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

@@ -152,6 +152,30 @@ SELECT * FROM (SELECT languages, gender, salary FROM test_emp) PIVOT (AVG(salary
 null           |48396.28571428572|62140.666666666664
 ;

+averageWithTwoValuesAndOrderDesc


matriv · 2019-09-23T07:19:34Z

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

+                else if (namedExpression.foldable()) {
+                    rawValues.add(Literal.of(namedExpression.name(), namedExpression));
+                }
+                // TOOD: same as above


typo -> TODO

matriv · 2019-09-23T07:24:41Z

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

@@ -203,18 +203,4 @@ null           |31070.0
 3              |26830.0        
 4              |24646.0        
 5              |23353.0        
-;
-
-innerAggPivot


Why did you remove this? Isn't this the case that is supported?

#46489 (comment)

Thx, I got confused and thought that we'd support this more trivial case.

Add initial PIVOT support for transforming a regular table into a statistics table around an arbitrary pivoting column: SELECT * FROM (SELECT languages, country, salary, FROM mp) PIVOT (AVG(salary) FOR countries IN ('NL', 'DE', 'ES', 'RO', 'US')) In the current implementation PIVOT allows only one aggregation however this restriction is likely to be lifted in the future. Also not all aggregations are working, in particular MatrixStats are not yet supported. (cherry picked from commit d912637)

* 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.

* 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)

costin added >feature :Analytics/SQL SQL querying v8.0.0 v7.5.0 labels Sep 9, 2019

costin requested review from astefan, bpintea and matriv September 9, 2019 13:07

costin commented Sep 9, 2019

View reviewed changes

matriv reviewed Sep 10, 2019

View reviewed changes

costin added 5 commits September 17, 2019 17:08

Merge branch 'master' into sql/piv

a0caa34

Address feedback

48c6a83

Address feedback

3f356ba

Restrict usage of PIVOT

7af8f37

Merge remote-tracking branch 'remotes/upstream/master' into sql/piv

d28984a

costin commented Sep 18, 2019

View reviewed changes

costin added 2 commits September 18, 2019 19:35

Prevent usage of InnerAgg for now

d668751

Remove unneeded file

854d09c

matriv approved these changes Sep 23, 2019

View reviewed changes

costin added 2 commits September 23, 2019 14:03

Fix typos

2606f36

Merge remote-tracking branch 'remotes/upstream/master' into sql/piv

7bbe026

costin merged commit d912637 into elastic:master Sep 23, 2019

costin mentioned this pull request Sep 23, 2019

SQL: Add docs for new PIVOT functionality #46979

Closed

costin deleted the sql/piv branch September 23, 2019 18:04

palesz mentioned this pull request Dec 3, 2020

SQL: Enable the InnerAggregates inside PIVOT #65792

Merged

palesz mentioned this pull request Dec 7, 2020

SQL: Enable the InnerAggregates inside PIVOT (#65792) #65987

Merged

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

		;


		averageWithOneValueAndAlias

SQL: Add PIVOT support #46489

SQL: Add PIVOT support #46489

Uh oh!

Conversation

costin commented Sep 9, 2019

Uh oh!

elasticmachine commented Sep 9, 2019

Uh oh!

costin commented Sep 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin commented Sep 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin commented Sep 18, 2019

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!