bkietz commented on a change in pull request #10395:
URL: https://github.com/apache/arrow/pull/10395#discussion_r652077054
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -928,14 +974,32 @@ TEST(TestUnaryArithmetic, DispatchBest) {
}
}
+ // Signed arithmetic
for (std::string name : {"negate_checked"}) {
for (const auto& ty : {int8(), int16(), int32(), int64(), float32(),
float64()}) {
CheckDispatchBest(name, {ty}, {ty});
CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
}
}
- for (std::string name : {"negate", "negate_checked", "abs", "abs_checked"}) {
+ // Functions with fixed output type
+ struct FuncAndOutType {
+ std::string name;
+ std::shared_ptr<DataType> out_type;
+ };
+
+ FuncAndOutType funcs[] = {{"sign", int8()}};
+ for (const auto& func : funcs) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(),
uint16(), uint32(),
+ uint64(), float32(), float64()}) {
+ CheckDispatchBest(func.name, {ty}, {func.out_type});
+ CheckDispatchBest(func.name, {dictionary(int8(), ty)}, {func.out_type});
+ }
+ }
Review comment:
This seems unnecessarily general, especially given we have only "sign".
Additionally, note that CheckDispatchBest does not assert the output type: it
asserts the type to which arguments must be implicitly cast:
```suggestion
// Sign always outputs to int8
for (std::string name : {"sign", "sign_with_negative_zero"}) {
for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(),
uint16(), uint32(),
uint64(), float32(), float64()}) {
CheckDispatchBest(name, {ty}, {ty});
CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
}
}
```
In the example above, `CheckDispatchBest(name, {ty}, {int8()});` would imply
that we must first cast `ty -> int8()` inputs before invoking the kernel, which
is incorrect. Compare this with `CheckDispatchBest(name, {dictionary(int8(),
ty)}, {ty});` which asserts that we can't directly extract the sign bit from
dictionary encoded data; encoded arrays must first be decoded for the kernel to
operate on them.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
