Grouping on arrays as arrays #12078

cryptoe · 2021-12-17T11:19:11Z

Description

Currently, grouping on multivalue columns inside druid work by exploding each value. From the druid docs :

For an example datasoure

{"timestamp": "2011-01-12T00:00:00.000Z", "tags": ["t1","t2","t3"]}  #row1
{"timestamp": "2011-01-13T00:00:00.000Z", "tags": ["t3","t4","t5"]}  #row2
{"timestamp": "2011-01-14T00:00:00.000Z", "tags": ["t5","t6","t7"]}  #row3
{"timestamp": "2011-01-14T00:00:00.000Z", "tags": []}                #row4

{
  "queryType": "groupBy",
  "dataSource": "test",
  "intervals": [
    "1970-01-01T00:00:00.000Z/3000-01-01T00:00:00.000Z"
  ],
  "granularity": {
    "type": "all"
  },
  "dimensions": [
    {
      "type": "default",
      "dimension": "tags",
      "outputName": "tags"
    }
  ],
  "aggregations": [
    {
      "type": "count",
      "name": "count"
    }
  ]
}

This query returns the following result:

[
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 1,
      "tags": "t1"
    }
  },
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 1,
      "tags": "t2"
    }
  },
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 2,
      "tags": "t3"
    }
  },
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 1,
      "tags": "t4"
    }
  },
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 2,
      "tags": "t5"
    }
  },
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 1,
      "tags": "t6"
    }
  },
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 1,
      "tags": "t7"
    }
  }
]

Notice that original rows are "exploded" into multiple rows and merged.

The goal of this PR is to group on multi value columns as arrays without exploding them. Please look at the virtual column and the dimensionSpec to see how we are activating this behavior. For eg:

{
  "queryType": "groupBy",
  "dataSource": "test",
  "intervals": [
    "1970-01-01T00:00:00.000Z/3000-01-01T00:00:00.000Z"
  ],
  "granularity": {
    "type": "all"
  },
  "virtualColumns" : [ {
    "type" : "expression",
    "name" : "v0",
    "expression" : "mv_to_array(\"tags\")",
    "outputType" : "ARRAY<STRING>"
  } ],
  "dimensions": [
    {
      "type": "default",
      "dimension": "v0",
      "outputName": "tags"
      "outputType":"ARRAY<STRING>"
    }
  ],
  "aggregations": [
    {
      "type": "count",
      "name": "count"
    }
  ]
}

will return with the following results

[
 {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 1,
      "tags": "[]"
    }
  },
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 1,
      "tags": "["t1","t2","t3"]"
    }
  },
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 1,
      "tags": "[t3","t4","t5"]"
    }
  },
  {
    "timestamp": "1970-01-01T00:00:00.000Z",
    "event": {
      "count": 2,
      "tags": "["t5","t6","t7"]"
    }
  }
]

To activate this behavior in SQL we use a new mv_to_array which takes in a multiValue/String column.

select mv_to_array[tags] , count(*) from inline_data group by 1

Core Engine changes

In druid, we were coercing array's to string in the calcites layer. With this PR, we have removed the coercion and arrays are being passed as arrays to the native layer.

The idea was to copy concepts from how DictionaryBuildingStringGroupByColumnSelectorStrategy generated int<-> List on the fly.
Some optimizations are done for Array[String] while generating the dictionary keeping in mind that we want to store each string only once on the heap and reference that with the integer. As we are dealing with arrays, we are referencing the indexed array with yet another int so that the least amount of heap is used when string cardinality is low.
So if we have 2 rows:

 [a,b]
 [b,c]

the global dictionary would look like this:

"a"<>1
"b"<>2
"c"<>3

and the corresponding arrays would look like

[1,2]<>1
[2,3]<>2

Also the inMemory structures needed to implement comparable and would be called per row. Hence ComparableStringArrays, ComparableIntArrays are coded in such a way that they don't have the overhead of Lists and Integers.

Key changed/added classes in this PR

ComparableStringArray.java
ComparableIntArray.java
ResultRowDeserializer.java
RowBasedGrouperHelper.java
ArrayConstructorOperatorConversion.java
ArrayStringGroupByColumnSelectorStrategy.java
ListGroupByColumnSelectorStrategy.java
ComparableList.java
Calcites.java
Function.java

To enable the legacy behavior, for whatever reason, one can set a runtime property
-Ddruid.expressions.processArraysAsMultiValueStrings=true while starting the broker.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
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 or updated version, license, or notice information in licenses.yaml
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.

2.Optimized inmem structs

FrankChen021 · 2021-12-17T13:01:44Z

I don't think putting the output format into the data source spec is a good design. Is it possible to extend current ResultFormat to meet your need?

cryptoe · 2021-12-19T18:18:42Z

Hey @FrankChen021 Thanks for looking into it. I am not sure that I understand your concern. We generally put the column type in the dimension spec no? Checkout the class DefaultDimensionSpec. I am using that.

The pr extends the functionality of the group by engine. The grouping keys themselves are changing. I am not sure how resultFormat would help here.

dbardbar · 2021-12-20T12:19:32Z

Just wondering - can't you achieve the same thing with MV_TO_STRING?
Also, have you given any thought about how your code will be generated from SQL?

cryptoe · 2021-12-20T13:39:08Z

@dbardbar Good Question. The goal here is to make druid SQL layer similar to other SQL layers. In traditional DB's like POSTGRES, the SQL construct of declaring an array is an 'array'. I am using that construct itself to hook the SQL layer with the native layer. Just finishing up some test cases for that.

dbardbar · 2021-12-21T07:03:00Z

@cryptoe - two questions

Would this work also for a VirtualColumn which produces a multi-value? Specifically, VirtualColumn of type "mv-filtered".
We needed the capability in this PR, but since it was missing we created another string dimension during ingestion, which holds the values of the MV column as a simple string. We then used that extra column in our GROUP BY, to achieve the same functionality as this PR. Do you think that your method would be comparable in performance?

cryptoe · 2021-12-21T13:34:28Z

1. Would this work also for a VirtualColumn which produces a multi-value? Specifically, VirtualColumn of type "mv-filtered".

Is it possible to share a sample Q here. I am trying to understand the use case here. Do you want to group by on array ?
https://github.com/apache/druid/pull/12078/files#diff-8bc53aec44924e671b3c6f6f34c6f0c499f873d9000649c47c237444707aea4bR975 . This PR does support virtual col's .

2. We needed the capability in this PR, but since it was missing we created another string dimension during ingestion, which holds the values of the MV column as a simple string. We then used that extra column in our GROUP BY, to achieve the same functionality as this PR. Do you think that your method would be comparable in performance?

If the original string dimension is of low cardinality, then IMHO this implementation will be slightly more performant with regards to memory usage across the historical's, brokers as we are doing optimizations on the way we lookup.

dbardbar · 2021-12-21T14:13:17Z

@cryptoe,

The use-case is that some of the values in the MV field are not interesting for a specific query, and we would like to ignore them for the purpose of the GROUP BY. They are kept there because those ignored tags might be used for filtering, or might be used for GROUP BY when performing a different query.

An example query, using the example data appearing at the top of this PR:

SELECT MV_FILTER_ONLY(tags, ARRAY['t3', 't4']), COUNT(*) FROM test GROUP BY 1

with your new code enabled, I would expect the following to return:

["t3"],       1            (from row1)
["t3", "t4"], 1            (from row2)
null,         2            (from row3+row4)

clintropolis

thanks for looking into this, super into being able to group on arrays natively 👍

processing/src/main/java/org/apache/druid/query/groupby/ResultRowDeserializer.java

.../org/apache/druid/query/groupby/epinephelinae/column/ArrayGroupByColumnSelectorStrategy.java

processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChestTest.java

clintropolis · 2021-12-22T03:54:24Z

...essing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java

@@ -1357,6 +1373,12 @@ private RowBasedKeySerdeHelper makeSerdeHelper(
    )
    {
      switch (valueType.getType()) {
+        case ARRAY:
+          return new ArrayRowBasedKeySerdeHelper(


this doesn't look like it handles all arrays, though using the TypeStrategy added in #11888 would maybe give the necessary comparators to handle the other types of arrays, but arrayDictionary would need to be able to hold any type of array, not just strings.

Acked. Working on it.

.../org/apache/druid/query/groupby/epinephelinae/column/ArrayGroupByColumnSelectorStrategy.java

processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java

clintropolis · 2021-12-22T04:14:47Z

sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java

+      if (plannerContext.getQueryContext()
+                        .getOrDefault(
+                            QueryContexts.ENABLE_UNNESTED_ARRAYS_KEY,
+                            QueryContexts.DEFAULT_ENABLE_UNNESTED_ARRAYS
+                        ).equals(Boolean.FALSE)) {
+        outputType = Calcites.getValueTypeForRelDataTypeFull(dataType);
+      } else {
+        outputType = Calcites.getColumnTypeForRelDataType(dataType);
+      }


if grouping on arrays supported all array types, the correct thing to do instead of a flag I think would to just be consolidate getColumnTypeForRelDataType and getValueTypeForRelDataTypeFull to remove the string coercion and let arrays stay as arrays. They are separate basically because we allow using array functions on string typed columns, but grouping on them requires they remain as string typed in the native layer, and if that were no longer true we wouldn't have to do this coercion any longer.

...java/org/apache/druid/sql/calcite/expression/builtin/ArrayConstructorOperatorConversion.java

processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java

processing/src/main/java/org/apache/druid/query/dimension/ColumnSelectorStrategyFactory.java

FrankChen021 · 2021-12-24T08:50:27Z

@cryptoe It's my fault to misunderstand your design. Just ignore the comment I left before.

cryptoe

Thanks for the Review

2. Removing ResultRowDeserializer

2. Removing dimension spec as part of columnSelector

cryptoe

Moving to PR out of draft

.../org/apache/druid/query/groupby/epinephelinae/column/ArrayGroupByColumnSelectorStrategy.java

2. Removing flag

cryptoe · 2022-01-14T04:05:32Z

Sure. Collating all the next steps:

Getting ordering on array grouping key to work
Null coercion
Optimized Limit pushdown in array comparators
mv_to_array to support expressions. Move to native cast.
Query context flag to not allow grouping on multi value columns
Refactor stuff to remove DimensionHandlerUtils returning a comparable
Tighter validation on matching dimension spec with column type

core/src/main/java/org/apache/druid/math/expr/ExpressionProcessingConfig.java

clintropolis · 2022-01-20T06:30:57Z

processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java

+            case DOUBLE:
+              return new ArrayDoubleGroupByColumnSelectorStrategy();
+            case FLOAT:
+              // Array<Float> not supported in expressions, ingestion


note to self, double check this is actually true at this layer (if it is not, it might be possibly handled with the Double strategy). While definitely true that FLOAT doesn't exist in expressions, and so within expressions there exists no float array, this type might still be specified by the SQL planner, whenever some float column is added into an array for example. I'm unsure if the expression selectors column capabilities would report ARRAY or ARRAY as the type of the virtual column, i know it coerces DOUBLE back to FLOAT when the planner requests FLOAT types, but don't think it does the same thing for ARRAY so, this is probably true.

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

* only coerce multi-value string null values when `ExpressionPlan.Trait.NEEDS_APPLIED` is set * correct return type inference for ARRAY_APPEND,ARRAY_PREPEND,ARRAY_SLICE,ARRAY_CONCAT * fix bug with ExprEval.ofType when actual type of object from binding doesn't match its claimed type

clintropolis

lot of stuff I can think to do as follow-ups, but the basic behavior lgtm, i think this is a good start 👍

As part of #12078 one of the followup's was to have a specific config which does not allow accidental unnesting of multi value columns if such columns become part of the grouping key. Added a config groupByEnableMultiValueUnnesting which can be set in the query context. The default value of groupByEnableMultiValueUnnesting is true, therefore it does not change the current engine behavior. If groupByEnableMultiValueUnnesting is set to false, the query will fail if it encounters a multi-value column in the grouping key.

cryptoe added 4 commits December 7, 2021 20:39

init multiValue column group by

2265991

Changing sorting to Lexicographic as default

fe3c9d9

Adding initial tests

d50cdfb

1.Fixing test cases adding

ba28913

2.Optimized inmem structs

Linking SQL layer to native layer

6cd8b65

cryptoe added 2 commits December 21, 2021 18:44

Adding multiDimension support to group by column strategy

23a44a4

Merge branch 'master' into group_by_arrays

e44258b

clintropolis added Area - Querying Area - SQL Release Notes labels Dec 21, 2021

clintropolis reviewed Dec 23, 2021

View reviewed changes

cryptoe commented Jan 5, 2022

View reviewed changes

cryptoe added 3 commits January 5, 2022 13:20

1. Removing array coercion in Calcite layer

5b6fa68

2. Removing ResultRowDeserializer

1. Supporting all primitive array types

3ca3aa0

2. Removing dimension spec as part of columnSelector

1. Supporting all primitive array types

80ff204

2. Removing dimension spec as part of columnSelector

cryptoe commented Jan 6, 2022

View reviewed changes

.../org/apache/druid/query/groupby/epinephelinae/column/ArrayGroupByColumnSelectorStrategy.java Outdated Show resolved Hide resolved

cryptoe marked this pull request as ready for review January 6, 2022 07:04

cryptoe added 5 commits January 6, 2022 15:50

1. Checkstyle things

aeba7c5

2. Removing flag

Minor naming things

7bb3df0

Merge branch 'master' of github.com:apache/druid into group_by_arrays

a4b7954

CheckStyle Things

6e3f806

Fixing test case

c6db303

cryptoe added 4 commits January 13, 2022 11:01

Fixing few failing tests

40b877e

Do no convert to topN Q incase of group by on array

57dcbe1

Fixing checkstyle

86fb48b

Fixing differences between jdk's class cast exception message

8619964

cryptoe added 4 commits January 14, 2022 21:15

1. Fixing ordering if the grouping key is an array

63081d3

Fixing DefaultLimitSpec

97350a5

Fixing CalciteArraysQueryTest

9b21150

Merge branch 'master' of github.com:apache/druid into group_by_arrays

ea5ba15

abhishekagarwal87 closed this Jan 17, 2022

abhishekagarwal87 reopened this Jan 17, 2022

Dummy commit for LGTM

1a6927f

cryptoe closed this Jan 18, 2022

cryptoe reopened this Jan 18, 2022

clintropolis reviewed Jan 24, 2022

View reviewed changes

clintropolis and others added 5 commits January 25, 2022 08:29

Review comments

1156180

Fixing test cases

d00e0b2

Fixing spot bugs

169f6e2

Fixing strict compile

e4e8d13

clintropolis approved these changes Jan 26, 2022

View reviewed changes

clintropolis merged commit 96b3498 into apache:master Jan 26, 2022

clintropolis mentioned this pull request Jan 28, 2022

add backwards compatibility mode for multi-value string array null value coercion #12210

Merged

6 tasks

This was referenced Feb 8, 2022

allow optimizing sql expressions and virtual columns #12241

Merged

fix bugs with multi-value string array expression handling #12244

Closed

cryptoe mentioned this pull request Feb 10, 2022

Adding new config for disabling group by on multiValue column #12253

Merged

9 tasks

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

abhishekagarwal87 mentioned this pull request May 11, 2022

[Draft] 0.23.0 Release notes #12510

Closed

clintropolis mentioned this pull request May 20, 2022

document arrays in sql #12549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grouping on arrays as arrays #12078

Grouping on arrays as arrays #12078

cryptoe commented Dec 17, 2021 •

edited

Loading

FrankChen021 commented Dec 17, 2021

cryptoe commented Dec 19, 2021 •

edited

Loading

dbardbar commented Dec 20, 2021

cryptoe commented Dec 20, 2021 •

edited

Loading

dbardbar commented Dec 21, 2021

cryptoe commented Dec 21, 2021

dbardbar commented Dec 21, 2021 •

edited

Loading

clintropolis left a comment

clintropolis Dec 22, 2021

cryptoe Jan 4, 2022

clintropolis Dec 22, 2021

cryptoe Jan 4, 2022

FrankChen021 commented Dec 24, 2021

cryptoe left a comment

cryptoe left a comment

cryptoe commented Jan 14, 2022 •

edited

Loading

clintropolis Jan 20, 2022

clintropolis left a comment

Grouping on arrays as arrays #12078

Grouping on arrays as arrays #12078

Conversation

cryptoe commented Dec 17, 2021 • edited Loading

Description

Core Engine changes

Key changed/added classes in this PR

FrankChen021 commented Dec 17, 2021

cryptoe commented Dec 19, 2021 • edited Loading

dbardbar commented Dec 20, 2021

cryptoe commented Dec 20, 2021 • edited Loading

dbardbar commented Dec 21, 2021

cryptoe commented Dec 21, 2021

dbardbar commented Dec 21, 2021 • edited Loading

clintropolis left a comment

Choose a reason for hiding this comment

clintropolis Dec 22, 2021

Choose a reason for hiding this comment

cryptoe Jan 4, 2022

Choose a reason for hiding this comment

clintropolis Dec 22, 2021

Choose a reason for hiding this comment

cryptoe Jan 4, 2022

Choose a reason for hiding this comment

FrankChen021 commented Dec 24, 2021

cryptoe left a comment

Choose a reason for hiding this comment

cryptoe left a comment

Choose a reason for hiding this comment

cryptoe commented Jan 14, 2022 • edited Loading

clintropolis Jan 20, 2022

Choose a reason for hiding this comment

clintropolis left a comment

Choose a reason for hiding this comment

cryptoe commented Dec 17, 2021 •

edited

Loading

cryptoe commented Dec 19, 2021 •

edited

Loading

cryptoe commented Dec 20, 2021 •

edited

Loading

dbardbar commented Dec 21, 2021 •

edited

Loading

cryptoe commented Jan 14, 2022 •

edited

Loading