ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic #7784
Conversation
This file contains 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
pitrou
force-pushed
the
ARROW-9402-overflow-arith
branch
from
8e57e8c
to
478f663
Compare
pitrou
force-pushed
the
ARROW-9402-overflow-arith
branch
from
4b145a6
to
1b646ee
Compare
|
I think I finally squashed the horrid Windows preprocessor macro issues (Windows headers define OPTIONAL, but you can't undef it blindly because other subsequent includes may require it...). |
|
@wesm You'll probably want to take a look at the codegen changes when you have some time. |
pitrou
force-pushed
the
ARROW-9402-overflow-arith
branch
from
0ac10bb
to
7d5b6e3
Compare
|
Rebased. @wesm could you perhaps review this? |
|
Or perhaps @bkietz . |
bkietz
requested changes
Aug 6, 2020
Comment on lines
+90
to
+92
| template <typename T, typename Arg0, typename Arg1> | ||
| enable_if_floating_point<T> Call(KernelContext*, Arg0 left, Arg1 right) { | ||
| static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, ""); |
There was a problem hiding this comment.
What happens if we remove those template arguments?
Suggested change
| template <typename T, typename Arg0, typename Arg1> | |
| enable_if_floating_point<T> Call(KernelContext*, Arg0 left, Arg1 right) { | |
| static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, ""); | |
| template <typename T> | |
| enable_if_floating_point<T> Call(KernelContext*, T left, T right) { |
There was a problem hiding this comment.
It doesn't compile, because Call is invoked with explicit template arguments by the kernel execution machinery.
| @@ -103,6 +103,16 @@ void CheckScalarUnary(std::string func_name, std::shared_ptr<Scalar> input, | |||
| std::shared_ptr<Scalar> expected, | |||
| const FunctionOptions* options = nullptr); | |||
|
|
|||
| void CheckScalarBinary(std::string func_name, std::shared_ptr<Scalar> left_input, | |||
| @@ -55,6 +55,8 @@ | |||
| #include "parquet/schema.h" | |||
| #include "parquet/statistics.h" | |||
| #include "parquet/types.h" | |||
| // Required after "arrow/util/int_util_internal.h" (for OPTIONAL) | |||
There was a problem hiding this comment.
Suggested change
| // Required after "arrow/util/int_util_internal.h" (for OPTIONAL) | |
| // Required after "arrow/util/int_util_internal.h" (for OPTIONAL) |
I'm not sure if clang-format will always recognize this comment as breaking this #include out of the above block. I suppose it's alphabetically always going to be lower in the sort, though
…etic Vendor relevant code from the portable-snippets library (~ public domain): https://github.com/nemequ/portable-snippets/tree/master/safe-math Also fix some bugs in checked arithmetic (null values had their value slots checked as well), add compute scaffolding for stateful binary scalar functions.
pitrou
force-pushed
the
ARROW-9402-overflow-arith
branch
from
7d5b6e3
to
75afb88
Compare
|
I believe I addressed your comments @bkietz , so I'm going to merge now. |
kszucs
pushed a commit
to kszucs/arrow
that referenced
this pull request
Aug 11, 2020
…etic Vendor relevant code from the portable-snippets library (~ public domain): https://github.com/nemequ/portable-snippets/tree/master/safe-math Also fix some bugs in checked arithmetic (null values had their value slots checked). Add compute scaffolding for stateful binary scalar functions. Closes apache#7784 from pitrou/ARROW-9402-overflow-arith Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
kszucs
pushed a commit
to kszucs/arrow
that referenced
this pull request
Aug 11, 2020
…etic Vendor relevant code from the portable-snippets library (~ public domain): https://github.com/nemequ/portable-snippets/tree/master/safe-math Also fix some bugs in checked arithmetic (null values had their value slots checked). Add compute scaffolding for stateful binary scalar functions. Closes apache#7784 from pitrou/ARROW-9402-overflow-arith Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
kszucs
pushed a commit
to kszucs/arrow
that referenced
this pull request
Aug 17, 2020
…etic Vendor relevant code from the portable-snippets library (~ public domain): https://github.com/nemequ/portable-snippets/tree/master/safe-math Also fix some bugs in checked arithmetic (null values had their value slots checked). Add compute scaffolding for stateful binary scalar functions. Closes apache#7784 from pitrou/ARROW-9402-overflow-arith Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
GeorgeAp
pushed a commit
to sirensolutions/arrow
that referenced
this pull request
Jun 7, 2021
…etic Vendor relevant code from the portable-snippets library (~ public domain): https://github.com/nemequ/portable-snippets/tree/master/safe-math Also fix some bugs in checked arithmetic (null values had their value slots checked). Add compute scaffolding for stateful binary scalar functions. Closes apache#7784 from pitrou/ARROW-9402-overflow-arith Authored-by: Antoine Pitrou <antoine@python.org> 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
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.
Vendor relevant code from the portable-snippets library (~ public domain):
https://github.com/nemequ/portable-snippets/tree/master/safe-math
Also fix some bugs in checked arithmetic (null values had their value slots checked).
Add compute scaffolding for stateful binary scalar functions.