jihoonson commented on a change in pull request #6505: Add default comparison
to HavingSpecMetricComparator for custom Aggregator types
URL: https://github.com/apache/incubator-druid/pull/6505#discussion_r227612116
##########
File path:
processing/src/main/java/org/apache/druid/query/groupby/having/HavingSpecMetricComparator.java
##########
@@ -73,6 +73,9 @@ static int compare(String aggregationName, Number value,
Map<String, AggregatorF
double d = Double.parseDouble(metricValueStr);
return Double.compare(d, value.doubleValue());
}
+ } else if (metricValueObj instanceof Number) {
Review comment:
Hmm, what I meant was actually to add an `if` clause to handle the case when
`metricValueObj` is an instance of `BigDecimal`. I think this makes more sense
because `new BigDecimal(String)` requires the string to be formatted to a
string representation of `BigDecimal`. So, if one of child classes of Number
returns a string which is not `BigDecimal` representation, the below code will
throw an exception. This would work for your custom type if it extends
`BigDecimal`.
But, I think we might need to add support for the custom comparator for
custom metric types. This would be more complicated, but it would be worth if
this kind of use case is common.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
