[GitHub] [druid] jihoonson commented on a change in pull request #12226: array_concat_agg and array_agg support for array inputs

[GitBox <gi...@apache.org>]

jihoonson commented on a change in pull request #12226:
URL: https://github.com/apache/druid/pull/12226#discussion_r799757101



##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -502,15 +503,16 @@ public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<E
     @Override
     ExprEval doApply(ExprEval arrayExpr, ExprEval scalarExpr)
     {
+      final ExpressionType arrayType = arrayExpr.asArrayType();
       if (!scalarExpr.type().equals(arrayExpr.elementType())) {
         // try to cast
         ExprEval coerced = scalarExpr.castTo(arrayExpr.elementType());
-        return ExprEval.ofArray(arrayExpr.asArrayType(), add(arrayExpr.asArray(), coerced.value()).toArray());
+        return ExprEval.ofArray(arrayType, add(arrayType.getElementType(), arrayExpr.asArray(), coerced.value()).toArray());

Review comment:
       :+1: 

##########
File path: processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java
##########
@@ -347,10 +347,10 @@ public static boolean hasNoExplodingDimensions(
                 return false;
               }
 
-              // Now check column capabilities, which must be present and explicitly not multi-valued
+              // Now check column capabilities, which must be present and explicitly not multi-valued and not arrays
               final ColumnCapabilities columnCapabilities = inspector.getColumnCapabilities(dimension.getDimension());
-              return dimension.getOutputType().equals(ColumnType.STRING_ARRAY)
-                     || (columnCapabilities != null && columnCapabilities.hasMultipleValues().isFalse());
+              return dimension.getOutputType().isArray()

Review comment:
       Please fix the javadoc as well in https://github.com/apache/druid/pull/12226/files#diff-b31d54fc59e6e7f1443992705fbc05f034260d57652c479c98ca12a0a99f0d39R331.

##########
File path: docs/querying/sql.md
##########
@@ -376,9 +376,11 @@ In the aggregation functions supported by Druid, only `COUNT`, `ARRAY_AGG`, and
 |`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.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `''`|
 |`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.|N/A|
 |`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). If the aggregated array grows larger than the maximum size in bytes, the query will fail. 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.|`null`|
-|`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. If the aggregated array grows larger than the maximum size in bytes, the query will fail. 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.|`null`|
+|`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. If the aggregated array grows larger than the maximum size in bytes, the query will fail. Use of `ORDER BY` within the `ARRAY_AGG` expression is not currently supported, and the ordering of results will be based on the default for the element type.|`null`|
+|`ARRAY_CONCAT_AGG(expr, [size])`|Concatenates all array `expr` into a single ARRAY, with `size` in bytes limit on aggregation size (default of 1024 bytes).   Input `expr` _must_ be an array. Null `expr` will be ignored, but any null values within an `expr` _will_ be included in the resulting array. If the aggregated array grows larger than the maximum size in bytes, the query will fail. Use of `ORDER BY` within the `ARRAY_CONCAT_AGG` expression is not currently supported, and the ordering of results within the output array may vary depending on processing order.|`null`|

Review comment:
       I wonder if it will be useful to have some examples of `ARRAY_AGG`, `ARRAY_CONCAT_AGG`, and `STRING_AGG`. That way it could be more straight forward to understand what their inputs and outputs are.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org