SQL: Implement scripting inside aggs by matriv · Pull Request #55241 · elastic/elasticsearch

matriv · 2020-04-15T14:46:28Z

Implement the use of scalar functions inside aggregate functions.
This allows for complex expressions inside aggregations, with or without
GROUBY as well as with or without a HAVING clause. e.g.:

SELECT MAX(CASE WHEN a IS NULL then -1 ELSE abs(a * 10) + 1 END) AS max, b
FROM test
GROUP BY b
HAVING MAX(CASE WHEN a IS NULL then -1 ELSE abs(a * 10) + 1 END) > 5

Scalar functions are still not allowed for KURTOSIS and SKEWNESS as
this is currently not implemented on the Elasticsearch side.

Fixes: #29980
Fixes: #36865
Fixes: #37271

elasticmachine · 2020-04-15T14:46:30Z

Pinging @elastic/es-ql (:Query Languages/SQL)

costin

Left one comment

costin · 2020-04-15T16:36:12Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

So this code becomes:

Object argument = asFieldOrLiteralOrScript(c.field()); if (c.distinct()) { return new CardinalityAgg(id, argument); } else { return new FilterExistsAgg(id, argument); }

Implement the use of scalar functions inside aggregate functions. This allows for complex expressions inside aggregations, with or without GROUBY as well as with or without a HAVING clause. e.g.: ``` SELECT MAX(CASE WHEN a IS NULL then -1 ELSE abs(a * 10) + 1 END) AS max, b FROM test GROUP BY b HAVING MAX(CASE WHEN a IS NULL then -1 ELSE abs(a * 10) + 1 END) > 5 ``` Scalar functions are still not allowed for `KURTOSIS` and `SKEWNESS` as this is currently not implemented on the ElasticSearch side. Fixes: elastic#29980 Fixes: elastic#36865 Fixes: elastic#37271

costin

Looks good.
Left some comments regarding testing and potentially improvements in the agg hierarchy- if they are unclear, we can address that through a different PR.

costin · 2020-04-15T20:54:00Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/gen/script/Scripts.java

                DataTypes.BOOLEAN);
    }

+    public static ScriptTemplate isNotNullCardinality(ScriptTemplate script) {


This method seems to be used in one place as such extracting it in a different class doesn't add much value since there's no reuse.
The name also indicates that - a method specific to cardinality/count.

costin · 2020-04-15T20:54:17Z

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

+
+// aggregates with scalars
+aggregateFunctionsWithScalars
+SELECT MAX(CASE WHEN (salary - 10) > 70000 THEN (salary + 12345) * 1.2 ELSE (salary - 12345) * 2.7 END) AS "max",


costin · 2020-04-15T20:55:32Z

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

+
+
+// aggregates with scalars
+aggregateFunctionsWithScalars


Can you please add a couple of tests where the aggs with scalars are inside having?
Even better have scalars over aggs over scalars inside Having.

Also another that sorts by an aggregation with an internal scalar.

Can you please add a couple of tests where the aggs with scalars are inside having?
There is already:

https://github.com/elastic/elasticsearch/pull/55241/files/b8bc29f350f24b22bde8b9096a95d4273dd822fe#diff-002044c6b4e1de5a6960847971f85532R986

Even better have scalars over aggs over scalars inside Having.

I will add one with scalars also on top of the agg function.

Also another that sorts by an aggregation with an internal scalar.

I have already: https://github.com/elastic/elasticsearch/pull/55241/files/b8bc29f350f24b22bde8b9096a95d4273dd822fe#diff-002044c6b4e1de5a6960847971f85532R960

I have already:

I was thinking of a test with the agg declared just inside the order and not referencing an already declared aggregation.

costin · 2020-04-15T21:02:06Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

    static String field(AggregateFunction af) {
        Expression arg = af.field();
+        if (arg.foldable()) {
+            return String.valueOf(((Literal) arg).value());


String.valueOf(arg.fold())
There's rarely a reason to cast to a Literal

costin · 2020-04-15T21:03:24Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

+    }
+
+    private static Object getFieldOrLiteralOrScript(AggregateFunction af) {
+        if (isFieldOrLiteral(af)) {


return isFieldOrLiteral(af) ? field(af) : ((ScalarFunction) af.field()).asScript()

The logic from topAggsFieldOrScript and this method is similar and should be reused - I like the instanceof check for Scalar instead of the direct cast; a bit verbose but safe.

costin · 2020-04-15T21:04:22Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

+        return af.field().foldable() || af.field() instanceof FieldAttribute;
+    }
+
+    private static Object getFieldOrLiteralOrScript(AggregateFunction af) {


get prefix is for getter - use something like toField.. or asField..

costin · 2020-04-15T21:04:57Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

    }

-    private static String topAggsField(AggregateFunction af, Expression e) {
+    private static Object topAggsFieldOrScript(AggregateFunction af, Expression e) {


This can be moved under the TopHit agg class

Consolidated into one method.

costin · 2020-04-15T21:22:14Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Agg.java

+    private final ScriptTemplate scriptTemplate;

-    Agg(String id, String fieldName) {
+    Agg(String id, Object fieldOrScript) {


Thinking a bit more about this, a better solution might be creating a simple DTO specific for Agg that can hold either a String or a ScriptTemplate.
That one would handle the hashCode, equals, etc.
Further more maybe extend it to accept behavior by accepting a ValuesSourceAggregationBuilder and internally call either field or script based on its content.

Suggested change

Agg(String id, Object fieldOrScript) {

Agg(String id, AggTarget target) {

this.target = target;

}

...

class AggTarget {

private final String fieldName;

private final ScriptTemplate script;

...

ValuesSourceAggregationBuilder with(ValuesSourceAggregationBuilder aggBuilder) {

if (field != null) {

aggBuilder.field(field);

}

else {

aggBuilder.script(scriptTemplate.asScript());

}

}

}

This would improve LeafAgg which would improve toBuilder and call AggTarget so that subclasses would only have to create the ValueSourceAggregationBuilder. In fact we could just declare them as a method reference for a string:

public abstract class LeafAgg extends Agg { LeafAgg(String id, AggTarget target) { super(id, target); } ValuesSourceAggregationBuilder builder() { return aggBuilder.apply(id()).with(aggTarget); } protected abstract Function<String, ValuesSourceAggregationBuilder> aggBuilder();

so that subclasses would now look like:

public class CardinalityAgg extends LeafAgg { public CardinalityAgg(String id, AggTarget aggTarget) { super(id, aggTarget); } @Override ValuesSourceAggregationBuilder aggBuilder() { return AggregationBuilders::cardinality; } }

Thx @costin, It's cleaner this way!

costin · 2020-04-16T10:20:07Z

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

+
+
+// aggregates with scalars
+aggregateFunctionsWithScalars


I have already:

I was thinking of a test with the agg declared just inside the order and not referencing an already declared aggregation.

costin

👍

costin · 2020-04-16T12:11:35Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Agg.java

    @Override
    public int hashCode() {
-        return Objects.hash(id, fieldName, scriptTemplate);
+        return Objects.hash(id) + target.hashCode();


Objects.hash(id, target)

costin

AggSource conveys better the intent than AggTarget.

costin · 2020-04-16T12:16:21Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/LeafAgg.java

    abstract AggregationBuilder toBuilder();
+
+    @SuppressWarnings("rawtypes")
+    protected ValuesSourceAggregationBuilder addFieldOrScript(ValuesSourceAggregationBuilder builder) {


The name is leaking implementation details - why not addTarget()?

Yep, forgot to rename that, thx.

astefan

LGTM. This PR will make a lot of users happy.
Left few minor comments.

astefan · 2020-04-16T12:15:46Z

docs/reference/sql/functions/aggs.asciidoc

+not allowed and an error is returned:
+[source, sql]
+---------------------------------------
+ SELECT KURTOSIS(salary), gender FROM emp GROUP BY gender


Reading the NOTE I would have expected to see "KURTOSIS ... on top of scalar functions" but I'm seeing KURTOSIS applied on a field.

astefan · 2020-04-16T12:22:18Z

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

+
+aggregatesWithScalarsAndGroupByOrderByAggWithoutProjection
+schema::gender:s
+SELECT gender FROM test_emp GROUP BY gender ORDER BY MAX(salary % 100) DESC;


Please, add MAX(salary % 100) to the result.

This is a special case, requested by @costin, ORDER BY without having it in the projection.

astefan · 2020-04-16T12:23:56Z

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

+18601.965319409886|3.4461553130896095E10|3.460331137445282E8 |null
+17811.071545718776|1.2151168881502939E11|3.1723426960671306E8|F
+15904.093950318531|1.699198993070239E11 |2.529402043805585E8 |M
+;


I like the complexities of these tests, but the queries are too complex for me to assess that the example is relevant :-). I trust that you checked the numbers against the data and made sure everything is ok.

I've tested all the queries against PostgreSQL and the results are not always the same but very similar (double-rounding diffs).

astefan · 2020-04-16T12:34:29Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

+                return new MatrixStatsAgg(id, singletonList(field(m, m.field())));
+            }
+            throw new SqlIllegalArgumentException(
+                "Cannot use scalar functions or operators: [{}] in matrix stats " + "aggregate functions [KURTOSIS] and [SKEWNESS]",


Can this error message be improved?

why is it mentioning matrix stats, isn't this an implementation detail?

not a biggie and can be left as is, but can the message be more targeted to the exact aggregate function? (mention only KURTOSIS or SKEWNESS)

why is it mentioning matrix stats, isn't this an implementation detail?

Yep, I can remove that.

not a biggie and can be left as is, but can the message be more targeted to the exact aggregate function? (mention only KURTOSIS or SKEWNESS)

Not really, I've tried it, because we don't have the original function anymore here, only MatrixStats. I was considering checking that earlier on the functions themselves, but then resolveType() didn't seem to me a good place to add such a verification, since it's regarding the DataType and not if the arg is field or function.

This is better handled in the verifier by checking that Kurtosis & co (namely MatrixStatsEnclosed) don't have a scalar expression as argument.
Further more I would add an issue to extend the aggregation to support scripts not just fields.

Created issue: #55344

astefan · 2020-04-16T12:35:57Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/Agg.java

+    Agg(String id, AggTarget target) {
        this.id = id;
-        this.fieldName = fieldName;
+        Objects.requireNonNull(target, "AggTarget must not be null");


You could move this call as the first in the constructor, no?

costin · 2020-04-16T18:29:54Z

docs/reference/sql/functions/aggs.asciidoc

@@ -397,6 +397,16 @@ https://en.wikipedia.org/wiki/Kurtosis[Quantify] the shape of the distribution o
 include-tagged::{sql-specs}/docs/docs.csv-spec[aggKurtosis]


I think it's worth adding examples in the docs (either a dedicated one or for most aggregations) that shows aggregations alongside a scalar function as it might not be obvious, especially for existing users.

This would also address @astefan point of having a test that can be easily be verified.

bpintea

Looks great!

Avoid repetition of the aggregation builder setup Relates elastic#55241

Implement the use of scalar functions inside aggregate functions. This allows for complex expressions inside aggregations, with or without GROUBY as well as with or without a HAVING clause. e.g.: ``` SELECT MAX(CASE WHEN a IS NULL then -1 ELSE abs(a * 10) + 1 END) AS max, b FROM test GROUP BY b HAVING MAX(CASE WHEN a IS NULL then -1 ELSE abs(a * 10) + 1 END) > 5 ``` Scalar functions are still not allowed for `KURTOSIS` and `SKEWNESS` as this is currently not implemented on the ElasticSearch side. Fixes: #29980 Fixes: #36865 Fixes: #37271 (cherry picked from commit 506d1be)

Avoid repetition of the aggregation builder setup Relates #55241

Avoid repetition of the aggregation builder setup Relates #55241 (cherry picked from commit 6cfe130)

matriv added >feature :Analytics/SQL SQL querying v8.0.0 v7.8.0 labels Apr 15, 2020

matriv force-pushed the impl-29980 branch from 02eb795 to 57bd9c0 Compare April 15, 2020 14:48

costin reviewed Apr 15, 2020

View reviewed changes

matriv force-pushed the impl-29980 branch 2 times, most recently from 1ae462d to 8bbfaa1 Compare April 15, 2020 19:36

matriv force-pushed the impl-29980 branch from 8bbfaa1 to 1002af1 Compare April 15, 2020 19:43

matriv requested review from astefan, bpintea and costin April 15, 2020 19:43

matriv marked this pull request as ready for review April 15, 2020 19:44

matriv added 3 commits April 15, 2020 22:02

Avoid instantiating SqlFunctionRegistry multiple times

18b38d1

remove unused import

30f0be6

re-arrange imports

b8bc29f

costin approved these changes Apr 15, 2020

View reviewed changes

matriv added 2 commits April 16, 2020 10:58

Address comments

ea82403

fix check for matrix aggs

fabece7

costin approved these changes Apr 16, 2020

View reviewed changes

matriv added 2 commits April 16, 2020 13:00

Add one more ordering test

5dae3ae

use a POJO for fielname or script

2ef7067

costin approved these changes Apr 16, 2020

View reviewed changes

astefan approved these changes Apr 16, 2020

View reviewed changes

matriv added 3 commits April 16, 2020 14:43

rename class and method

86f701e

address more comments

4545879

fix docs

4dc7a81

costin reviewed Apr 16, 2020

View reviewed changes

bpintea approved these changes Apr 16, 2020

View reviewed changes

matriv added 2 commits April 16, 2020 21:49

address comments

a54145d

fix imports

126460b

matriv merged commit 506d1be into elastic:master Apr 17, 2020

matriv deleted the impl-29980 branch April 17, 2020 09:22

costin added a commit to costin/elasticsearch that referenced this pull request Apr 17, 2020

SQL: Streamline declaration of LeafAggs

f835ec6

Avoid repetition of the aggregation builder setup Relates elastic#55241

costin mentioned this pull request Apr 17, 2020

SQL: Streamline declaration of LeafAggs #55380

Merged

costin added a commit that referenced this pull request Apr 17, 2020

SQL: Streamline declaration of LeafAggs (#55380)

6cfe130

Avoid repetition of the aggregation builder setup Relates #55241

costin added a commit that referenced this pull request Apr 17, 2020

SQL: Streamline declaration of LeafAggs (#55380)

fc62619

Avoid repetition of the aggregation builder setup Relates #55241 (cherry picked from commit 6cfe130)

matriv added the release highlight label May 7, 2020

bpintea mentioned this pull request Dec 4, 2020

SQL: Using CASE expression in COUNT aggregate fails #65036

Closed

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

-    Agg(String id, Object fieldOrScript) {
+    Agg(String id, AggTarget target) {
+       this.target = target;
+    }
+    ...
+    class AggTarget {
+        private final String fieldName;
+        private final ScriptTemplate script;
+        ...
+        ValuesSourceAggregationBuilder with(ValuesSourceAggregationBuilder aggBuilder) {
+            if (field != null) {
+                 aggBuilder.field(field);
+             }
+             else {
+                 aggBuilder.script(scriptTemplate.asScript());
+              }
+        }
+    }

		@@ -397,6 +397,16 @@ https://en.wikipedia.org/wiki/Kurtosis[Quantify] the shape of the distribution o
		include-tagged::{sql-specs}/docs/docs.csv-spec[aggKurtosis]

Conversation

matriv commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 15, 2020

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin 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!

matriv Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin Apr 16, 2020 • edited by matriv Loading Uh oh! There was an error while loading. Please reload this page.

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 Apr 16, 2020 • edited by matriv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin 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!

astefan 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

matriv commented Apr 15, 2020 •

edited

Loading

matriv Apr 16, 2020 •

edited

Loading

costin Apr 16, 2020 •

edited by matriv

Loading

costin Apr 16, 2020 •

edited by matriv

Loading