GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs - #50870
Open
zanmato1984 wants to merge 1 commit into
Open
GH-50869: [C++][Compute] Tighten coalesce exact dispatch for decimal varargs#50870zanmato1984 wants to merge 1 commit into
zanmato1984 wants to merge 1 commit into
Conversation
…cimal varargs Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
|
|
Contributor
Author
|
@pitrou, could you please review this when you have a chance? Thanks! |
Contributor
There was a problem hiding this comment.
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
MatchConstraintforcoalesceexact dispatch that requires all arguments to have identical concrete decimalDataType(precision/scale and width). - Attaches that constraint to the
decimal128anddecimal256varargs kernel registrations forcoalesce. - 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Expression binding tries
DispatchExactbeforeDispatchBest. Thecoalescedecimal varargs kernels used broad decimal signatures, so mixed concrete decimal types could exact-match and bypass the existing decimal normalization and cast insertion inDispatchBest. Executing the resulting bound expression then failed with a type compatibility error.What changes are included in this PR?
MatchConstraintrequiring allcoalescearguments to have the same full decimalDataTypefor exact dispatch.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
TestCoalesceselection (13 tests) passed locally.AI assistance
I used an AI coding assistant to help inspect the existing
MatchConstraintpatterns, 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 currentmain, 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.
coalesceexpressions 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.