add ability to customize json (and auto) column physical storage details on per column basis#17762
Conversation
…lumn physical storage details on per column basis changes: * NestedDataColumnSchema and AutoTypeColumnSchema now accept a NestedCommonFormatColumn as a new 'columnFormatSpec' property, which allows customizing how json and auto columns are compressed, how strings are encoded in dictionaries, etc, on a per column level. The nested column format spec has several properties: objectKeysEncoding, objectStorageEncoding, objectStorageCompression, stringDictionaryEncoding, dictionaryEncodedColumnCompression, longColumnEncoding, longColumnCompression, doubleColumnCompression NestedCommonFormatColumn blends in segment level config from IndexSpec to populate any values not explicitly specified with segment level. * JSON field names can now be stored with front-coding with columnFormatSpec property objectKeysEncoding * Adds 'druid.indexing.formats.nestedColumnFormatSpec' to define a default NestedCommonFormatColumn
github-actions
Bot
added
Area - Batch Ingestion
Area - Querying
Area - Segment Format and Ser/De
Area - Ingestion
Area - MSQ
|
This pull request has been marked as stale due to 60 days of inactivity. |
|
This pull request has been marked as stale due to 60 days of inactivity. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to customize JSON (and auto) column physical storage details on a per-column basis by introducing a new configuration mechanism. It allows fine-grained control over compression, encoding, and storage settings for nested data columns.
- Introduces
NestedCommonFormatColumnFormatSpecfor per-column configuration of JSON/auto column storage - Updates
NestedDataColumnSchemaandAutoTypeColumnSchemato accept a newcolumnFormatSpecproperty - Adds global default configuration via
druid.indexing.formats.nestedColumnFormatSpec
Reviewed Changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/src/test/java/org/apache/druid/sql/calcite/util/TestDataBuilder.java | Updated to use new AutoTypeColumnSchema.of() factory method |
| sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java | Replaced explicit constructor calls with factory method for cleaner API |
| services/src/test/java/org/apache/druid/cli/DumpSegmentTest.java | Updated DefaultColumnFormatConfig constructor calls to include new nested format spec parameter |
| services/src/main/java/org/apache/druid/cli/DumpSegment.java | Fixed generic type declaration for better type safety |
| server/src/test/java/org/apache/druid/server/compaction/CompactionStatusTest.java | Added test cases for auto schema compaction matching/mismatching |
| server/src/main/java/org/apache/druid/server/compaction/CompactionStatus.java | Enhanced dimension comparison logic to handle effective schemas and format specs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| final CompressedNestedDataComplexColumn<?, ?> nestedDataColumn = | ||
| (CompressedNestedDataComplexColumn<?, ?>) baseColumn; |
There was a problem hiding this comment.
[nitpick] The cast from wildcard generic type <?> to <?, ?> is redundant and adds no type safety. Consider using a single wildcard CompressedNestedDataComplexColumn<?> or removing the explicit type parameters altogether.
| final CompressedNestedDataComplexColumn<?, ?> nestedDataColumn = | |
| (CompressedNestedDataComplexColumn<?, ?>) baseColumn; | |
| final CompressedNestedDataComplexColumn<?> nestedDataColumn = | |
| (CompressedNestedDataComplexColumn<?>) baseColumn; |
| return CHECKS.stream() | ||
| .map(f -> f.apply(evaluator)) | ||
| .filter(status -> !status.isComplete()) | ||
| .findFirst().orElse(COMPLETE); |
There was a problem hiding this comment.
[nitpick] The formatting change here appears to be a style-only modification without functional impact. Consider maintaining consistency with the existing codebase formatting patterns unless there's a specific reason for this change.
| return CHECKS.stream() | |
| .map(f -> f.apply(evaluator)) | |
| .filter(status -> !status.isComplete()) | |
| .findFirst().orElse(COMPLETE); | |
| return CHECKS.stream().map(f -> f.apply(evaluator)) | |
| .filter(status -> !status.isComplete()) | |
| .findFirst().orElse(COMPLETE); |
There was a problem hiding this comment.
oh no, the bots are nitpicking now? 😆 we are doomed
| return dimensionSchemas.stream() | ||
| .map(dim -> dim.getEffectiveSchema(indexSpec)) | ||
| .collect(Collectors.toList()); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| final List<String> partitionsDimensions = ((DimensionRangePartitionsSpec) partitionsSpec).getPartitionDimensions(); | ||
| return dimensionSchemas.stream() | ||
| .filter(dim -> !partitionsDimensions.contains(dim.getName())) | ||
| .collect(Collectors.toList()); | ||
| .filter(dim -> !partitionsDimensions.contains(dim.getName())) | ||
| .map(dim -> dim.getEffectiveSchema(indexSpec)) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
[nitpick] The logic for transforming dimension schemas is duplicated below. Consider extracting this transformation into a private helper method to reduce code duplication and improve maintainability.
| return dimensionSchemas.stream() | |
| .map(dim -> dim.getEffectiveSchema(indexSpec)) | |
| .collect(Collectors.toList()); | |
| } | |
| return null; | |
| } | |
| final List<String> partitionsDimensions = ((DimensionRangePartitionsSpec) partitionsSpec).getPartitionDimensions(); | |
| return dimensionSchemas.stream() | |
| .filter(dim -> !partitionsDimensions.contains(dim.getName())) | |
| .collect(Collectors.toList()); | |
| .filter(dim -> !partitionsDimensions.contains(dim.getName())) | |
| .map(dim -> dim.getEffectiveSchema(indexSpec)) | |
| .collect(Collectors.toList()); | |
| return transformDimensionSchemas(dimensionSchemas, indexSpec); | |
| } | |
| return null; | |
| } | |
| final List<String> partitionsDimensions = ((DimensionRangePartitionsSpec) partitionsSpec).getPartitionDimensions(); | |
| return transformDimensionSchemas( | |
| dimensionSchemas.stream() | |
| .filter(dim -> !partitionsDimensions.contains(dim.getName())) | |
| .collect(Collectors.toList()), | |
| indexSpec | |
| ); | |
| } | |
| private static List<DimensionSchema> transformDimensionSchemas( | |
| final List<DimensionSchema> dimensionSchemas, | |
| final IndexSpec indexSpec | |
| ) | |
| { | |
| return dimensionSchemas.stream() | |
| .map(dim -> dim.getEffectiveSchema(indexSpec)) | |
| .collect(Collectors.toList()); |
| throw new IAE("Trying to persist an empty index!"); | ||
| } | ||
|
|
||
| indexSpec = indexSpec.getEffectiveSpec(); |
There was a problem hiding this comment.
It would be good, at this spot or even closer to the actual usage site, to log the spec that is actually used. Ideally just once per segment, if the multiple-entry-point design of this class allows it.
|
Hey this is a new feature, I drafted some release note:
WDYT? |
…ils on per column basis (apache#17762) changes: * `NestedDataColumnSchema` and `AutoTypeColumnSchema` now accept a `NestedCommonFormatColumnFormatSpec` as a new 'columnFormatSpec' property, which allows customizing how json and auto columns are compressed, how strings are encoded in dictionaries, etc, on a per column level. The nested column format spec has several properties: `objectKeysEncoding`, `objectStorageEncoding`, `objectStorageCompression`, `stringDictionaryEncoding`, `dictionaryEncodedColumnCompression`, `longColumnEncoding`, `longColumnCompression`, `doubleColumnCompression` * JSON field names can now be stored with front-coding with `columnFormatSpec` property `objectKeysEncoding` * `IndexSpec` has a new `autoColumnFormatSpec` to set default `NestedCommonFormatColumnFormatSpec` * `IndexSpec` fields are now all nullable and NOT populated by default. A new `getEffectiveSpec()` method has been added to combine an `IndexSpec` with the system default `IndexSpec` and finally the hard-coded defaults if no override values have been specified at any level * Adds 'druid.indexing.formats.indexSpec' to define a system default `IndexSpec`. This system default will be blended with the job `IndexSpec` at segment building time, before `NestedCommonFormatColumn` blends in segment level config from the computed `IndexSpec` to populate any values not explicitly specified with segment level
Description
changes:
NestedDataColumnSchemaandAutoTypeColumnSchemanow accept aNestedCommonFormatColumnFormatSpecas a new 'columnFormatSpec' property, which allows customizing how json and auto columns are compressed, how strings are encoded in dictionaries, etc, on a per column level. The nested column format spec has several properties:objectKeysEncoding,objectStorageEncoding,objectStorageCompression,stringDictionaryEncoding,dictionaryEncodedColumnCompression,longColumnEncoding,longColumnCompression,doubleColumnCompressioncolumnFormatSpecpropertyobjectKeysEncodingIndexSpechas a newautoColumnFormatSpecto set defaultNestedCommonFormatColumnFormatSpecIndexSpecfields are now all nullable and NOT populated by default. A newgetEffectiveSpec()method has been added to combine anIndexSpecwith the system defaultIndexSpecand finally the hard-coded defaults if no override values have been specified at any levelIndexSpec. This system default will be blended with the jobIndexSpecat segment building time, beforeNestedCommonFormatColumnblends in segment level config from the computedIndexSpecto populate any values not explicitly specified with segment level