Skip to content

GH-38833: [C++] Avoid hash_mean overflow#39349

Merged
pitrou merged 1 commit intoapache:mainfrom
js8544:jinshang/hash_mean_double
Jan 11, 2024
Merged

GH-38833: [C++] Avoid hash_mean overflow#39349
pitrou merged 1 commit intoapache:mainfrom
js8544:jinshang/hash_mean_double

Conversation

@js8544
Copy link
Copy Markdown
Contributor

@js8544 js8544 commented Dec 22, 2023

Rationale for this change

hash_mean overflows if the sum of a group is larger than uint64 max.

What changes are included in this PR?

Save the intermediate sum as double to avoid overflow

Are these changes tested?

yes

Are there any user-facing changes?

no

@js8544
Copy link
Copy Markdown
Contributor Author

js8544 commented Dec 22, 2023

Similar to #37243, but for hash_mean.

@github-actions github-actions bot added the awaiting review Awaiting review label Dec 22, 2023
Copy link
Copy Markdown
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thank you @js8544 ! This LGTM, but can you rebase on main?

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 9, 2024
@js8544 js8544 force-pushed the jinshang/hash_mean_double branch from 7b2f703 to b564ab6 Compare January 11, 2024 09:37
@js8544
Copy link
Copy Markdown
Contributor Author

js8544 commented Jan 11, 2024

Thank you @js8544 ! This LGTM, but can you rebase on main?

Done

@pitrou pitrou merged commit d181072 into apache:main Jan 11, 2024
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Jan 11, 2024
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Jan 11, 2024
@conbench-apache-arrow
Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit d181072.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
### Rationale for this change

hash_mean overflows if the sum of a group is larger than uint64 max.

### What changes are included in this PR?

Save the intermediate sum as double to avoid overflow

### Are these changes tested?

yes

### Are there any user-facing changes?
 no

* Closes: apache#38833

Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] hash_mean overflows if numeric sum is larger than int64 max

2 participants