make dimension column extensible with COMPLEX type #10277

himanshug · 2020-08-13T21:43:39Z

WIP tag is added because I am still in the process of testing it more thoroughly, However code in current patch is working in preliminary testing. I will also try and add some UTs for the changes. That said, code is totally reviewable.

Description

This patch adds support for adding COMPLEX type Dimension column via Extensions, similar support for Metric columns exist.

Main changes are to add String getTypeName() in ColumnCapabilities interface and to add DimensionHandlerUtils.registerDimensionHandlerProvider(String typeName, DimensionHandlerProvider) to enable extensions to register custom type specific DimensionHandler, very similar to existing ComplexMetrics.registerSerde(..)

After this patch, Users can have extensions with custom implementations of DimensionHandler, ComplexMetricSerde
and related interfaces to add COMPLEX type Dimension Column.
In addition to above, extension would also make ComplexMetrics.registerSerde(typeName,..) and DimensionHandlerUtils.registerDimensionHandlerProvider(typeName,..) to register custom type specific implementations.

Side-note: I am using this extensibility to add a Map<String, String> type Dimension column for various reasons which will later be added in a separate PR as contrib extension.

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.

Key changed/added classes in this PR

ColumnCapabilities
DimensionHandlerUtils

clintropolis · 2020-08-13T21:50:57Z

Cool 👍

What do you think about deprecating the ComplexMetric terminology and replacing it with ComplexType (e.g. ComplexMetricSerde -> ComplexTypeSerde, etc) since it isn't strictly associated to metrics? (I have been wanting to do this for a while, just haven't got to it).

himanshug · 2020-08-14T17:54:03Z

@clintropolis thanks, yes they could all be renamed , deserialization is only based on column type , nothing to do with dimension vs metric. however those renaming would be backward incompatible so better done in an independent PR.

clintropolis

overall lgtm 🤘

clintropolis · 2020-08-18T19:35:32Z

processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilities.java

+   *
+   * If ValueType is COMPLEX, then the typeName associated with it.
+   */
+  String getTypeName();


could this be getComplexTypeName to tie it more strongly to complex types? It would also match the aggregator factory changes in #9638 which I've started working on again recently. If you recall this discussion #9638 (comment), I decided to go ahead and rename it to getComplexTypeName there because there are already breaking changes with the new abstract methods, so might as well make it intuitive.

clintropolis · 2020-08-18T19:40:34Z

processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java

+      // Note: Things might be simpler if DimensionSchema had a method "getColumnCapabilities()" which could return
+      // type specific capabilities by itself. However, for various reasons, DimensionSchema currently lives in druid-core
+      // while ColumnCapabilities lives in druid-processing which makes that approach difficult.
+      ColumnCapabilitiesImpl capabilities = makeDefaultCapabilitiesFromValueType(type, dimSchema.getTypeName());


Apologies for the conflicts here, I made some semi disruptive changes to push ColumnCapabilities into the DimensionIndexer implementations so they can be more accurate. I think the changes should still be workable with your addition of DimensionHandlerProvider, just the dimension indexer it provides will need to provide the complex column capabilities.

it should be alright, I will update this PR later when I have my MapStringString extension ready/deployed and tested with changes in this patch. thanks for the heads up.

Change-Id: I6aad1f268c9304082806e2efb5f8fe9ec03a1aa6

Change-Id: I9707dd644b8d71030b74a8c1d6fff0c0020d960d

Change-Id: I4f900940120512b0e058bdbbf9cc0d3b17a1fea0

Change-Id: I687a2799c105cc4ec68bbbc51f613ee36075541a

Change-Id: I146f95a41b79d20edb1721be13f0e9641f788e0e

stale · 2020-11-07T22:09:23Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

himanshug · 2020-11-09T19:15:19Z

not stale

stale · 2020-11-09T19:15:21Z

This issue is no longer marked as stale.

himanshug · 2020-11-30T03:44:58Z

this is ready to review/merge, no longer wip.

clintropolis · 2020-11-30T11:23:50Z

processing/src/main/java/org/apache/druid/segment/column/ColumnBuilder.java

@@ -58,6 +58,12 @@ public ColumnBuilder setType(ValueType type)
    return this;
  }

+  public ColumnBuilder setTypeName(String typeName)


nit: this should be setComplexTypeName probably to match capabilities

thanks, yeah updated, I missed it.

…ame(..)

* make dimension column extensible with COMPLEX type * more changes Change-Id: I9707dd644b8d71030b74a8c1d6fff0c0020d960d * processing module changes for build fix Change-Id: I146f95a41b79d20edb1721be13f0e9641f788e0e * rename ColumnCapabilities.getTypeName() to getComplexTypeName() * rename ColumnBuilder.setTypeName(..) -> ColumnBuilder.setComplexTypeName(..)

make dimension column extensible with COMPLEX type

bb93426

himanshug added WIP Area - Extension labels Aug 13, 2020

clintropolis reviewed Aug 18, 2020

View reviewed changes

clintropolis mentioned this pull request Aug 18, 2020

refactor internal type system #9638

Merged

5 tasks

himanshug added 5 commits August 26, 2020 11:18

Merge remote-tracking branch 'apache/master' into extensible_dim

ef550dc

Change-Id: I6aad1f268c9304082806e2efb5f8fe9ec03a1aa6

more changes

a9eb0f8

Change-Id: I9707dd644b8d71030b74a8c1d6fff0c0020d960d

Merge remote-tracking branch 'apache/master' into extensible_dim

7535e3d

Change-Id: I4f900940120512b0e058bdbbf9cc0d3b17a1fea0

Merge remote-tracking branch 'apache/master' into extensible_dim

4998a0c

Change-Id: I687a2799c105cc4ec68bbbc51f613ee36075541a

processing module changes for build fix

026ca40

Change-Id: I146f95a41b79d20edb1721be13f0e9641f788e0e

clintropolis mentioned this pull request Sep 14, 2020

Vectorized variance aggregators #10390

Merged

6 tasks

stale bot added the stale label Nov 7, 2020

stale bot removed the stale label Nov 9, 2020

himanshug added 3 commits November 23, 2020 17:41

Merge remote-tracking branch 'origin/master' into extensible_dim

3cc4de7

rename ColumnCapabilities.getTypeName() to getComplexTypeName()

a2a7d9e

Merge remote-tracking branch 'origin/master' into extensible_dim

aef7890

himanshug removed the WIP label Nov 30, 2020

clintropolis reviewed Dec 2, 2020

View reviewed changes

himanshug added 2 commits December 2, 2020 16:42

Merge remote-tracking branch 'origin/master' into extensible_dim

5ccbf40

rename ColumnBuilder.setTypeName(..) -> ColumnBuilder.setComplexTypeN…

fa365ef

…ame(..)

clintropolis approved these changes Dec 3, 2020

View reviewed changes

himanshug merged commit 813e187 into apache:master Dec 3, 2020

himanshug mentioned this pull request Dec 4, 2020

map-string-string dimension column contrib extension #10628

Closed

9 tasks

jihoonson added this to the 0.21.0 milestone Jan 4, 2021

jihoonson mentioned this pull request Jan 13, 2021

[Draft] 0.21.0 Release Notes #10752

Closed

clintropolis mentioned this pull request Mar 11, 2021

Add support to first/last aggregators for numeric types during ingestion #10949

Closed

9 tasks

clintropolis mentioned this pull request Jun 23, 2022

Druid nested data columns #12695

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make dimension column extensible with COMPLEX type #10277

make dimension column extensible with COMPLEX type #10277

himanshug commented Aug 13, 2020 •

edited

Loading

clintropolis commented Aug 13, 2020

himanshug commented Aug 14, 2020

clintropolis left a comment

clintropolis Aug 18, 2020

himanshug Aug 18, 2020

himanshug Nov 30, 2020

clintropolis Aug 18, 2020

himanshug Aug 18, 2020

stale bot commented Nov 7, 2020

himanshug commented Nov 9, 2020

stale bot commented Nov 9, 2020

himanshug commented Nov 30, 2020

clintropolis Nov 30, 2020

himanshug Dec 3, 2020

make dimension column extensible with COMPLEX type #10277

make dimension column extensible with COMPLEX type #10277

Conversation

himanshug commented Aug 13, 2020 • edited Loading

Description

Key changed/added classes in this PR

clintropolis commented Aug 13, 2020

himanshug commented Aug 14, 2020

clintropolis left a comment

Choose a reason for hiding this comment

clintropolis Aug 18, 2020

Choose a reason for hiding this comment

himanshug Aug 18, 2020

Choose a reason for hiding this comment

himanshug Nov 30, 2020

Choose a reason for hiding this comment

clintropolis Aug 18, 2020

Choose a reason for hiding this comment

himanshug Aug 18, 2020

Choose a reason for hiding this comment

stale bot commented Nov 7, 2020

himanshug commented Nov 9, 2020

stale bot commented Nov 9, 2020

himanshug commented Nov 30, 2020

clintropolis Nov 30, 2020

Choose a reason for hiding this comment

himanshug Dec 3, 2020

Choose a reason for hiding this comment

himanshug commented Aug 13, 2020 •

edited

Loading