-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make dimension column extensible with COMPLEX type #10277
Conversation
Cool 👍 What do you think about deprecating the |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm 🤘
* | ||
* If ValueType is COMPLEX, then the typeName associated with it. | ||
*/ | ||
String getTypeName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
// 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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
not stale |
This issue is no longer marked as stale. |
this is ready to review/merge, no longer wip. |
@@ -58,6 +58,12 @@ public ColumnBuilder setType(ValueType type) | |||
return this; | |||
} | |||
|
|||
public ColumnBuilder setTypeName(String typeName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this should be setComplexTypeName
probably to match capabilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, yeah updated, I missed it.
* 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(..)
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()
inColumnCapabilities
interface and to addDimensionHandlerUtils.registerDimensionHandlerProvider(String typeName, DimensionHandlerProvider)
to enable extensions to register custom type specificDimensionHandler
, very similar to existingComplexMetrics.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,..)
andDimensionHandlerUtils.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:
Key changed/added classes in this PR
ColumnCapabilities
DimensionHandlerUtils