Jackie-Jiang commented on code in PR #19653:
URL: https://github.com/apache/pinot/pull/19653#discussion_r4109651264
##########
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:
This new guard rejects every STRING column before the group-by path reads
its values. The existing `getDoubleValuesSV/MV()` readers parse numeric
strings, so a query such as `SELECT g, MIN(x) FROM t GROUP BY g` with STRING
`x` values `2` and `10` previously returned a numeric minimum but now fails.
The same change is present in `MaxAggregationFunction`. Since the PR is scoped
to improving error messages, could we wrap conversion failures with the new
guidance while preserving successful parses? If rejecting numeric strings is
intentional, please document and test that behavior change.
--
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]
