xiangfu0 commented on code in PR #19883:
URL: https://github.com/apache/pinot/pull/19883#discussion_r4285790826
##########
pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/QueryValidationTest.java:
##########
@@ -167,6 +171,51 @@ public void testReplicaGroupToQueryInvalidQuery() {
() -> BaseSingleStageBrokerRequestHandler.validateRequest(pinotQuery,
10));
}
+ @Test
+ public void testExprMinMaxWithoutRewriters() {
Review Comment:
Minor: this test (and `testExprMinMaxWithoutResultRewriter`) rely on the
global `QueryRewriterFactory`/`ResultRewriterFactory` statics being in their
default state, but don't reset them in a `finally` of their own. It works today
because the other test resets in `finally`, but it's fragile against reordering
or another test class in the same fork leaving state behind. Non-blocking.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -2464,11 +2474,97 @@ static void validateRequest(PinotQuery pinotQuery, int
queryResponseLimit) {
"Value for 'LIMIT' (" + limit + ") exceeds maximum allowed value of
" + queryResponseLimit);
}
QueryOptionsUtils.getNumReplicaGroupsToQuery(pinotQuery.getQueryOptions());
+ validateExprMinMax(pinotQuery);
if (pinotQuery.getDataSource().getSubquery() != null) {
validateRequest(pinotQuery.getDataSource().getSubquery(),
queryResponseLimit);
}
}
+ /// Servers cannot execute EXPR_MIN/EXPR_MAX directly. The broker must
rewrite them into parent and child
+ /// aggregations with [ExprMinMaxRewriter], and merge the parent result back
into the child columns with
+ /// [ParentAggregationResultRewriter]. Neither rewriter is enabled by
default.
+ ///
+ /// Without this check, a query that still has EXPR_MIN/EXPR_MAX fails on
the servers, and a query without the
+ /// result rewriter returns the placeholder values of the child aggregations
instead of the expected values.
+ @VisibleForTesting
+ static void validateExprMinMax(PinotQuery pinotQuery) {
+ String exprMinMax = findFunction(pinotQuery, EXPR_MIN_MAX_FUNCTIONS);
+ if (exprMinMax != null) {
+ String functionName = exprMinMax.equals(EXPR_MIN) ? "EXPR_MIN" :
"EXPR_MAX";
+ boolean queryRewriterEnabled =
+
CalciteSqlParser.QUERY_REWRITERS.stream().anyMatch(ExprMinMaxRewriter.class::isInstance);
+ if (!queryRewriterEnabled) {
+ throw new BadQueryRequestException(
+ functionName + " is not enabled on this broker. To enable it, add
'" + ExprMinMaxRewriter.class.getName()
+ + "' to '" +
Broker.CONFIG_OF_BROKER_QUERY_REWRITER_CLASS_NAMES + "' and '"
+ + ParentAggregationResultRewriter.class.getName() + "' to '"
+ + Broker.CONFIG_OF_BROKER_RESULT_REWRITER_CLASS_NAMES + "'");
+ }
+ // The rewriter only handles the top-level expressions of the select
list, so the function is nested in another
+ // expression or has an alias.
+ throw new BadQueryRequestException(
+ functionName + " is only supported as a top-level expression in the
select list, without an alias");
Review Comment:
Minor: this branch also fires when EXPR_MIN/EXPR_MAX appears in WHERE /
GROUP BY / HAVING / ORDER BY (since `findFunction` searches all of them), but
the message only mentions the select list and alias. For a query like `WHERE
EXPR_MIN(a,b) > 5` the message is a little misleading. Could soften it to
something like "EXPR_MIN is only supported as a top-level select expression
without an alias" and note other locations aren't supported. Non-blocking.
--
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]
