gortiz opened a new pull request, #19553: URL: https://github.com/apache/pinot/pull/19553
The single-stage query compiler reads every clause of the `SqlSelect` node except `getQualify()`, so a QUALIFY predicate is parsed and then dropped. The query runs as if the clause were absent and returns every row, with no exception and no warning. The common de-duplication idiom: ```sql SELECT city, category FROM myTable QUALIFY ROW_NUMBER() OVER (PARTITION BY city ORDER BY orderDate DESC) = 1 ``` returns the whole table instead of one row per city. It works correctly on the multi-stage engine, so the same query gives different answers depending on which engine runs it — and the single-stage one gives no hint that anything was ignored. ## Behavior change **A single-stage query containing QUALIFY now fails at compile time with `SQL_PARSING` where it previously returned HTTP 200.** Those queries were already returning wrong rows, so no correct result becomes an error — but a saved query, dashboard or alert using QUALIFY against a single-stage table flips from "runs, wrong rows" to "hard failure" on upgrade. During a rolling broker upgrade the same query succeeds on old brokers and fails on new ones, so clients may see intermittent errors until the rollout completes. Operators may want to grep query logs for QUALIFY before rolling out. ## Why not rewrite QUALIFY as HAVING QUALIFY filters rows after the window functions of the SELECT list are evaluated, the way HAVING filters them after aggregation. With no window functions in the single-stage engine, QUALIFY looks equivalent to HAVING — but the single-stage engine applies its HAVING filter only in `GroupByDataTableReducer`. `SelectionDataTableReducer`, `AggregationDataTableReducer` and `DistinctDataTableReducer` all ignore it, so `SELECT city FROM t HAVING city > 'a'` compiles to a populated `havingExpression` that nothing ever evaluates. Folding QUALIFY into HAVING would therefore have moved the silent drop rather than removed it, for every query shape except an aggregating GROUP BY. Gating the rewrite on "is this the shape where HAVING actually runs?" would mean duplicating reducer-selection logic in the parser, where getting it wrong reintroduces exactly this class of bug. (The HAVING silent-drop is a pre-existing bug of the same class and is not addressed here.) ## Error message The message conditions its two remedies on whether the predicate references a window function: > QUALIFY is not supported by the single-stage query engine. Use the multi-stage query engine to filter on the result of a window function. If the predicate does not reference a window function, rewrite it as a WHERE clause (to filter on columns) or as a HAVING clause (to filter on aggregates of a GROUP BY query). Both branches are needed. Calcite rejects a QUALIFY without a window function as invalid SQL (`QUALIFY expression 'x > 1' must contain a window function`), so the multi-stage engine is the remedy only when a window function is present; otherwise WHERE or HAVING is. The single-stage parser never validated this, so predicates of both kinds reach the check. On the broker path this message is usually not what the user sees: `BaseSingleStageBrokerRequestHandler.compileRequest` catches the compilation failure and substitutes the canonical "retry using the multi-stage query engine" response whenever `ParserUtils.canCompileWithMultiStageEngine` succeeds — which is exactly the window-function case. `PinotQueryResource` has the same fallback. ## `TableNameExtractor` `visitSelect()` had the same omission: it enumerates FROM / WHERE / GROUP BY / HAVING / ORDER BY / SELECT but not `getQualify()`. That walker runs on multi-stage queries, where QUALIFY is legal, so a table referenced only from a subquery inside a QUALIFY was invisible to the controller's multi-stage query validation (`PinotQueryResource.validateMultiStageQuery`) and to the java client's `Connection.resolveTableName`. ## Testing Parser rejection is covered for the reported idiom, the SELECT-list alias spelling (`QUALIFY rn = 1`, the more common form), the shapes with no window function, QUALIFY sitting next to a HAVING, a FROM subquery and EXPLAIN. The multi-stage engine had no QUALIFY coverage at all — a repo-wide grep for `QUALIFY` found nothing before this PR. Added: - `WindowFunctionPlans.json` — pins the desugaring to a `LogicalFilter` over `LogicalWindow`. - `WindowFunctions.json` — proves the reported idiom returns one row per partition, under the default, new-optimizer and lite-mode runners. The `TableNameExtractor` test fails before the fix (finds 1 table, expects 2). ## Not addressed `UNION` / `INTERSECT` / `EXCEPT` and `WITH` carrying a QUALIFY hit the unguarded `(SqlSelect)` cast in `compileWithoutRewrite` and throw a raw `ClassCastException` before reaching this check. That is pre-existing, and the broker's `catch (Exception)` still routes those to the multi-stage response, so behavior is reasonable — just not via this message. -- 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]
