-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-43956: [C++][Format] Add initial Decimal32/Decimal64 implementations #43957
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
Merged
Merged
Changes from all commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
50b57b6
GH-43956: [C++][Format] Add initial Decimal32/Decimal64 implementations
zeroshade 12896a9
fix linting
zeroshade 5e125e0
fixing a build issue
zeroshade 64aad89
add not implemented for new decimal types with pyarrow
zeroshade f2a46dd
make conversion explicit instaed of implicit
zeroshade ed3ff00
deprecate old decimal, add smallest_decimal, fix tests
zeroshade 00caa8a
fix linting
zeroshade 1684403
fix implicit cast
zeroshade 9b28a0e
update from feedback
zeroshade 6ba84d5
fix build issue
zeroshade a510534
enable decimal32/64 integration testing
zeroshade 67512e5
python lint
zeroshade 02aea22
Update cpp/src/arrow/integration/json_internal.cc
zeroshade db59d6d
Update cpp/src/arrow/ipc/metadata_internal.cc
zeroshade ebed805
updates from feedback
zeroshade b95d9ba
pre-commit and fix
zeroshade f4b3a81
rebase and fix
zeroshade 5d82818
ran pre-commit
zeroshade 315579d
Update cpp/src/arrow/type_fwd.h
zeroshade 5bef850
updates from feedback and propagation
zeroshade 53cf289
more updates from feedback
zeroshade 87a7445
pre-commit linting
zeroshade 7ad7dbc
convert away from deprecated function
zeroshade 202df3b
pre-commit
zeroshade 683a789
fix test
zeroshade ccbe6ca
changes from feedback for swapendian
zeroshade 91e3c13
fix unit tests
zeroshade f47a1f4
Update cpp/src/arrow/array/builder_dict.h
zeroshade 86aa26c
fixed test issue in parquet
zeroshade 34493f7
Update cpp/src/arrow/engine/substrait/expression_internal.cc
zeroshade f71303c
Update cpp/src/arrow/compute/kernels/hash_aggregate.cc
zeroshade f276a9e
updates from feedback
zeroshade 3b6d74a
lint and windows build fix
zeroshade 7ea13c4
Update cpp/src/arrow/integration/json_internal.cc
zeroshade 295b4c8
Update cpp/src/arrow/util/basic_decimal.h
zeroshade a8c0c75
updates from feedback and comments
zeroshade 46c84f1
remove commented out code
zeroshade 4973864
ran pre-commit for linting
zeroshade b7d2bf3
sumtype and accumulator type for decimals should be consistent
zeroshade 512d464
simplify check
zeroshade 8f46e50
linting
zeroshade ae3d8a2
Add tests for Decimal32 and Decimal64
zeroshade bf9eb74
linting
zeroshade 2fb271a
remove abs from FromReal, only constexpr in C++23 and newer
zeroshade b44888d
simplify a bunch of tests with a generic typed_test
zeroshade e2957a9
use FromRealApprox
zeroshade 980d6fb
static_cast instead of implicit cast
zeroshade 2a3e5c4
remove special cases, adjust tests
zeroshade c723754
Update cpp/src/arrow/util/decimal.cc
zeroshade 5382eb4
more updates from comments
zeroshade 9fda783
add reference to issue for decimal32 approx
zeroshade af8c722
make RoundedRightShift a no-op
zeroshade 48639e3
fix tests
zeroshade b110605
avoid ASAN issue
zeroshade 1d97e27
fix ubsan test
zeroshade 39032f2
fix ubsan
zeroshade File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Has there been discussion about this namin already?
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.
smallest_decimalwas the suggestion by @pitrou, no one made any objections and I doubt he's particularly tied to the name if you have a suggestion for a better one. What are your thoughts?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.
"narrowest_decimal"?
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.
I added suggestions here #43957 (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.
Alright. Can we name it something shorter? I have at least 3 suggestions:
best_decimalfast_decimalleast_decimalSome of these naming conventions exist in the C++ standard library https://en.cppreference.com/w/cpp/types/integer
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.
"least_decimal" is the best of these 3 suggestions ("best" and "fast" do not convey the right meaning), but "smallest" is easier to remember than "least" IMHO.
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.
I agree with @pitrou that "smallest" is easier to remember than "least" but I'm fine with renaming it to "least" if that is preferred.
@felipecrv, @bkietz, @lidavidm thoughts?