Skip to content

Fix MIN(x, n) and MAX(x, n) returning NULL on window aggregations#18615

Merged
kaikalur merged 1 commit intoprestodb:masterfrom
sviscaino:fix/minmaxn-window
Nov 5, 2022
Merged

Fix MIN(x, n) and MAX(x, n) returning NULL on window aggregations#18615
kaikalur merged 1 commit intoprestodb:masterfrom
sviscaino:fix/minmaxn-window

Conversation

@sviscaino
Copy link
Contributor

@sviscaino sviscaino commented Nov 3, 2022

These functions have an odd behaviour when used on window aggregations. They return NULL for all rows except the first one in the window, because the heap is being cleared by popAll here, so calling output() a second time will return NULL. We add a call to recover the state of the heap after building the output array.

Test plan -
Before
Screenshot 2022-11-03 at 12 09 02

After
Screenshot 2022-11-03 at 12 09 08

Unit test: mvn test -Dtest=TestMinMaxNFunction + non regression mvn test -Dtest=TestMinMaxByNAggregation

== RELEASE NOTES ==

General Changes
* Fix max(x, n) and min(x, n) returning NULL on window aggregations

@sviscaino sviscaino requested a review from a team as a code owner November 3, 2022 12:44
@sviscaino sviscaino requested a review from presto-oss November 3, 2022 12:44
@sviscaino sviscaino changed the title Fix MIN(x, n) and MAX(x, n) on window functions Fix MIN(x, n) and MAX(x, n) returning NULL on window aggregations Nov 3, 2022
@kaikalur
Copy link
Contributor

kaikalur commented Nov 3, 2022

Please add a couple of e2e test in abstract test queries that actually tests queries in a distributed environment

@sviscaino
Copy link
Contributor Author

Please add a couple of e2e test in abstract test queries that actually tests queries in a distributed environment

Added some e2e tests

@kaikalur kaikalur requested a review from highker November 3, 2022 20:09
@sviscaino
Copy link
Contributor Author

Addressed the issues, should be good to merge

@kaikalur kaikalur merged commit 4053ddc into prestodb:master Nov 5, 2022
@wanglinsong wanglinsong mentioned this pull request Jan 12, 2023
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants