rohangarg commented on code in PR #16402:
URL: https://github.com/apache/druid/pull/16402#discussion_r1596678213
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/GroupingSqlAggregator.java:
##########
@@ -90,7 +91,17 @@ public Aggregation toDruidAggregation(
}
}
}
- AggregatorFactory factory = new GroupingAggregatorFactory(name, arguments);
+ AggregatorFactory factory;
+ try {
+ factory = new GroupingAggregatorFactory(name, arguments);
+ }
+ catch (Exception e) {
Review Comment:
pass by comment, is this the correct way to handle any errors coming out of
the agg factory?
A couple of things that I find ambiguous :
1. `Grouping Aggregation [%s] is not supported` seems like misleading
message incase there's an actual bug in the agg factory
2. `CannotBuildQueryException` only takes in the message of the exception,
which would make things harder to debug without a stack if there's an exception.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
