make double sum/min/max agg work on string columns #8243
Conversation
|
@clintropolis @jon-wei @jihoonson can one of you please review this PR ? |
👍, I should be able to have a look sometime today |
|
I will take a look this PR as well as the discussion issue this week. |
|
|
||
| import javax.annotation.Nullable; | ||
| import java.util.Collections; | ||
| import java.util.Comparator; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| public abstract class SimpleDoubleAggregatorFactory extends NullableAggregatorFactory<BaseDoubleColumnValueSelector> | ||
| public abstract class SimpleDoubleAggregatorFactory extends NullableAggregatorFactory<ColumnValueSelector> |
There was a problem hiding this comment.
Would you please add a javadoc explaining why the type for NullableAggregatorFactory is ColumnValueSelector instead of BaseDoubleColumnValueSelector?
| protected Aggregator factorize(ColumnSelectorFactory metricFactory, ColumnValueSelector selector) | ||
| { | ||
| if (shouldUseStringColumnAggregatorWrapper(metricFactory)) { | ||
| return new StringColumnDoubleAggregatorWrapper(selector, SimpleDoubleAggregatorFactory.this::buildAggregator, nullValue()); |
There was a problem hiding this comment.
This line exceeds 120 characters.
| ) | ||
| { | ||
| if (shouldUseStringColumnAggregatorWrapper(metricFactory)) { | ||
| return new StringColumnDoubleBufferAggregatorWrapper(selector, SimpleDoubleAggregatorFactory.this::buildBufferAggregator, nullValue()); |
There was a problem hiding this comment.
This line exceeds 120 characters.
| } | ||
| } | ||
|
|
||
| public static double tryParseDouble(Object val, double nullValue) |
There was a problem hiding this comment.
Could be moved to Numbers.
There was a problem hiding this comment.
Please annotate val with @Nullable.
There was a problem hiding this comment.
changed and moved to Numbers which is a better home for this.
| import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector; | ||
| import org.apache.druid.segment.BaseDoubleColumnValueSelector; | ||
|
|
||
| public class SettableValueDoubleColumnValueSelector implements BaseDoubleColumnValueSelector |
There was a problem hiding this comment.
Hmm, is it possible to use existing SettableDoubleColumnValueSelector? Looks feasible if the value type conversion is done in some other selector. Not sure this way is better though.
There was a problem hiding this comment.
they are somewhat different , and that is harder to use in this context. that said, I have added comments on this class to explain its use.
|
@jihoonson thanks for quick review |
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| public class StringColumnAggregationTest |
There was a problem hiding this comment.
nit: maybe it would be more clear if the class name indicates that it's testing double aggregation on string columns, such as DoubleAggregationOnStringColumnTest?
There was a problem hiding this comment.
there would be followup PR to aggregate string columns correctly in long/float versions as well and I plan to modify the test here with those additional aggregators . so , later it wouldn't be so double aggregation specific.
Partial work towards #8148 . There would be follow up PR to do same for other core aggregators once this is merged.
Description
This patch adds handling of single/multi value column handling by double sum/min/max aggregators to do a best effort parsing string as double.
StringColumnDoubleAggregatorWrapperandStringColumnDoubleBufferAggregatorWrapperclasses are introduced that can wrap existing double aggregators to handle string columns. Both of the classes are used bySimpleDoubleAggregatorFactoryto be used when input column is known to be of String type.Currently it does not work if
fieldExpressioninstead offieldNameis provided. (Related #8242 )This PR has: