Improvements/add testcode modify cadinality #81
Conversation
Each individual element of the "fields" list can be a String or DimensionSpec. A String dimension in the fields list is equivalent to a DefaultDimensionSpec (no transformations).
…m json when index is 0
There was a problem hiding this comment.
Probably it would deserve multiple PR so I would have merged the index & lookup dimension quickly since I have some reserve with cardinality change ("method polymorphism").
See https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/query/aggregation/cardinality/CardinalityAggregatorFactory.java#L118
Probably we should enforce DimensionSpec type here.
Maybe we could add an extra method SetFieldNames that would be an adapter for convenience but I don't even think we should. I would always tend to go with the strongly typed way instead of the shorthand.
What do you think ?
builder/dimension/lookup.go
Outdated
| @@ -37,6 +37,11 @@ func (l *Lookup) SetOutputName(outputName string) *Lookup { | |||
| return l | |||
| } | |||
|
|
|||
| func (l *Lookup) SetDimension(dimensionName string) *Lookup { | |||
There was a problem hiding this comment.
better name it dimension instead of dimensionName since it's what we've done everywhere (just like druid own codebase @JsonProperty("dimension") String dimension, https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/query/dimension/LookupDimensionSpec.java)
There was a problem hiding this comment.
Thanks for the detailed review, @jbguerraz
I completely agree with your thoughts. I hadn't thought of specifying it until I looked at the Java source code. I created a DimensionSpec interface and modified the Fields variable of cadinality. Please check if it matches your idea.
type DimensionSpec interface {
Dimension
GetDimension() string
GetOutputName() string
GetOutputType() types.OutputType
GetExtractionFn() ExtractionFn // Deprecated
}
Hello, I modified some code and added test code. #31
Changed the type of cardinality's fieid([]string -> []interface{}). According to the documentation
Each individual element of the "fields" list can be a String or DimensionSpecRemove omitempty of index of substring - If omitempty is present, it disappears from json when index is 0
add SetDimension method of lookup - I wrote a test code with the example code in the official documentation, but it didn't work properly, so I modified it.
Additionally, I wrote test codes for cardinality, extraction, javascript, and lookup.