GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs

[zanmato1984]

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: [C++][Compute] coalesce expression binding skips decimal normalization for mixed decimal types #50869

[github-actions]

⚠️ GitHub issue #50869has been automatically assigned in GitHub to PR creator.

[zanmato1984]

@pitrou, could you please review this when you have a chance? Thanks!

[Copilot]

Pull request overview

This PR prevents coalesce expression binding from selecting a too-broad “exact” decimal varargs kernel when decimal arguments differ in precision/scale, ensuring the binder falls back to DispatchBest so decimal normalization and implicit casts are applied before execution.

Changes:

• Introduces a decimal-only MatchConstraint for coalesce exact dispatch that requires all arguments to have identical concrete decimal DataType (precision/scale and width).
• Attaches that constraint to the decimal128 and decimal256 varargs kernel registrations for coalesce.
• Adds regression tests covering both dispatch behavior (DispatchExact/DispatchBest) and expression bind/execute for mixed decimal types.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
cpp/src/arrow/compute/kernels/scalar_if_else.cc	Adds and wires a decimal-only exact-dispatch constraint for coalesce decimal varargs kernels.
cpp/src/arrow/compute/kernels/scalar_if_else_test.cc	Adds dispatch regressions ensuring mixed concrete decimal types do not exact-dispatch and instead normalize via DispatchBest.
cpp/src/arrow/compute/expression_test.cc	Adds expression-binding and end-to-end execution regressions for mixed decimal inputs to coalesce.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cpp/src/arrow/compute/kernels/scalar_if_else.cc:2814

• AddCoalesceKernel now supports MatchConstraint, but it’s only applied to decimal kernels. The DictionaryType coalesce kernel signature is still broad (InputType(Type::DICTIONARY)), so expression binding can exact-dispatch mixed dictionary types (different value/index types) and then fail at execution time in CheckIdenticalTypes. Consider adding an exact-dispatch constraint for dictionary coalesce kernels requiring all args to have identical full DataType, so mixed dictionaries fall back to DispatchBest (which already decodes dictionaries before selecting a kernel).

void AddCoalesceKernel(const std::shared_ptr<ScalarFunction>& scalar_function,
detail::GetTypeId get_id, ArrayKernelExec exec,
std::shared_ptr<MatchConstraint> constraint = nullptr) {
ScalarKernel kernel(KernelSignature::Make({InputType(get_id.id)}, FirstType,
/*is_varargs=*/true, std::move(constraint)),
exec);

[zanmato1984]

Kindly ping @pitrou . Thanks.

[pitrou]

What about e.g. {decimal256(4, 1), decimal128(3, 2)}?

[zanmato1984]

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.

[pitrou]

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.

[zanmato1984]

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).

[Copilot]

🟡 Changes recommended

The newly added AllTypesAreIdenticalFrom constraint can invoke undefined behavior in release builds when called with too few input types due to reliance on DCHECK only.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[zanmato1984]

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().

[Copilot]

🟢 Approval recommended

The constraint change is narrowly scoped to decimal exact dispatch and is supported by targeted dispatch, binding, and execution regressions covering the reported failure modes.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[zanmato1984]

Hi @pitrou , I've addressed the comments. Mind to take another look? Thanks.