Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-16013: [C++][Python] Signed overflow when using negative stride in NumPyStridedConverter #12699

Closed
wants to merge 4 commits into from

Conversation

zagto
Copy link
Contributor

@zagto zagto commented Mar 23, 2022

No description provided.

@github-actions
Copy link

@pitrou
Copy link
Member

pitrou commented Mar 24, 2022

Is it possible to add a test that would fail without this fix?

@zagto
Copy link
Contributor Author

zagto commented Mar 24, 2022

Not in a way I know of.

  • To reliably break it, we could use it on a numpy type, that is not a power of 2 in sizeof(). But that does not seem to exist x86_64. But for example long double on 32-bit x86 is 96-bit sizeof() (?)
  • As long is the size is a power of two it depends on the behavior of signed overflow and the binary representation of negative numbers to work/break, which happens to make it work on common platforms.
  • It could still break because of compiler optimizations, but then again that is something we can't really control in a test

@pitrou
Copy link
Member

pitrou commented Mar 24, 2022

Ok, thanks. Can you then add a comment near your change pointing to the issue?

@zagto zagto force-pushed the numpy_negative_stride branch from 4ea5ef6 to ffafd8c Compare March 24, 2022 13:43
@zagto
Copy link
Contributor Author

zagto commented Mar 24, 2022

yes, added a comment

Copy link
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 for the update @zagto, will merge.

@pitrou pitrou closed this in 6773da1 Mar 29, 2022
@ursabot
Copy link

ursabot commented Mar 29, 2022

Benchmark runs are scheduled for baseline = 5cedd25 and contender = 6773da1. 6773da1 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️1.01% ⬆️0.04%] test-mac-arm
[Finished ⬇️5.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.26% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/402| 6773da12 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/388| 6773da12 test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/388| 6773da12 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/398| 6773da12 ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/401| 5cedd25e ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/387| 5cedd25e test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/387| 5cedd25e ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/397| 5cedd25e ursa-thinkcentre-m75q>
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

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

Successfully merging this pull request may close these issues.

3 participants