-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-13617: [C++] Make Decimal representations consistent #12134
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
Conversation
456d500 to
d7cbefd
Compare
58fc6ad to
1302436
Compare
Factor out some basics of decimal representation and implementation in a generic base class.
1302436 to
5d74a90
Compare
lidavidm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for doing this.
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for baseline = b1a3e8c and contender = 5d74a90. Results will be available as each benchmark for each run completes. |
|
Hmm, hopefully Travis also gets around to building this PR so that we can run the tests on big-endian. |
|
Unfortunately, Travis never runs on my PRs. I have no idea why. |
|
Indeed, this PR isn't in the Travis queue, and I also can't manually trigger a build for a fork from the UI, which is unfortunate. |
|
I ran the s390x tests locally under emulation and they seemed to work (I had to disable Gandiva, though). |
| kRescaleDataLoss, | ||
| }; | ||
|
|
||
| template <typename Derived, int BIT_WIDTH, int NWORDS = BIT_WIDTH / 64> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Derived is not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
| return result; | ||
| } | ||
|
|
||
| // Explicitly instantiate template base class, for DLL linking on Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this approach avoid putting all template class implementation code inside the header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should, though that's not the goal here :-)
IBM provides free s390x cloud instances. I tried, it works. But it expires in half year and you have to apply for a new one. |
|
Benchmark runs are scheduled for baseline = ab86daf and contender = d67a210. d67a210 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Factor out some basics of decimal representation and implementation in a generic base class.