gortiz opened a new pull request, #19883:
URL: https://github.com/apache/pinot/pull/19883
## Summary
This PR makes a misconfigured `EXPR_MIN`/`EXPR_MAX` query fail fast with a
clear error. Before this change, the broker waited for the query timeout and
returned `427 ... servers not responded`. The PR has two independent changes:
1. **Server:** when the server cannot compile a request, it sends an error
response to the broker. Before, it sent no response at all.
2. **Broker:** the single-stage broker rejects `EXPR_MIN`/`EXPR_MAX` queries
that the servers cannot execute with the configured rewriters. The error
message names the missing rewriter and its config key.
## Background
The servers cannot execute `EXPR_MIN`/`EXPR_MAX` directly.
`AggregationFunctionFactory` throws for `EXPRMIN`/`EXPRMAX`. The broker must do
two rewrites:
- `ExprMinMaxRewriter` (`pinot.broker.query.rewriter.class.names`) changes
each call into a parent and child aggregations.
- `ParentAggregationResultRewriter`
(`pinot.broker.result.rewriter.class.names`) copies the parent result into the
child columns.
Neither rewriter is enabled by default. Also, `ExprMinMaxRewriter` only
rewrites top-level expressions of the select list. An aliased call
(`EXPR_MIN(a, b) AS x`) or a nested call (`EXPR_MIN(a, b) + 1`) reaches the
servers unchanged.
A simple query such as `SELECT EXPR_MIN(a, b) AS x FROM t` on a default
cluster fails like this:
```
broker ──InstanceRequest──▶ server
InstanceRequestHandler.channelRead0
├─ deserialize Thrift OK
└─ new ServerQueryRequest(...) throws
(EXPRMIN not supported)
catch: "request id is unknown" → log,
return (no response)
broker waits until timeout ──▶ 427 "1 servers [...] not responded"
```
The catch block assumed a deserialization failure. But `ServerQueryRequest`
also compiles the `QueryContext`, and that step can fail for a valid Thrift
request. The real error was only in the server log, as `Failed to deserialize
instance request`.
## Changes
**`InstanceRequestHandler` (pinot-core)**
- Split the `try` block in `channelRead0`.
- A Thrift deserialization failure keeps the old behavior, because the
request id is unknown.
- A failure in `new ServerQueryRequest(...)` now calls `sendErrorResponse`
with the known request id. The broker gets a `QUERY_EXECUTION` error with the
real message immediately.
- This applies to any failure to compile a query on the server, not only to
`EXPR_MIN`.
**`BaseSingleStageBrokerRequestHandler#validateRequest` (pinot-broker)**
`validateRequest` now calls the new `validateExprMinMax`, which returns
`QUERY_VALIDATION` before dispatch in these cases:
| Case | Error message |
|---|---|
| `EXPR_MIN`/`EXPR_MAX` is in the query and `ExprMinMaxRewriter` is not
enabled | `EXPR_MIN is not enabled on this broker. To enable it, add
'...ExprMinMaxRewriter' to 'pinot.broker.query.rewriter.class.names' and
'...ParentAggregationResultRewriter' to
'pinot.broker.result.rewriter.class.names'` |
| `ExprMinMaxRewriter` is enabled, but the call has an alias or is nested |
`EXPR_MIN is only supported as a top-level expression in the select list,
without an alias` |
| The query has child `EXPR_MIN`/`EXPR_MAX` aggregations and
`ParentAggregationResultRewriter` is not enabled | `EXPR_MIN and EXPR_MAX
require the result rewriter '...ParentAggregationResultRewriter'. To enable it,
add it to 'pinot.broker.result.rewriter.class.names'` |
The check is in the broker and not in `CalciteSqlParser.validate`. The
controller, materialized views, and the recommender also use the parser, and
they must not depend on the broker rewriter config.
## Behavior change
The first two cases already failed before this PR, but with a timeout. The
third case did not fail before. Based on the code, the broker returned the
placeholder values of the child aggregations
(`ChildAggregationFunction#extractFinalResult` returns `0`) and an extra parent
column. Now the broker rejects the query.
## Tests
- `InstanceRequestHandlerTest#testRepliesWithErrorWhenQueryCannotBeCompiled`
sends a serialized `InstanceRequest` with `EXPR_MIN` to `channelRead0`. It
checks that the server writes a `DataTable` with the request id and the
`EXPRMIN` error. Without the fix, the test fails with `Wanted but not invoked:
ctx.writeAndFlush(...)`.
- `QueryValidationTest` covers the three cases above and a valid query with
both rewriters enabled.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
