gianm commented on a change in pull request #7792: Fix wrong order when
ordering by string first/last agg with GroupBy V2
URL: https://github.com/apache/incubator-druid/pull/7792#discussion_r288725592
##########
File path:
processing/src/main/java/org/apache/druid/query/groupby/orderby/DefaultLimitSpec.java
##########
@@ -243,7 +246,30 @@ public int compare(Row left, Row right)
if (postAggregatorsMap.containsKey(columnName)) {
nextOrdering = metricOrdering(columnName,
postAggregatorsMap.get(columnName).getComparator());
} else if (aggregatorsMap.containsKey(columnName)) {
- nextOrdering = metricOrdering(columnName,
aggregatorsMap.get(columnName).getComparator());
+ Comparator comparator;
+ AggregatorFactory aggFactory = aggregatorsMap.get(columnName);
+ if (aggFactory instanceof StringLastAggregatorFactory
Review comment:
Ouch!! We should try to avoid query engines (like groupBy) using
`instanceof` to be aware of special cases for specific aggregators. It likely
means something is flawed with the design somewhere.
Maybe StringLastAggregatorFactory & StringFirstAggregatorFactory should have
their getComparator method changed to only look at the string value, not the
timestamp. I'm not sure if there's any good reason for them to look at the
timestamp.
Please add tests too. Ideally, one in TopNQueryRunnerTest and one in
GroupByQueryRunnerTest.
----------------------------------------------------------------
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]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
