Skip to content

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

Open
zanmato1984 wants to merge 1 commit into
apache:mainfrom
zanmato1984:codex/coalesce-exact-dispatch-decimal
Open

GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs#50870
zanmato1984 wants to merge 1 commit into
apache:mainfrom
zanmato1984:codex/coalesce-exact-dispatch-decimal

Conversation

@zanmato1984

Copy link
Copy Markdown
Contributor

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.

…cimal varargs

Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
@zanmato1984
zanmato1984 requested a review from pitrou as a code owner August 15, 2026 09:13
Copilot AI lite review requested due to automatic review settings August 15, 2026 09:13
@github-actions

Copy link
Copy Markdown

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

@zanmato1984

Copy link
Copy Markdown
Contributor Author

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants