Adding new config for disabling group by on multiValue column by cryptoe · Pull Request #12253 · apache/druid

cryptoe · 2022-02-10T12:36:35Z

Description

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.

Key changed/added classes in this PR

GroupByQueryConfig
GroupByQueryEngineV2
GroupByQueryEngine

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.

gianm · 2022-02-10T20:49:31Z

+                                                                            query.getDimensions());
+                if (!(query.getContextValue(GroupByQueryConfig.CTX_KEY_ENABLE_MULTI_VALUE_UNNESTING, true))
+                    && !allSingleValueDims) {
+                  throw new ISE(


We should name the dimension so this error message is more actionable. People are going to need to know which dimension is causing the problem, so they can either remove it, set it to array mode, or process it in some other way.

How about:

Change hasNoExplodingDimensions to findExplodingDimensions (i.e., return the names of the exploding dimensions).

Change the error message to:

Encountered multi-value dimensions [%s] that cannot be processed with %s set to false. Consider changing these dimensions to arrays or setting %s to true.

Acked. Agreed such messaging would be better for the users

cryptoe

Thanks for the review @gianm . Please have a look at findAllProbableExplodingDimensions

cryptoe · 2022-02-11T09:38:36Z

+                                                                            query.getDimensions());
+                if (!(query.getContextValue(GroupByQueryConfig.CTX_KEY_ENABLE_MULTI_VALUE_UNNESTING, true))
+                    && !allSingleValueDims) {
+                  throw new ISE(


Acked. Agreed such messaging would be better for the users

gianm · 2022-02-11T14:31:39Z

+## Disable GroupBy on multivalue columns
+
+As grouping on multivalue columns causes implicit unnest, users can avoid this behaviour by setting
+`groupByEnableMultiValueUnnesting` in the query context to `false`. This will result the query to error out.


I think we should leave this out of the docs until we the array-based story is complete. Then we can document this, array types, array functions, etc.

Alternatively we can keep this documented but change the error message to not mention array-based dimensions. In that case, we can change the error message to this:

Encountered multi-value dimension [%s] that cannot be processed with %s set to false. Consider setting %s to true for unnesting behavior, or using an expression to create a scalar from the multi-value dimension.

For the docs, a couple style points:

The rest of this page uses second person ("you can…") rather than third ("users can…") so we should stick to that.

We usually use US spelling in documentation (e.g. behavior instead of behaviour).

So I'd go with:

You can disable the implicit unnesting behavior for groupBy by setting groupByEnableMultiValueUnnesting: false in your query context. In this mode, the groupBy engine will return an error instead of completing the query. This is a safety feature for situations where you believe that all dimensions are singly-valued and want the engine to reject any multi-valued dimensions that were inadvertently included.

Also, all documented groupBy parameters should be included in the groupbyquery.md document as well, under "GroupBy v2 configurations". So if you mention this here it should be mentioned in the main doc too.

In the latest draft I have kept the documentation and changed the error messaging a bit.
Update the "GroupBy v2 configurations" as well.

gianm · 2022-02-11T14:33:50Z

+   * mark columns with null capabilites as candidates for explosion.
   */
-  public static boolean hasNoExplodingDimensions(
+  public static List<String> findAllProbableExplodingDimensions(


Hmm, looking through this I just realized that this function can't tell if dimensions are definitely multi-valued or not. It can only tell if they might be multi-valued. (But they also might not be!)

So I think we should move the check; instead of checking here, we should check row by row as we actually run the query. If a multi-value row is ever encountered, then at that point we should throw the error.

Thanks for the catch. Updated the check while doing row by row iteration.

clintropolis · 2022-02-16T05:52:55Z


            if (doAggregate) {
+              // this check is done during the row aggregation as a dimension can become multi-value col if
+              // {@link org.apache.druid.segment.column.ColumnCapabilities} is unkown.


nit: javadoc links don't work here, maybe just say "column capabilities", also "unkown" -> "unknown"

clintropolis · 2022-02-16T05:53:10Z

        } else {
+          if (multiValuesSize > 1) {
+            // this check is done during the row aggregation as a dimension can become multi-value col if
+            // {@link org.apache.druid.segment.column.ColumnCapabilities} is unkown.


nit: same nit and typo

gianm · 2022-02-16T07:49:11Z

Looks good after the changes. Thanks!

Adding new config for disabling group by on multiValue column

203fc7a

clintropolis added Area - Querying Ease of Use labels Feb 10, 2022

gianm reviewed Feb 10, 2022

View reviewed changes

Addressing review comments and doc changes

f4f0740

cryptoe commented Feb 11, 2022

View reviewed changes

gianm reviewed Feb 11, 2022

View reviewed changes

cryptoe added 2 commits February 15, 2022 00:07

Addressing review comments and doc changes

2630874

Fixing static checks

dc09ce0

clintropolis approved these changes Feb 16, 2022

View reviewed changes

Fixing typos

e891646

gianm approved these changes Feb 16, 2022

View reviewed changes

abhishekagarwal87 merged commit 5794331 into apache:master Feb 16, 2022

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

abhishekagarwal87 mentioned this pull request May 25, 2022

[Draft] 0.23.0 Release notes #12510

Closed

Conversation

cryptoe commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

gianm commented Feb 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cryptoe commented Feb 10, 2022 •

edited

Loading