You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zanmato1984 (via GitHub)" <gi...@apache.org> on 2026/08/15 09:13:48 UTC
[PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
zanmato1984 opened a new pull request, #50870:
URL: https://github.com/apache/arrow/pull/50870
### Rationale for this change
Expression binding tries `DispatchExact` before `DispatchBest`. The `coalesce` decimal varargs kernels used broad decimal signatures, so mixed concrete decimal types could exact-match and bypass the existing decimal normalization and cast insertion in `DispatchBest`. Executing the resulting bound expression then failed with a type compatibility error.
### What changes are included in this PR?
- Add a decimal-only `MatchConstraint` requiring all `coalesce` arguments to have the same full decimal `DataType` for exact dispatch.
- Attach the constraint to decimal128 and decimal256 kernel registrations.
- Add dispatch, expression-binding, and end-to-end regressions covering same-scale/different-precision, crossed precision/scale, reversed argument order, and decimal128/decimal256 inputs.
### Are these changes tested?
Yes. I ran:
- `arrow-compute-expression-test --gtest_filter='Expression.BindWithImplicitCastsForCoalesceOnDecimal:Expression.ExecuteCoalesceOnMixedDecimalTypes'`
- `arrow-compute-scalar-if-else-test --gtest_filter='TestCoalesce.*:TestCoalesceNumeric.*:TestCoalesceBinary.*:TestCoalesceList.*'`
The expression tests (2 tests) and complete `TestCoalesce` selection (13 tests) passed locally.
### AI assistance
I used an AI coding assistant to help inspect the existing `MatchConstraint` patterns, draft the implementation and regression tests, and prepare the issue and pull request text. I reviewed and revised the generated changes, reproduced the bug on current `main`, verified the dispatch and expression-binding behavior before and after the fix, and ran the tests listed above. I understand and take responsibility for the submitted changes. No external copyrighted material was incorporated.
### Are there any user-facing changes?
Yes. `coalesce` expressions with compatible mixed decimal types now bind with casts to a common decimal type and execute successfully instead of failing with a type compatibility error.
* GitHub Issue: #50869
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50870:
URL: https://github.com/apache/arrow/pull/50870#issuecomment-5301550967
:warning: GitHub issue #50869 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #50870:
URL: https://github.com/apache/arrow/pull/50870#issuecomment-5534495543
Hi @pitrou , I've addressed the comments. Mind to take another look? Thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #50870:
URL: https://github.com/apache/arrow/pull/50870
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #50870:
URL: https://github.com/apache/arrow/pull/50870#issuecomment-5301554900
@pitrou, could you please review this when you have a chance? Thanks!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #50870:
URL: https://github.com/apache/arrow/pull/50870#issuecomment-5487289292
Kindly ping @pitrou . Thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50870:
URL: https://github.com/apache/arrow/pull/50870#discussion_r3910816463
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -519,6 +519,22 @@ std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
return instance;
}
+std::shared_ptr<MatchConstraint> AllTypesAreIdenticalFrom(size_t first_type_index) {
+ return MatchConstraint::Make(
+ [first_type_index](const std::vector<TypeHolder>& types) -> bool {
+ DCHECK_LT(first_type_index, types.size());
+ return std::all_of(types.begin() + first_type_index + 1, types.end(),
+ [&types, first_type_index](const TypeHolder& type) {
+ return type == types[first_type_index];
+ });
+ });
Review Comment:
AllTypesAreIdenticalFrom() relies on DCHECK_LT(first_type_index, types.size()) and then unconditionally does iterator arithmetic / indexing. In release builds (DCHECK compiled out), an unexpected call with too few input types would be undefined behavior; it should defensively return false instead of potentially reading past the end of `types`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50870:
URL: https://github.com/apache/arrow/pull/50870#discussion_r3902239522
##########
cpp/src/arrow/compute/kernels/scalar_if_else_test.cc:
##########
@@ -3693,8 +3693,22 @@ TEST(TestCoalesce, DispatchBest) {
CheckDispatchBest("coalesce", {int32(), decimal128(3, 2)},
{decimal128(12, 2), decimal128(12, 2)});
CheckDispatchBest("coalesce", {float32(), decimal128(3, 2)}, {float64(), float64()});
+ CheckDispatchBest("coalesce", {decimal128(3, 2), decimal128(4, 2)},
+ {decimal128(4, 2), decimal128(4, 2)});
+ CheckDispatchBest("coalesce", {decimal128(4, 2), decimal128(3, 2)},
+ {decimal128(4, 2), decimal128(4, 2)});
+ CheckDispatchBest("coalesce", {decimal128(4, 1), decimal128(3, 2)},
+ {decimal128(5, 2), decimal128(5, 2)});
+ CheckDispatchBest("coalesce", {decimal128(3, 2), decimal128(4, 1)},
+ {decimal128(5, 2), decimal128(5, 2)});
+ CheckDispatchBest("coalesce", {decimal128(3, 2), decimal128(4, 3)},
+ {decimal128(4, 3), decimal128(4, 3)});
+ CheckDispatchBest("coalesce", {decimal128(4, 3), decimal128(3, 2)},
+ {decimal128(4, 3), decimal128(4, 3)});
CheckDispatchBest("coalesce", {decimal128(3, 2), decimal256(3, 2)},
{decimal256(3, 2), decimal256(3, 2)});
+ CheckDispatchBest("coalesce", {decimal256(3, 2), decimal128(3, 2)},
Review Comment:
What about e.g. `{decimal256(4, 1), decimal128(3, 2)}`?
##########
cpp/src/arrow/compute/kernels/scalar_if_else.cc:
##########
@@ -2035,6 +2035,20 @@ struct CoalesceFunction : ScalarFunction {
if (auto kernel = DispatchExactImpl(this, *types)) return kernel;
return arrow::compute::detail::NoMatchingKernel(this, *types);
}
+
+ static std::shared_ptr<MatchConstraint> DecimalMatchConstraint() {
Review Comment:
It would be surprising if `if_else` was the only kernel to use this constraint, should it be exposed in `kernel.h`? I see there's already `DecimalsHaveSameScale` there.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #50870:
URL: https://github.com/apache/arrow/pull/50870#discussion_r3910995882
##########
cpp/src/arrow/compute/kernels/scalar_if_else_test.cc:
##########
@@ -3693,8 +3693,22 @@ TEST(TestCoalesce, DispatchBest) {
CheckDispatchBest("coalesce", {int32(), decimal128(3, 2)},
{decimal128(12, 2), decimal128(12, 2)});
CheckDispatchBest("coalesce", {float32(), decimal128(3, 2)}, {float64(), float64()});
+ CheckDispatchBest("coalesce", {decimal128(3, 2), decimal128(4, 2)},
+ {decimal128(4, 2), decimal128(4, 2)});
+ CheckDispatchBest("coalesce", {decimal128(4, 2), decimal128(3, 2)},
+ {decimal128(4, 2), decimal128(4, 2)});
+ CheckDispatchBest("coalesce", {decimal128(4, 1), decimal128(3, 2)},
+ {decimal128(5, 2), decimal128(5, 2)});
+ CheckDispatchBest("coalesce", {decimal128(3, 2), decimal128(4, 1)},
+ {decimal128(5, 2), decimal128(5, 2)});
+ CheckDispatchBest("coalesce", {decimal128(3, 2), decimal128(4, 3)},
+ {decimal128(4, 3), decimal128(4, 3)});
+ CheckDispatchBest("coalesce", {decimal128(4, 3), decimal128(3, 2)},
+ {decimal128(4, 3), decimal128(4, 3)});
CheckDispatchBest("coalesce", {decimal128(3, 2), decimal256(3, 2)},
{decimal256(3, 2), decimal256(3, 2)});
+ CheckDispatchBest("coalesce", {decimal256(3, 2), decimal128(3, 2)},
Review Comment:
Good catch. I added this case in both argument orders. The common type is `decimal256(5, 2)`, and the binding tests verify that both inputs are cast to it.
##########
cpp/src/arrow/compute/kernels/scalar_if_else.cc:
##########
@@ -2035,6 +2035,20 @@ struct CoalesceFunction : ScalarFunction {
if (auto kernel = DispatchExactImpl(this, *types)) return kernel;
return arrow::compute::detail::NoMatchingKernel(this, *types);
}
+
+ static std::shared_ptr<MatchConstraint> DecimalMatchConstraint() {
Review Comment:
Agreed. I moved the constraint to `kernel.h` / `kernel.cc` and generalized it as `AllTypesAreIdentical()` and `AllTypesAreIdenticalFrom(first_type_index)`. `coalesce` uses the former, while `case_when` uses `AllTypesAreIdenticalFrom(1)`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #50870:
URL: https://github.com/apache/arrow/pull/50870#discussion_r3910982585
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -519,6 +519,22 @@ std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
return instance;
}
+std::shared_ptr<MatchConstraint> AllTypesAreIdenticalFrom(size_t first_type_index) {
+ return MatchConstraint::Make(
+ [first_type_index](const std::vector<TypeHolder>& types) -> bool {
+ DCHECK_LT(first_type_index, types.size());
+ return std::all_of(types.begin() + first_type_index + 1, types.end(),
+ [&types, first_type_index](const TypeHolder& type) {
+ return type == types[first_type_index];
+ });
+ });
Review Comment:
Thanks. This follows the existing `MatchConstraint` precondition style: `DecimalsHaveSameScale()` likewise uses `DCHECK` for both the minimum input count and the input kinds. In the normal dispatch path, `Function::CheckArity()` validates the argument count and the per-input signature checks run before the constraint; the current callers guarantee at least one input for `AllTypesAreIdentical()` and at least two for `AllTypesAreIdenticalFrom(1)`.
Therefore, a shorter `types` vector would be a caller contract violation rather than a runtime input condition. I would prefer to keep the `DCHECK` here for consistency. Making all public constraints defensive could be considered separately, including `DecimalsHaveSameScale()`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org