GH-41336: [C++][Compute] Fix the bug of decimal types skipping cast in IfElse related expression function calls#41363
GH-41336: [C++][Compute] Fix the bug of decimal types skipping cast in IfElse related expression function calls#41363ZhangHuiGui wants to merge 2 commits intoapache:mainfrom
Conversation
|
|
There was a problem hiding this comment.
The returned invalid status used in the logic :
https://github.com/bkietz/arrow/blob/c25866bc59e30e43e8dc3b05f3973d48074c3594/cpp/src/arrow/compute/expression.cc#L551-L552
Let the expression call go into DispatchBest so they can be cast correctly.
There was a problem hiding this comment.
In BindNonRecursive:
FinishBind will do output type resolve. For the first time go into the resolver, our decimal related types haven't cast, we could check if the decimal types have different precision and scale.
And then return invalid to make the BindNonRecursive go into IfElse's related function's DispatchBest and do cast.
After that, when we go into type resolver in BindNoRecursive second time, we could find the decimal types already cast to same precision and scale, so we could find the correct result output type.
github-actions
bot
added
awaiting committer review
| }; | ||
|
|
||
| template <typename ResolverForOtherTypes> | ||
| Result<TypeHolder> ResolveDecimalCaseType(KernelContext* ctx, |
There was a problem hiding this comment.
Name suggestion: ResolveDecimalCaseWhenOutputType
Because "DecimalCase" is ambiguous by "case" being a word that would fit here, but it actually refers to the kernel function called "CaseWhen".
| if (is_floating(type.id()) || is_integer(type.id())) { | ||
| return Status::Invalid("Need to cast numeric types containing decimal types"); |
There was a problem hiding this comment.
I think a better approach for these failures is to have kernels that fail. You write input matchers to cover all cases and dispatch to the fail kernels for types that can't be handled without casts.
There was a problem hiding this comment.
Yes, the approach with TypeMatcher is more suitable. If there is no problem with the PR #41012 that contains a new matcher interface for combination-types, current PR will continue to be promoted.
github-actions
bot
added
awaiting changes
|
Issue fixed by #47479. Closing this one. Thanks you all the same @ZhangHuiGui ! |
4 participants
Rationale for this change
Fix the bug of decimal types skipping cast in IfElse related expression function calls
What changes are included in this PR?
if_else,case_when,coalesce.Output resolver will make sure the input decimal types casted correctly by their
DispatchBestin expression call.cast and execute correctly as
CallFunction.Are these changes tested?
Yes
Are there any user-facing changes?
No
Co-authored-by ZhangHuiGui 2689496754@qq.com
Co-authored-by laotan332 qinpengxiang@outlook.com