ARRAY_AGG sql aggregator function #11157

clintropolis · 2021-04-23T09:32:41Z

Description

This PR adds ARRAY_AGG to SQL, building it on top of the expression aggregator provided in #11104.

function	description
`ARRAY_AGG(expr, [size])`	Collects all values of `expr` into an ARRAY, including null values, with `size` in bytes limit on aggregation size (default of 1024 bytes). Use of `ORDER BY` within the `ARRAY_AGG` expression is not currently supported, and the ordering of results within the output array may vary depending on processing order.
`ARRAY_AGG(DISTINCT expr, [size])`	Collects all distinct values of `expr` into an ARRAY, including null values, with `size` in bytes limit on aggregation size (default of 1024 bytes) per aggregate. Use of `ORDER BY` within the `ARRAY_AGG` expression is not currently supported, and the ordering of results within the output array may vary depending on processing order.

Since this is an expression aggregator, it is unlikely to be the most optimal way to provide this functionality, But, it importantly makes it so that we can offer this functionality relatively easily, since arrays are more or less only supported in the expression system at this point, so it is still an improvement.

Until we update Apache Calcite to a newer version, we are unable to support the ORDER BY that some other databases which offer ARRAY_AGG have, due to https://issues.apache.org/jira/browse/CALCITE-4335. Upgrading Calcite to any newer version is tricky because of requirements of a newer Guava dependency.

We also will accept a non-standard 2 argument form of ARRAY_AGG as specified above, so that we can specify the maximum buffer aggregator size in bytes, which will be necessary until we have growable buffer aggregators.

I added a lot of SQL query tests to try and stress some areas of array handling in addition to testing this aggregator itself, including expected failure tests such as trying to group on the output of an array agg output of a subquery or join.

It was admittedly rather pleasing to see stuff like

SELECT dim2, COUNT(*)
FROM foo
WHERE ARRAY_CONTAINS((SELECT ARRAY_AGG(DISTINCT dim1) FROM foo WHERE dim1 is not null), dim1)
GROUP BY 1

able to just work out of the box after I added the aggregator function, so I consider array support overall to be progressing pretty smoothly.

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

jihoonson

Nice work 👍

jihoonson · 2021-04-29T04:25:54Z

docs/querying/sql.md

+|`ARRAY_AGG(expr)`|Collects all values of `expr` into an ARRAY, including null values, with the default limit on aggregation size of 1024 bytes. `ORDER BY` on the `ARRAY_AGG` expression is not currently supported.|
+|`ARRAY_AGG(DISTINCT expr)`|Collects all distinct values of `expr` into an ARRAY, including null values, with the default limit on aggregation size of 1024 bytes per aggregate. `ORDER BY` on the `ARRAY_AGG` expression is not currently supported.|
+|`ARRAY_AGG(expr, maxSizeBytes)`|Collects all values of `expr` into an ARRAY, including null values, with specified maximum byte size per aggregate. `ORDER BY` on the `ARRAY_AGG` expression is not currently supported.|
+|`ARRAY_AGG(DISTINCT expr, maxSizeBytes)`|Collects all distinct values of `expr` into an ARRAY, including null values, with specified maximum byte size per aggregate. `ORDER BY` on the `ARRAY_AGG` expression is not currently supported.|


Maybe better to consolidate these into 2, such as ARRAY_AGG(expr) and ARRAY_AGG(DISTINCT expr) with an optional size parameter as in https://github.com/apache/druid/pull/11157/files#diff-cada0a48f60f548ce82ef1008918e0aed6b3de0339312c662761855a35dde6c6L339.

👍 changed

jihoonson · 2021-04-29T04:30:31Z

sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArraySqlAggregator.java

+  @Override
+  public SqlAggFunction calciteFunction()
+  {
+    return new ArrayAggFunction();


nit: is it worth making it as a singleton?

jihoonson · 2021-04-29T23:30:06Z

sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArraySqlAggregator.java

+    final String initialvalue;
+    final ValueType elementType;
+    final ValueType druidType = Calcites.getValueTypeForRelDataTypeFull(aggregateCall.getType());
+    switch (druidType) {


This can throw NPE since druidType can be null.

good catch, modified to default to string if null

jihoonson · 2021-04-29T23:36:58Z

sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArraySqlAggregator.java

+      return null;
+    }
+
+    Integer maxSizeBytes = null;


nit: I think probably most people just use the byte size for this function, but it seems that it can be a string literal which will allow using the HumanReadable format?

Yeah good point. I was just matching other sketch aggregators, I think it makes sense to be able to handle it, but maybe it makes sense to do this to all of them in a follow-up, so they are consistent?

jihoonson

LGTM, but please address my comment on docs before you merge.

jihoonson · 2021-05-04T04:39:55Z

sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArraySqlAggregator.java

+      return null;
+    }
+
+    Integer maxSizeBytes = null;


jihoonson · 2021-05-04T04:50:45Z

docs/querying/sql.md

@@ -353,6 +353,8 @@ Only the COUNT aggregation can accept DISTINCT.
 |`ANY_VALUE(expr)`|Returns any value of `expr` including null. `expr` must be numeric. This aggregator can simplify and optimize the performance by returning the first encountered value (including null)|
 |`ANY_VALUE(expr, maxBytesPerString)`|Like `ANY_VALUE(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.|
 |`GROUPING(expr, expr...)`|Returns a number to indicate which groupBy dimension is included in a row, when using `GROUPING SETS`. Refer to [additional documentation](aggregations.md#grouping-aggregator) on how to infer this number.|
+|`ARRAY_AGG(expr, [size])`|Collects all values of `expr` into an ARRAY, including null values, with `size` in bytes limit on aggregation size (default of 1024 bytes). Use of `ORDER BY` within the `ARRAY_AGG` expression is not currently supported, and the ordering of results within the output array may vary depending on processing order.|


Can you please document that this function can return null when it processes no rows?

👍, but I will do that in #11188, which adds that documentation for all aggregator functions

Sounds good 👍

clintropolis · 2021-05-04T05:17:07Z

thanks for review @jihoonson

clintropolis added 2 commits April 23, 2021 02:12

ARRAY_AGG sql aggregator function

4951327

add javadoc

8cb659c

clintropolis added Area - Querying Area - SQL labels Apr 23, 2021

spelling

5cbe533

jihoonson added the Release Notes label Apr 28, 2021

jihoonson reviewed Apr 29, 2021

View reviewed changes

clintropolis added 5 commits April 30, 2021 00:38

Merge remote-tracking branch 'upstream/master' into array_agg

3ccc19b

review stuff, return null instead of empty when nil input

7c9d098

review stuff

9c1a7b9

Update sql.md

b84c69c

use type inference for finalize, refactor some things

04dcefe

This was referenced May 2, 2021

SQL timeseries no longer skip empty buckets with all granularity #11188

Merged

sql count(*) returns no results instead of 0 when no rows meet filter #11145

Closed

jihoonson approved these changes May 4, 2021

View reviewed changes

clintropolis merged commit 554f1ff into apache:master May 4, 2021

clintropolis deleted the array_agg branch May 4, 2021 05:17

clintropolis mentioned this pull request May 12, 2021

STRING_AGG SQL aggregator function #11241

Merged

5 tasks

clintropolis added this to the 0.22.0 milestone Aug 12, 2021

clintropolis mentioned this pull request Sep 3, 2021

[Draft] 0.22.0 Release Notes #11657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARRAY_AGG sql aggregator function #11157

ARRAY_AGG sql aggregator function #11157

clintropolis commented Apr 23, 2021 •

edited

Loading

jihoonson left a comment

jihoonson Apr 29, 2021

clintropolis Apr 30, 2021

jihoonson Apr 29, 2021

clintropolis Apr 30, 2021

jihoonson Apr 29, 2021

clintropolis Apr 30, 2021

jihoonson Apr 29, 2021

clintropolis Apr 30, 2021

jihoonson May 4, 2021

jihoonson left a comment

jihoonson May 4, 2021

jihoonson May 4, 2021

clintropolis May 4, 2021

jihoonson May 4, 2021

clintropolis commented May 4, 2021

ARRAY_AGG sql aggregator function #11157

ARRAY_AGG sql aggregator function #11157

Conversation

clintropolis commented Apr 23, 2021 • edited Loading

Description

jihoonson 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

jihoonson 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

clintropolis commented May 4, 2021

clintropolis commented Apr 23, 2021 •

edited

Loading