Skip to content

Extend filtered aggregation optimizer to support only masked partial aggregation cases#25171

Merged
feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu:fix_merge_agg_filter
Jun 3, 2025
Merged

Extend filtered aggregation optimizer to support only masked partial aggregation cases#25171
feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu:fix_merge_agg_filter

Conversation

@feilong-liu
Copy link
Contributor

@feilong-liu feilong-liu commented May 21, 2025

Description

The current optimizer MergePartialAggregationsWithFilter only supports case where there are both aggregations with and without filter on the same filter. For example,
select sum(totalprice), sum(totalprice) filter (where orderpriority='2-HIGH') from orders group by orderdate
will work, but
select sum(totalprice) filter (where orderstatus='F'), sum(totalprice) filter (where orderpriority='2-HIGH') from orders group by orderdate
will not work.
In this change, I extend it to work for the second case too.

Motivation and Context

Extend existing optimizer to a broader range of queries.

Impact

Extend existing optimizer to a broader range of queries.

Test Plan

Add unit tests

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Extend  MergePartialAggregationsWithFilter to work for queries where all aggregations have mask

@feilong-liu feilong-liu requested a review from ZacBlanco May 21, 2025 18:21
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label May 21, 2025
@feilong-liu feilong-liu marked this pull request as draft May 21, 2025 18:21
@feilong-liu feilong-liu requested a review from kaikalur May 21, 2025 18:21
@feilong-liu feilong-liu force-pushed the fix_merge_agg_filter branch from cf74f97 to e322aec Compare May 21, 2025 18:36
@feilong-liu feilong-liu force-pushed the fix_merge_agg_filter branch from e322aec to 40b134f Compare May 23, 2025 19:24
@feilong-liu feilong-liu requested a review from kaikalur May 27, 2025 04:31
@feilong-liu feilong-liu marked this pull request as ready for review May 28, 2025 16:52
@feilong-liu feilong-liu requested a review from rschlussel May 28, 2025 16:52
Copy link
Member

@jaystarshot jaystarshot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default case when one mask, one normal is always beneficial but this new case might not be beneficial for high cardinality cases so we should try to make the first one on by default later

@kaikalur
Copy link
Contributor

kaikalur commented Jun 2, 2025

I think the default case when one mask, one normal is always beneficial but this new case might not be beneficial for high cardinality cases so we should try to make the first one on by default later

But I hope adding the disjunction mitigates that? We will run some benchmarks and see.

@feilong-liu feilong-liu merged commit 1bc7da4 into prestodb:master Jun 3, 2025
98 checks passed
@prestodb-ci prestodb-ci mentioned this pull request Jul 28, 2025
6 tasks
@yhwang
Copy link
Member

yhwang commented Aug 18, 2025

Hi @feilong-liu , I am working on the release notes for this PR. Can you revise the PR note and use more user-understandable wording?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants