This is an automated email from the ASF dual-hosted git repository.
zanmato pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new deb73a70ba GH-35957: [C++][Compute] Graceful error for decimal binary
arithmetic and comparison instead of firing confusing assertion (#48639)
deb73a70ba is described below
commit deb73a70ba8dd57be489f5d7fabbcfc74f574dfe
Author: Rossi Sun <[email protected]>
AuthorDate: Mon Jan 5 17:40:01 2026 +0800
GH-35957: [C++][Compute] Graceful error for decimal binary arithmetic and
comparison instead of firing confusing assertion (#48639)
### Rationale for this change
When dispatching binary arithmetic and comparison kernels, we do a special
casting ahead for decimal arguments. If one argument is decimal and another is
the type not castable (e.g., string) to decimal, an assertion fires. On the
other hand, we have a graceful way to error on dispatch failure in the general
kernel dispatching path after this special casting:
```
Function 'greater' has no kernel matching input types (string, double)
```
We want to unify the error path for decimal.
### What changes are included in this PR?
Bypass the decimal casting early and not error out if we see the other
argument is not castable to decimal, and let the subsequent general kernel
dispatching path to handle the error gracefully.
### Are these changes tested?
Test included.
### Are there any user-facing changes?
None.
* GitHub Issue: #35957
Authored-by: Rossi Sun <[email protected]>
Signed-off-by: Rossi Sun <[email protected]>
---
cpp/src/arrow/compute/kernels/codegen_internal.cc | 11 +++++++++++
.../arrow/compute/kernels/codegen_internal_test.cc | 16 +++++++++++++++
.../compute/kernels/scalar_arithmetic_test.cc | 23 ++++++++++++++++++++++
.../arrow/compute/kernels/scalar_compare_test.cc | 20 +++++++++++++++++++
4 files changed, 70 insertions(+)
diff --git a/cpp/src/arrow/compute/kernels/codegen_internal.cc
b/cpp/src/arrow/compute/kernels/codegen_internal.cc
index 10ed9344d9..99a9173041 100644
--- a/cpp/src/arrow/compute/kernels/codegen_internal.cc
+++ b/cpp/src/arrow/compute/kernels/codegen_internal.cc
@@ -393,11 +393,22 @@ TypeHolder CommonBinary(const TypeHolder* begin, size_t
count) {
return large_binary();
}
+bool CastableToDecimal(const DataType& type) {
+ return is_numeric(type.id()) || is_decimal(type.id());
+}
+
Status CastBinaryDecimalArgs(DecimalPromotion promotion,
std::vector<TypeHolder>* types) {
const DataType& left_type = *(*types)[0];
const DataType& right_type = *(*types)[1];
DCHECK(is_decimal(left_type.id()) || is_decimal(right_type.id()));
+ if ((is_decimal(left_type.id()) && !CastableToDecimal(right_type)) ||
+ (is_decimal(right_type.id()) && !CastableToDecimal(left_type))) {
+ // If the other type is not castable to decimal, do not cast. The dispatch
will
+ // gracefully fail by kernel selection.
+ return Status::OK();
+ }
+
// decimal + float64 = float64
// decimal + float32 is roughly float64 + float32 so we choose float64
if (is_floating(left_type.id()) || is_floating(right_type.id())) {
diff --git a/cpp/src/arrow/compute/kernels/codegen_internal_test.cc
b/cpp/src/arrow/compute/kernels/codegen_internal_test.cc
index 8aa90823c1..8596aa7680 100644
--- a/cpp/src/arrow/compute/kernels/codegen_internal_test.cc
+++ b/cpp/src/arrow/compute/kernels/codegen_internal_test.cc
@@ -51,6 +51,22 @@ TEST(TestDispatchBest, CastBinaryDecimalArgs) {
EXPECT_RAISES_WITH_MESSAGE_THAT(
NotImplemented, ::testing::HasSubstr("Decimals with negative scales not
supported"),
CastBinaryDecimalArgs(DecimalPromotion::kAdd, &args));
+
+ // Non-castable -> unchanged
+ for (const auto promotion :
+ {DecimalPromotion::kAdd, DecimalPromotion::kMultiply,
DecimalPromotion::kDivide}) {
+ for (const auto& args : std::vector<std::vector<TypeHolder>>{
+ {decimal128(3, 2), boolean()},
+ {boolean(), decimal128(3, 2)},
+ {decimal128(3, 2), utf8()},
+ {utf8(), decimal128(3, 2)},
+ }) {
+ auto args_copy = args;
+ ASSERT_OK(CastBinaryDecimalArgs(promotion, &args_copy));
+ AssertTypeEqual(*args_copy[0], *args[0]);
+ AssertTypeEqual(*args_copy[1], *args[1]);
+ }
+ }
}
TEST(TestDispatchBest, CastDecimalArgs) {
diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
index 9ff3a7fa18..9367ad2c89 100644
--- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
@@ -2494,6 +2494,29 @@ TEST_F(TestBinaryArithmeticDecimal, Power) {
}
}
+TEST_F(TestBinaryArithmeticDecimal, ErrorOnNonCastable) {
+ for (const auto& name : {"add", "subtract", "multiply", "divide"}) {
+ for (const auto& suffix : {"", "_checked"}) {
+ auto func = std::string(name) + suffix;
+ SCOPED_TRACE(func);
+ for (const auto& dec_ty : PositiveScaleTypes()) {
+ SCOPED_TRACE(dec_ty->ToString());
+ auto dec_arr = ArrayFromJSON(dec_ty, R"([])");
+ for (const auto& other_ty : {boolean(), fixed_size_binary(42),
utf8()}) {
+ SCOPED_TRACE(other_ty->ToString());
+ auto other_arr = ArrayFromJSON(other_ty, R"([])");
+ EXPECT_RAISES_WITH_MESSAGE_THAT(NotImplemented,
+ ::testing::HasSubstr("has no kernel
matching"),
+ CallFunction(func, {dec_arr,
other_arr}));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(NotImplemented,
+ ::testing::HasSubstr("has no kernel
matching"),
+ CallFunction(func, {other_arr,
dec_arr}));
+ }
+ }
+ }
+ }
+}
+
TYPED_TEST(TestBinaryArithmeticIntegral, ShiftLeft) {
for (auto check_overflow : {false, true}) {
this->SetOverflowCheck(check_overflow);
diff --git a/cpp/src/arrow/compute/kernels/scalar_compare_test.cc
b/cpp/src/arrow/compute/kernels/scalar_compare_test.cc
index 9372955fb4..23c7ab21bd 100644
--- a/cpp/src/arrow/compute/kernels/scalar_compare_test.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_compare_test.cc
@@ -681,6 +681,26 @@ TYPED_TEST(TestCompareDecimal, DifferentParameters) {
}
}
+TYPED_TEST(TestCompareDecimal, ErrorOnNonCastable) {
+ auto dec_ty = std::make_shared<TypeParam>(3, 2);
+ auto dec_arr = ArrayFromJSON(dec_ty, R"([])");
+
+ for (const auto& func :
+ {"equal", "not_equal", "less", "less_equal", "greater",
"greater_equal"}) {
+ SCOPED_TRACE(func);
+ for (const auto& other_ty : {boolean(), fixed_size_binary(42), utf8()}) {
+ SCOPED_TRACE(other_ty->ToString());
+ auto other_arr = ArrayFromJSON(other_ty, R"([])");
+ EXPECT_RAISES_WITH_MESSAGE_THAT(NotImplemented,
+ ::testing::HasSubstr("has no kernel
matching"),
+ CallFunction(func, {dec_arr,
other_arr}));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(NotImplemented,
+ ::testing::HasSubstr("has no kernel
matching"),
+ CallFunction(func, {other_arr,
dec_arr}));
+ }
+ }
+}
+
// Helper to organize tests for fixed size binary comparisons
struct CompareCase {
std::shared_ptr<DataType> lhs_type;
