Re: [PR] Improve MIN/MAX error message for non-numeric columns [pinot]

["akashchamp (via GitHub)" <gi...@apache.org>]

akashchamp commented on code in PR #19653:
URL: https://github.com/apache/pinot/pull/19653#discussion_r4208740381


##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/MinAggregationFunction.java:
##########
@@ -191,6 +213,7 @@ protected void updateAggregationResultHolder(AggregationResultHolder aggregation
   public void aggregateGroupBySV(int length, int[] groupKeyArray, GroupByResultHolder groupByResultHolder,
       Map<ExpressionContext, BlockValSet> blockValSetMap) {
     BlockValSet blockValSet = blockValSetMap.get(_expression);
+    checkNumericType(blockValSet);

Review Comment:
   Agreed. The up-front type guard is gone. aggregate(), aggregateGroupBySV() and aggregateGroupByMV() in Min/MaxAggregationFunction now catch the NumberFormatException from the double conversion and rethrow it as a BadQueryRequestException with the MINSTRING/MAXSTRING and autoRewriteAggregationType guidance. STRING columns whose values parse as numbers still aggregate numerically on the group-by path, and testNumericStringColumnStillAggregatesInGroupBy covers that. I haven't run the tests locally; they will run in CI once the workflows are approved.



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org