You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2026/09/14 14:26:30 UTC
[PR] Reject QUALIFY in the single-stage engine instead of silently ignoring it [pinot]
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: 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
Re: [PR] Reject QUALIFY in the single-stage engine instead of silently ignoring it [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19553:
URL: https://github.com/apache/pinot/pull/19553#issuecomment-5666184606
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19553?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`cab1382`](https://app.codecov.io/gh/apache/pinot/commit/cab13822b267fc301c4b78926331f9773f0d0bfc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7ae7e9f`](https://app.codecov.io/gh/apache/pinot/commit/7ae7e9f947691ec20c94df5f967115319cc9a240?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 44 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (cab1382) and HEAD (7ae7e9f). Click for more details.
>
> <details><summary>HEAD has 18 uploads less than BASE</summary>
>
>| Flag | BASE (cab1382) | HEAD (7ae7e9f) |
>|------|------|------|
>|lane-a|2|1|
>|integration|4|2|
>|java-25|6|2|
>|temurin|6|2|
>|lane-b|2|1|
>|integration1|2|0|
>|unittests1|1|0|
>|unittests|2|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19553 +/- ##
=============================================
- Coverage 67.70% 0.00% -67.71%
=============================================
Files 3488 3 -3485
Lines 224472 6 -224466
Branches 35440 0 -35440
=============================================
- Hits 151977 0 -151977
+ Misses 60472 6 -60466
+ Partials 12023 0 -12023
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-67.71%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-67.71%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19553/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19553?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] Reject QUALIFY in the single-stage engine instead of silently ignoring it [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #19553:
URL: https://github.com/apache/pinot/pull/19553
--
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
Re: [PR] Reject QUALIFY in the single-stage engine instead of silently ignoring it [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19553:
URL: https://github.com/apache/pinot/pull/19553#issuecomment-5678751920
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/1045 documents QUALIFY as multi-stage-only, the single-stage compile-time rejection, migration guidance, and rolling-upgrade caution.
--
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