lidavidm commented on a change in pull request #11080:
URL: https://github.com/apache/arrow/pull/11080#discussion_r714216439
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1622,61 +1622,80 @@ TEST(TestBinaryDecimalArithmetic, DispatchBest) {
}
}
- // decimal, integer
- for (std::string name : {"add", "subtract", "multiply", "divide"}) {
+ // decimal, decimal -> decimal and decimal, integer -> decimal
+ for (std::string name : {"add", "subtract"}) {
for (std::string suffix : {"", "_checked"}) {
name += suffix;
CheckDispatchBest(name, {int64(), decimal128(1, 0)},
- {decimal128(1, 0), decimal128(1, 0)});
+ {decimal128(19, 0), decimal128(1, 0)});
CheckDispatchBest(name, {decimal128(1, 0), int64()},
- {decimal128(1, 0), decimal128(1, 0)});
- }
- }
-
- // decimal, decimal
- for (std::string name : {"add", "subtract"}) {
- for (std::string suffix : {"", "_checked"}) {
- name += suffix;
+ {decimal128(1, 0), decimal128(19, 0)});
CheckDispatchBest(name, {decimal128(2, 1), decimal128(2, 1)},
- {decimal128(3, 1), decimal128(3, 1)});
+ {decimal128(2, 1), decimal128(2, 1)});
CheckDispatchBest(name, {decimal256(2, 1), decimal256(2, 1)},
- {decimal256(3, 1), decimal256(3, 1)});
+ {decimal256(2, 1), decimal256(2, 1)});
Review comment:
DispatchBest in this case doesn't actually promote either argument. When
I adjusted CheckDispatchBest to actually compare the final types against the
given types, it exposed discrepancies like this that I fixed. In the logic
here, for add/subtract, we do not scale up with the scales are the same:
https://github.com/apache/arrow/blob/3317f83526cf6cfc6c749748b3e836114262a8d0/cpp/src/arrow/compute/kernels/codegen_internal.cc#L268-L273
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
