-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add configurable truncation for string columns #19146
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
base: master
Are you sure you want to change the base?
Changes from all commits
ed7de0a
9958e34
9033b75
4d6bb1f
47a0684
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,7 @@ public class BuiltInTypesModule implements DruidModule | |
| */ | ||
| private static DimensionSchema.MultiValueHandling STRING_MV_MODE = DimensionSchema.MultiValueHandling.SORTED_ARRAY; | ||
| private static IndexSpec DEFAULT_INDEX_SPEC = IndexSpec.builder().build(); | ||
| private static int MAX_STRING_LENGTH = 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to set this to Integer max value? In case this is used elsewhere in the future there wouldn't need to explicit handling for 0 like you have in truncateIfNeeded
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are using NON_DEFAULT to not serialize the default value and I think for integer jackson's default is 0. If we set the MAX_STRING_LENGTH default as int max, it'll serialize this value for each dimension. |
||
|
|
||
| /** | ||
| * @return the configured string multi value handling mode from the system config if set; otherwise, returns | ||
|
|
@@ -89,6 +90,7 @@ public void configure(Binder binder) | |
| public SideEffectRegisterer initDimensionHandlerAndMvHandlingMode(DefaultColumnFormatConfig formatsConfig) | ||
| { | ||
| setStringMultiValueHandlingModeIfConfigured(formatsConfig.getStringMultiValueHandlingMode()); | ||
| setMaxStringLengthIfConfigured(formatsConfig.getMaxStringLength()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can take a look at druid.indexing.formats.stringMultiValueHandlingMode in BuiltInTypesModuleTest It would be good to have some test coverage for the new property
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added tests for this property. |
||
| setIndexSpecDefaults(formatsConfig.getIndexSpec()); | ||
| setNestedColumnDefaults(formatsConfig); | ||
|
|
||
|
|
@@ -128,6 +130,24 @@ private static void registerSerde() | |
| } | ||
| } | ||
|
|
||
| private static void setMaxStringLengthIfConfigured(@Nullable Integer maxStringLength) | ||
| { | ||
| if (maxStringLength != null) { | ||
| MAX_STRING_LENGTH = maxStringLength; | ||
| } | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| public static void setMaxStringLength(int maxStringLength) | ||
| { | ||
| MAX_STRING_LENGTH = maxStringLength; | ||
| } | ||
|
|
||
| public static int getMaxStringLength() | ||
| { | ||
| return MAX_STRING_LENGTH; | ||
| } | ||
|
|
||
| private static void setStringMultiValueHandlingModeIfConfigured(@Nullable String stringMultiValueHandlingMode) | ||
| { | ||
| if (stringMultiValueHandlingMode != null) { | ||
|
|
||
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.
drive by comment (i'll have a closer look at rest of PR later)
instead of adding additional arguments here, I was hoping to deprecate these arguments in favor of adding a column format spec similar to was done for auto/json columns in #17762, which could serve as a reference for how this should be wired up. I was planning to move the existing
createBitmapIndexandmultiValueHandlinginto such a spec, but just haven't got to it yet. I think this would be much cleaner and less disruptive to call sites going forward. It also allows wiring up toIndexSpecto be able to define job level defaults as a middle place between per column and system wide.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 for taking a look @clintropolis. Adding something like
StringCommonFormatColumnFormatSpecwould make it cleaner and makes sense to consolidate the configs there. Since it seems like a bigger refactor, does it make sense to do it in a follow up? Let me know what you think.