Skip to content

Extend max_by/min_by optimization to mix of array/map/scalar type#25435

Merged
feilong-liu merged 1 commit into
prestodb:masterfrom
feilong-liu:feature_dedup
Jun 26, 2025
Merged

Extend max_by/min_by optimization to mix of array/map/scalar type#25435
feilong-liu merged 1 commit into
prestodb:masterfrom
feilong-liu:feature_dedup

Conversation

@feilong-liu

Copy link
Copy Markdown
Contributor

Description

This optimizer was added in #25190
The current implementation needs all max_by/min_by operates on Map type, however, I found production queries where the max_by/min_by has a mix of map and non map type.
As long as one of the min_by/max_by has a complex type, here I use map and array type, this optimization will be useful. Hence I extend it to be effective as long as one min_by/max_by is on a map or array type.

Motivation and Context

Extend existing optimizer to cover more cases

Impact

Extend existing optimizer to cover more cases

Test Plan

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
* Improve `MinMaxByToWindowFunction` optimizer to cover cases where aggregation is on both map/array and non map/array types

@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Jun 25, 2025
else {
return Result.empty();
}
if (candidateAggregation.values().stream().noneMatch(x -> x.getArguments().get(0).getType() instanceof MapType || x.getArguments().get(0).getType() instanceof ArrayType)) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Move the map/array type check here, and apply optimization as long as one of the aggregations are in map/array type

@feilong-liu feilong-liu merged commit 2f7f08f into prestodb:master Jun 26, 2025
106 checks passed
@feilong-liu feilong-liu deleted the feature_dedup branch June 26, 2025 16:12
@prestodb-ci prestodb-ci mentioned this pull request Jul 28, 2025
6 tasks
@yhwang

yhwang commented Aug 18, 2025

Copy link
Copy Markdown
Member

Hi @feilong-liu , I am working on the release notes for this PR. Can you revise your release note from the user's perspective?

@kaikalur

Copy link
Copy Markdown
Contributor

It's actually more than that - it's also useful when you have a lot of columns. But yeah let's start with this.

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.

6 participants