gianm commented on a change in pull request #9638:
URL: https://github.com/apache/druid/pull/9638#discussion_r476264801
##########
File path:
processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java
##########
@@ -1135,21 +1117,21 @@ public MetricDesc(int index, AggregatorFactory factory)
this.index = index;
this.name = factory.getName();
- String typeInfo = factory.getTypeName();
- if ("float".equalsIgnoreCase(typeInfo)) {
- capabilities =
ColumnCapabilitiesImpl.createSimpleNumericColumnCapabilities(ValueType.FLOAT);
- this.type = typeInfo;
- } else if ("long".equalsIgnoreCase(typeInfo)) {
- capabilities =
ColumnCapabilitiesImpl.createSimpleNumericColumnCapabilities(ValueType.LONG);
- this.type = typeInfo;
- } else if ("double".equalsIgnoreCase(typeInfo)) {
- capabilities =
ColumnCapabilitiesImpl.createSimpleNumericColumnCapabilities(ValueType.DOUBLE);
- this.type = typeInfo;
+ ValueType valueType = factory.getType();
+
+ if (valueType.isPrimitive()) {
Review comment:
Previously this only did createSimpleNumericColumnCapabilities if the
type was numeric, now it includes strings too. Is that intentional?
##########
File path:
processing/src/main/java/org/apache/druid/query/aggregation/post/FieldAccessPostAggregator.java
##########
@@ -78,10 +89,28 @@ public String getName()
return name;
}
+ @Override
+ public ValueType getType()
+ {
+ return type;
+ }
+
@Override
public FieldAccessPostAggregator decorate(Map<String, AggregatorFactory>
aggregators)
{
- return this;
+ final ValueType type;
+
+ if (aggregators != null && aggregators.containsKey(fieldName)) {
+ type = aggregators.get(fieldName).getType();
+ } else {
+ type = ValueTypes.defaultAggregationType();
Review comment:
Why not keep it null here? What are the pros and cons of null vs
`ValueTypes.defaultAggregationType()`?
##########
File path:
processing/src/main/java/org/apache/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java
##########
@@ -94,29 +104,35 @@ public String getName()
return name;
}
+ @Override
+ public ValueType getType()
+ {
+ return finalizedType;
+ }
+
@Override
public FinalizingFieldAccessPostAggregator decorate(final Map<String,
AggregatorFactory> aggregators)
{
final Comparator<Object> theComparator;
final Function<Object, Object> theFinalizer;
+ final ValueType finalizedType;
if (aggregators != null && aggregators.containsKey(fieldName)) {
//noinspection unchecked
theComparator = aggregators.get(fieldName).getComparator();
+ theFinalizer = aggregators.get(fieldName)::finalizeComputation;
+ finalizedType = aggregators.get(fieldName).getFinalizedType();
} else {
//noinspection unchecked
theComparator = (Comparator) Comparators.naturalNullsFirst();
- }
-
- if (aggregators != null && aggregators.containsKey(fieldName)) {
- theFinalizer = aggregators.get(fieldName)::finalizeComputation;
- } else {
theFinalizer = Function.identity();
+ finalizedType = ValueTypes.defaultAggregationType();
Review comment:
Similar comment to FieldAccessPostAggregator: why not keep it null here?
What are the pros and cons of null vs `ValueTypes.defaultAggregationType()`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
