Skip to content

Fix bounds check in MultiChannelGroupByHash and BigintGroupByHash#17834

Merged
highker merged 1 commit intoprestodb:masterfrom
v-jizhang:fix-bounds-check
Aug 3, 2022
Merged

Fix bounds check in MultiChannelGroupByHash and BigintGroupByHash#17834
highker merged 1 commit intoprestodb:masterfrom
v-jizhang:fix-bounds-check

Conversation

@v-jizhang
Copy link
Copy Markdown
Contributor

Cherry-pick of trinodb/trino#12597

There's an off-by-one error in the check that
can cause a failure when the page is empty

Co-authored-by: Karol Sobczak napewnotrafi@gmail.com

== NO RELEASE NOTE ==

@v-jizhang v-jizhang requested a review from a team as a code owner June 2, 2022 23:37
@v-jizhang v-jizhang requested a review from presto-oss June 2, 2022 23:37
@v-jizhang v-jizhang marked this pull request as draft June 2, 2022 23:38
@v-jizhang v-jizhang marked this pull request as ready for review June 2, 2022 23:39
Copy link
Copy Markdown
Contributor

@ajaygeorge ajaygeorge left a comment

Choose a reason for hiding this comment

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

Changes look good.
Can we add a test case if possible to highlight the off by one issue we are fixing.

Cherry-pick of trinodb/trino#12597

There's an off-by-one error in the check that
can cause a failure when the page is empty.

Co-authored-by: Karol Sobczak <napewnotrafi@gmail.com>
@v-jizhang
Copy link
Copy Markdown
Contributor Author

Changes look good. Can we add a test case if possible to highlight the off by one issue we are fixing.

Added a test. Thanks.

@sopel39 sopel39 requested a review from ajaygeorge July 28, 2022 14:37
Copy link
Copy Markdown
Contributor

@ajaygeorge ajaygeorge left a comment

Choose a reason for hiding this comment

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

LGTM

@ajaygeorge ajaygeorge requested a review from highker August 2, 2022 23:40
@highker highker merged commit 43597f5 into prestodb:master Aug 3, 2022
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