gortiz opened a new pull request, #19554:
URL: https://github.com/apache/pinot/pull/19554
The single-stage engine evaluates a HAVING filter only while reducing a
GROUP BY aggregation (`GroupByDataTableReducer`). `SelectionDataTableReducer`,
`AggregationDataTableReducer` and `DistinctDataTableReducer` all ignore
`getHavingFilter()`, so for every other query shape the predicate was discarded
with no error and no warning — the query answered as if the clause were absent.
A third group of shapes failed instead with an internal error naming a clause
the user did not write.
The multi-stage engine answers all of these correctly or rejects them, so
the two engines disagreed on the same query.
## Before / after / multi-stage
Measured end to end, all three columns on the same data: `Athens` ×4
(amounts summing to 6) and `Madrid` ×2 (summing to 20), six rows total. "error"
means the query is rejected at compile time with a message naming the fix.
| Query | single-stage before | single-stage after | multi-stage |
|---|---|---|---|
| `SELECT city … GROUP BY city HAVING city > 'B'` | **`Madrid`, `Athens`** |
error | `Madrid` |
| `SELECT city … GROUP BY city, amount HAVING city > 'B'` | **all 6 raw
rows** | error | `Madrid` |
| `SELECT city … GROUP BY city HAVING amount > 1` | **`Madrid`, `Athens`** |
error | error |
| `SELECT city … GROUP BY city HAVING COUNT(*) > 2` | **500 internal error**
| `Athens` | `Athens` |
| `SELECT city … GROUP BY city HAVING SUM(amount) > 10` | **500 internal
error** | `Madrid` | `Madrid` |
| `SELECT city, COUNT(*) … GROUP BY city HAVING city > 'B'` | `Madrid, 2` |
`Madrid, 2` | `Madrid, 2` |
| `SELECT COUNT(*) … HAVING COUNT(*) > 100` | **`6`** | 0 rows | 0 rows |
| `SELECT SUM(amount) … HAVING SUM(amount) > 100` | **`26.0`** | 0 rows | 0
rows |
| `SELECT COUNT(*) … HAVING amount > 1` | **`6`** | error | error |
| `SELECT MIN(amount), MAX(amount) … HAVING MIN(amount) < MAX(amount)` |
`1.0, 10.0` | `1.0, 10.0` | `1.0, 10.0` |
| `SELECT city … HAVING city > 'B'` | **all 6 raw rows** | error | error |
| `SELECT city … HAVING COUNT(*) > 1` | **500 internal error** | error |
error |
| `SELECT DISTINCT city … HAVING city > 'B'` | **`Madrid`, `Athens`** |
error | error |
Bold marks a wrong answer or an internal error. Nothing that previously
produced a correct answer changed: the two unbolded "before" rows are identical
in all three columns.
After this change the single-stage engine agrees with the multi-stage engine
on every row except the first two, which are discussed under *Deliberate
divergence* below.
## Behavior change
**Queries that silently returned wrong rows now either return the correct
rows or fail at compile time.** Two consequences worth calling out for
operators:
- **New hard failures.** Shapes that returned HTTP 200 now return
`SQL_PARSING`. None of them were returning correct results, but a dashboard
that was quietly showing the wrong number will now show an error instead. The
repo's own corpus contains one such query (`JsonPathQueriesTest`), which is
evidence users write them.
- **A silent result change.** `SELECT COUNT(*) FROM t HAVING COUNT(*) > 100`
used to return the aggregate row and now correctly returns zero rows. No error
is raised — a number simply changes to the right one.
Validation is broker-side, so during a rolling upgrade the same query
succeeds on old brokers and fails on new ones until the roll completes.
Operators may want to grep query logs for `HAVING` before rolling out.
## Changes
**`CalciteSqlParser.validateHavingClause()`** rejects what the engine cannot
evaluate. A HAVING clause imposes grouping semantics: without a GROUP BY the
whole table becomes a single group, so every expression in HAVING — and in the
SELECT list of a query with no GROUP BY — must be an aggregation, a literal, or
functionally dependent on the GROUP BY columns. That is the rule Calcite
applies for the multi-stage engine (`Expression 'x' is not being grouped`),
which is why the two engines now reject the same set.
This lives in `validate()` rather than in a new `QueryRewriter` on purpose:
`QueryRewriterFactory.init()` *replaces* the default rewriter list rather than
merging into it, so any deployment that pins
`pinot.broker.query.rewriter.class.names` would have silently missed the check.
**`NonAggregationGroupByToDistinctQueryRewriter`** leaves a query carrying a
HAVING clause alone. The rewrite drops the GROUP BY list, and DISTINCT has no
reduce step that evaluates a HAVING filter, so the predicate used to disappear
there. Keeping the GROUP BY puts the query back on the reduce path that does
evaluate it — which is also what turns the two 500s in the table into correct
answers.
**`AggregationDataTableReducer`** applies the filter to the single group of
an aggregation without GROUP BY. Two details:
- Null awareness is requested unconditionally rather than from
`requiresNullAwareKeyEvaluation()`. That flag only reports the query's
null-handling option, but this reducer materializes a `null` final result
either way, because an aggregation over an empty whole-table group has no value
to report. The flag gates the "a null never matches" early return in
`PredicateRowMatcher`, so without it a null would be unboxed and throw.
- The result rewriters run even when the row is filtered away, because
`ParentAggregationResultRewriter` may replace the `DataSchema`. Returning early
would make the reported schema depend on the data.
## Deliberate divergence: GROUP BY with no aggregation
The first two rows of the table are rejected here but answered by the
multi-stage engine. This is a deliberate choice, not an oversight.
Such a query has no grouping operator in the single-stage engine —
`NonAggregationGroupByToDistinctQueryRewriter` turns it into a DISTINCT. Moving
the predicate into WHERE would be equivalent **for a single-valued grouping
column**, but not for a multi-valued one: `GROUP BY mvCol` builds one group per
value, while `WHERE mvCol > 5` keeps whole rows and `DISTINCT` then emits every
value of every qualifying row. For a row holding `{1, 9}` and the predicate `>
5`, grouping yields `{9}` while the WHERE form yields `{1, 9}`. The rewrite
runs before any schema is available, so the two cases cannot be told apart
there.
Rejecting is strictly better than what shipped before — these queries were
returning wrong rows, not right ones — and the error names both remedies: move
the predicate to WHERE, or use the multi-stage engine.
The second row also exposes a **pre-existing** gap that is out of scope
here: `SELECT city FROM t GROUP BY city, amount` returns all six raw rows even
with no HAVING at all. A GROUP BY whose SELECT list is a strict subset of the
grouping columns is not executed as a grouping query.
## Testing
`HavingQueriesTest` is new and covers every row of the table end to end on
real segments, plus:
- the HAVING predicate resolved through `PostAggregationHandler` rather than
by SELECT-list position (`SUM(amount) - COUNT(*)` in the SELECT list, and an
aggregate used only by the predicate), which is the only non-trivial logic in
the new reduce branch;
- an aggregation returning `null` over an empty group, run with null
handling both on and off, covering the unboxing path described above.
`QueryValidationTest`'s GROOVY-in-HAVING fixture used a shape that is now
rejected. It is rewritten as a legal aggregate query, and `compileToPinotQuery`
is moved inside the `try` so a future compile failure is reported as an
assertion rather than thrown out of the test.
## Not addressed
- The pre-existing `SELECT ⊂ GROUP BY` gap described above.
- A cluster-level single-stage/multi-stage parity test. Because this change
intentionally diverges on the two shapes above, a blanket parity assertion
would encode something false; it is worth adding as a follow-up with the
divergence written down explicitly.
--
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]
