GH-40126: [C++] Decimal types with different precisions and scales bind failed in resolve type when call arithmetic function - #40223
Conversation
|
|
|
@bkietz PTAL, the decimal types with arithmetic function need the user cast by their own in the past, for these changes, we could go into a correct path and do implicit cast internal. Any suggestions? |
mapleFU
left a comment
There was a problem hiding this comment.
I'm not sure handling it like this is prefered. cc @felipecrv @pitrou
|
@bkietz Would you like to take a look at this? |
7786cd6 to
ea0cfb2
Compare
bkietz
left a comment
There was a problem hiding this comment.
Please rebase to pick up the fix for https://github.com/apache/arrow/actions/runs/8092590090/job/22113575436?pr=40223#step:6:6328
3673071 to
7a4a003
Compare
7a4a003 to
13b6b42
Compare
bkietz
left a comment
There was a problem hiding this comment.
LGTM, I'll merge when CI completes
|
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit e295a7e. There was 1 benchmark result with an error:
There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them. |
|
(so, as mentioned: #40223 (comment) . This pr:
|
Actually, the failure of |
… and use it for binary decimal arithmetic kernels (#47297) ### Rationale for this change A rework of #40223 using a more systematic alternative. ### What changes are included in this PR? Introduce a structure `MatchConstraint` for applying extra (and optional) matching constraint for kernel signature matching, in additional to simply input type checks. Also implement two concrete `MatchConstraint`s for binary decimal arithmetic kernels, to suppress exact match even if the input types are OK, for example, by requiring all decimal must be of the same scale for `add` and `subtract`, and s1 >= s2 for `divide`. This should also be a fundamental enhancement to further resolve similar issues like: * #35843 * #39875 * #40911 * #41011 * #41336 (Haven't try each one of them. May do that if this PR gets merged.) ### Are these changes tested? UT included. ### Are there any user-facing changes? New public class `MatchConstraint`. * GitHub Issue: #47287 Authored-by: Rossi Sun <zanmato1984@gmail.com> Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
…tching and use it for binary decimal arithmetic kernels (apache#47297) ### Rationale for this change A rework of apache#40223 using a more systematic alternative. ### What changes are included in this PR? Introduce a structure `MatchConstraint` for applying extra (and optional) matching constraint for kernel signature matching, in additional to simply input type checks. Also implement two concrete `MatchConstraint`s for binary decimal arithmetic kernels, to suppress exact match even if the input types are OK, for example, by requiring all decimal must be of the same scale for `add` and `subtract`, and s1 >= s2 for `divide`. This should also be a fundamental enhancement to further resolve similar issues like: * apache#35843 * apache#39875 * apache#40911 * apache#41011 * apache#41336 (Haven't try each one of them. May do that if this PR gets merged.) ### Are these changes tested? UT included. ### Are there any user-facing changes? New public class `MatchConstraint`. * GitHub Issue: apache#47287 Authored-by: Rossi Sun <zanmato1984@gmail.com> Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
Rationale for this change
Fix decimal types with different precisions and scales bind failed in resolve type when call arithmetic function.
What changes are included in this PR?
Add
IsNeedDispatchBestfunction to check the decimal types and arithmetic functions, if success we willgo into the dispatchBest path and do the implicit cast correctly.
Are these changes tested?
Yes
Are there any user-facing changes?
Yes, user needn't do their own cast for decimal related logic.