Add DimensionSchema for specifying dimension types and properties #75
Conversation
| @JsonSubTypes.Type(name = "string", value = StringDimensionSchema.class), | ||
| @JsonSubTypes.Type(name = "long", value = LongDimensionSchema.class), | ||
| @JsonSubTypes.Type(name = "float", value = FloatDimensionSchema.class), | ||
| @JsonSubTypes.Type(name = "spatial", value = NewSpatialDimensionSchema.class), |
There was a problem hiding this comment.
also, I thought spatial was going to be an index, not a column type
There was a problem hiding this comment.
@fjy Added a comment on the intended approach/future direction for NewSpatialDimensionSchema:
/**
* NOTE:
* This class should be deprecated after Druid supports configurable index types on dimensions.
* When that exists, this should be the implementation: https://github.com/druid-io/druid/issues/2622
*
* This is a stop-gap solution to consolidate the dimension specs and remove the separate spatial
* section in DimensionsSpec.
*/
There was a problem hiding this comment.
@jon-wei we should add new dimension schema whenever adding new type. can we just use type field and validate?
There was a problem hiding this comment.
@navis What kind of validation did you have in mind?
|
👍 |
| protected DimensionSchema(String name) | ||
| { | ||
| Preconditions.checkArgument(name != null, "Dimension name cannot be null."); | ||
| this.name = name; |
There was a problem hiding this comment.
can just say this.name = Preconditions.checkNotNull(name, "msg")
jon-wei
force-pushed
the
dim_schema
branch
3 times, most recently
from
06970f8
to
b1dabe9
Compare
|
|
||
| public class FloatDimensionSchema extends DimensionSchema | ||
| { | ||
| private static final String TYPENAME = "float"; |
There was a problem hiding this comment.
can/should this use DimensionSchema.FLOAT_TYPE_NAME?
There was a problem hiding this comment.
@himanshug changed these to use the statics from DimensionSchema, thanks
| private final Map<String, DimensionSchema> dimensionSchemaMap; | ||
|
|
||
| @Deprecated | ||
| private List<SpatialDimensionSchema> spatialDimensions; |
There was a problem hiding this comment.
not sure why we need to retain this global variable, eventually it is set to null. i see we use it in verify() but it should be possible to adjust that.
There was a problem hiding this comment.
@himanshug got rid of that variable
|
@jon-wei looks good over all after few more comments. |
|
👍 (pls squash the commits) |
|
@fjy @himanshug squashed, thanks for the review! |
Add DimensionSchema for specifying dimension types and properties
This PR changes the way dimensions are specified in the DimensionsSpec.
Instead of a String name, a new DimensionSchema class has been introduced, which specifies dimension type and additional properties.
This change is made to support long and float-typed dimensions within Druid.
The user can still pass in String names into the DimensionsSpec; this will create the default String-type dimension.
This PR also deprecates the "spatialDimensions" field; spatial dimensions are now represented with a subclass of DimensionSchema, and stored in the main "dimensions" list.
The user can still use the old "spatialDimensions" field; spatial dims within that list will be converted to the new form.