You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/08/20 18:38:34 UTC
[PR] Preserve aggregate hints through ProjectAggregateMergeRule [pinot]
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: 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] Preserve aggregate hints through ProjectAggregateMergeRule [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19324:
URL: https://github.com/apache/pinot/pull/19324#issuecomment-5360587869
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19324?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 ([`2ae7195`](https://app.codecov.io/gh/apache/pinot/commit/2ae7195ea432915600f564e8f67bbdeb2eb04360?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`953fc97`](https://app.codecov.io/gh/apache/pinot/commit/953fc9727ee67409415bc4ed5e6939b971c70b9f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (2ae7195) and HEAD (953fc97). Click for more details.
>
> <details><summary>HEAD has 14 uploads less than BASE</summary>
>
>| Flag | BASE (2ae7195) | HEAD (953fc97) |
>|------|------|------|
>|lane-a|2|1|
>|integration|4|2|
>|java-25|5|2|
>|temurin|5|2|
>|lane-b|2|1|
>|integration1|2|0|
>|unittests2|1|0|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19324 +/- ##
=============================================
- Coverage 39.17% 0.00% -39.18%
=============================================
Files 3462 3 -3459
Lines 220695 6 -220689
Branches 35258 0 -35258
=============================================
- Hits 86447 0 -86447
+ Misses 126373 6 -126367
+ Partials 7875 0 -7875
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19324/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/19324/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/19324/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/19324/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/19324/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-39.18%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19324/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/19324/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/19324/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-39.18%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19324/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/19324/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/19324?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] Preserve aggregate hints through ProjectAggregateMergeRule [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #19324:
URL: https://github.com/apache/pinot/pull/19324
--
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