bkietz commented on a change in pull request #10364:
URL: https://github.com/apache/arrow/pull/10364#discussion_r651870110
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1148,5 +1148,326 @@ TYPED_TEST(TestUnaryArithmeticFloating, AbsoluteValue) {
}
}
+class TestBinaryArithmeticDecimal : public TestBase {
+ protected:
+ std::shared_ptr<Scalar> MakeScalar(const std::shared_ptr<DataType>& type,
+ const std::string& str) {
Review comment:
Can you reuse ScalarFromJSON instead?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1148,5 +1148,326 @@ TYPED_TEST(TestUnaryArithmeticFloating, AbsoluteValue) {
}
}
+class TestBinaryArithmeticDecimal : public TestBase {
+ protected:
+ std::shared_ptr<Scalar> MakeScalar(const std::shared_ptr<DataType>& type,
+ const std::string& str) {
+ std::shared_ptr<Scalar> scalar;
+ if (type->id() == Type::DECIMAL128) {
+ Decimal128 value;
+ int32_t dummy;
+ ABORT_NOT_OK(Decimal128::FromString(str, &value, &dummy));
+ ASSIGN_OR_ABORT(scalar, arrow::MakeScalar(type, value));
+ } else {
+ Decimal256 value;
+ int32_t dummy;
+ ABORT_NOT_OK(Decimal256::FromString(str, &value, &dummy));
+ ASSIGN_OR_ABORT(scalar, arrow::MakeScalar(type, value));
+ }
+ return scalar;
+ }
+};
+
Review comment:
Please add some tests specifically of the implicit casting behavior,
using CheckDispatchBest
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -461,6 +706,8 @@ std::shared_ptr<ScalarFunction>
MakeArithmeticFunction(std::string name,
auto exec = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Op>(ty);
DCHECK_OK(func->AddKernel({ty, ty}, ty, exec));
}
+ AddDecimalBinaryKernels<Op>(name, &func);
Review comment:
Please ensure we're not adding decimal kernels for functions which don't
support decimals yet:
```suggestion
// TODO($FOLLOW_UP_JIRA) remove when decimal is supported for all
arithmetic kernels
if (name == "add" || name == "add_checked" || ...) {
AddDecimalBinaryKernels<Op>(name, &func);
}
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -461,6 +706,8 @@ std::shared_ptr<ScalarFunction>
MakeArithmeticFunction(std::string name,
auto exec = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Op>(ty);
DCHECK_OK(func->AddKernel({ty, ty}, ty, exec));
}
+ AddDecimalBinaryKernels<Op>(name, &func);
Review comment:
Please open a follow up JIRA for supporting decimals in the other
arithmetic functions as well
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -451,8 +655,49 @@ struct ArithmeticFunction : ScalarFunction {
if (auto kernel = DispatchExactImpl(this, *values)) return kernel;
return arrow::compute::detail::NoMatchingKernel(this, *values);
}
+
+ Status CheckDecimals(std::vector<ValueDescr>* values) const {
+ bool has_decimal = false;
+ for (const auto& value : *values) {
+ if (is_decimal(value.type->id())) {
+ has_decimal = true;
+ break;
+ }
+ }
+ if (!has_decimal) return Status::OK();
+
+ if (values->size() == 2) {
+ return CastBinaryDecimalArgs(name(), values);
+ }
+ return Status::OK();
+ }
+};
+
+// resolve decimal operation output type
+struct BinaryDecimalOutputResolver {
+ const std::string func_name;
+
+ explicit BinaryDecimalOutputResolver(std::string func_name)
+ : func_name(std::move(func_name)) {}
+
+ Result<ValueDescr> operator()(KernelContext*, const std::vector<ValueDescr>&
args) {
+ ARROW_ASSIGN_OR_RAISE(auto type, ResolveBinaryDecimalOutput(func_name,
args));
+ return ValueDescr(std::move(type), GetBroadcastShape(args));
+ }
};
+template <typename Op>
+void AddDecimalBinaryKernels(const std::string& name,
+ std::shared_ptr<ArithmeticFunction>* func) {
+ auto out_type = OutputType(BinaryDecimalOutputResolver(name));
Review comment:
I think it'd be more straightforward to dispatch on name here:
```suggestion
OutputType out_type;
const std::string op = func_name.substr(0, func_name.find("_"));
if (op == "add" || op == "subtract") {
out_type = OutputType(ResolveDecimalAdditionOrSubtractionOutput);
} else if (op == "multiply") {
out_type = OutputType(ResolveDecimalMultiplicationOutput);
} else if (op == "divide") {
out_type = OutputType(ResolveDecimalDivisionOutput);
} else {
DCHECK(false);
}
```
--
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]
