yashmayya opened a new pull request, #19324:
URL: https://github.com/apache/pinot/pull/19324
`ProjectAggregateMergeRule` silently drops the matched `Aggregate`'s hints,
so every `aggOptions` option on that
aggregate is lost as soon as a `Project` sits directly above it. In
particular
`aggOptions(is_partitioned_by_group_by_keys='true')` stops working and the
aggregate is split into
LEAF + exchange + FINAL, shuffling data that was already colocated.
### Why the hints are lost
The rule rebuilds the aggregate from scratch with a `RelBuilder`, so the
rebuilt node starts with no hints.
Calcite normally repairs that automatically —
`RelOptRuleCall.transformTo(RelNode)` propagates the hints of
`rels[0]`, the node the rule matched on, into the new sub-tree. Here
`rels[0]` is the `Project`, and `aggOptions`
hints live on the `Aggregate` (`HintPredicates.AGGREGATE`), so there is
nothing to restore and the aggregate's
hints are dropped.
Rules whose root operand *is* the hinted node are unaffected, which is why
`AggregateReduceFunctionsRule`,
`AggregateProjectMergeRule` and `AggregateCaseToFilterRule` all keep their
hints (they come back with
`inheritPath:[0]`).
### Why it only shows up on some queries
A `Project` ends up directly above the aggregate most often because the
`SUM` argument is **nullable**:
`PinotAggregateReduceFunctionsRule` rewrites `SUM(x)` into `$SUM0(x) +
COUNT(x)` plus a
`CASE(COUNT(x) = 0, NULL, $SUM0(x))` project, which is exactly the pattern
`ProjectAggregateMergeRule` matches.
With a non-nullable argument the reduction collapses to a bare `$SUM0`, no
project is created, the rule does not
match, and the hint survives. Every existing test for this hint uses a
non-nullable argument, which is why the
regression went unnoticed.
Window functions are a common indirect trigger: `LAG`/`LEAD` are nullable,
so anything derived from them is
nullable too. That is how this was found — a customer query doing `LAG`
partitioned on the table's partition
column followed by a `GROUP BY` on that same column, where the window was
colocated but the aggregate above it
was not, despite the hint.
Tracing the rules on
`SELECT /*+ aggOptions(is_partitioned_by_group_by_keys='true') */ col1,
SUM(CASE WHEN col3 > 5 THEN col3 ELSE NULL END) FROM b GROUP BY col1`:
```
AggregateReduceFunctions produced -> AGG[$SUM0($1), COUNT($1)]
hints=[[aggOptions inheritPath:[0]
options:{is_partitioned_by_group_by_keys=true}]]
ProjectAggregateMerge produced -> AGG[$SUM0($1), COUNT($1)] hints=[]
<-- dropped
PinotAggregateExchangeNodeInsertRule sees hints=[] -> LEAF + exchange + FINAL
```
### Fix
`PinotProjectAggregateMergeRule` reuses Calcite's transformation verbatim —
no forked rule body to drift out of
sync — and re-attaches the matched aggregate's hints to the rebuilt
aggregate. The rule never changes the
aggregate's group set, so the hints stay valid.
The hints are copied **verbatim** rather than via
`RelOptUtil.propagateRelHints`: that helper appends the child
index to each hint's `inheritPath` as it descends, so re-propagating on
every application grows the inherit path
without bound, the rebuilt node never compares equal to the previous one,
the rule re-fires on its own output and
planning dies with a `StackOverflowError`. Copying the list unchanged makes
the rewrite a fixpoint.
When the aggregate has no hints the rule delegates straight to Calcite, so
the common path is untouched.
### Affected options
All `aggOptions` on the aggregate, not just the colocation one:
`is_partitioned_by_group_by_keys`,
`is_skip_leaf_stage_group_by`, `is_leaf_return_final_result` and the
group-trim options. Both the default logical
planner and the v2 physical optimizer (`usePhysicalOptimizer=true`) were
affected.
### Scope
`ProjectAggregateMergeRule` was added default-on in #18554, so this
regressed then. The pre-existing workaround
is `SET skipPlannerRules='ProjectAggregateMerge'`.
I did not find an upstream Calcite issue for the hint loss; the closest is
[CALCITE-6864](https://issues.apache.org/jira/browse/CALCITE-6864), which
fixed the same rule losing the
Project's field names. Worth reporting upstream separately.
### Tests
Three regression tests in `QueryCompilationTest`, all failing before this
change:
- `testAggregateHintSurvivesProjectAggregateMerge` — nullable `SUM` with the
colocation hint must plan a single
`DIRECT` aggregate with no exchange below it.
- `testAggregateHintSurvivesProjectAggregateMergeAboveWindow` — the same
through a `LAG` window, the shape this
was found on.
- `testSkipLeafStageGroupByHintSurvivesProjectAggregateMerge` — covers a
second option on the same hint.
### Possible follow-up
The same shape — a rule matching on a non-hinted parent while rebuilding a
hinted child — exists in at least
`FilterAggregateTransposeRule` (matches `Filter`, rebuilds `Aggregate`) and
`ProjectWindowTransposeRule` (matches
`Project`, rebuilds `Window`), both default-on. I have not confirmed either
actually loses hints; worth auditing
separately rather than widening this PR.
--
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]
