Skip to content

GH-40126: [C++] Decimal types with different precisions and scales bind failed in resolve type when call arithmetic function - #40223

Merged
bkietz merged 4 commits into
apache:mainfrom
ZhangHuiGui:fix-40126
Feb 29, 2024
Merged

GH-40126: [C++] Decimal types with different precisions and scales bind failed in resolve type when call arithmetic function#40223
bkietz merged 4 commits into
apache:mainfrom
ZhangHuiGui:fix-40126

Conversation

@ZhangHuiGui

@ZhangHuiGuiZhangHuiGui commented Feb 24, 2024

Copy link
Copy Markdown
Contributor

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 IsNeedDispatchBest function to check the decimal types and arithmetic functions, if success we will
go 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.

@github-actions

Copy link
Copy Markdown

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

@ZhangHuiGui

ZhangHuiGui commented Feb 24, 2024

Copy link
Copy Markdown
ContributorAuthor

@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?

@mapleFUmapleFU left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure handling it like this is prefered. cc @felipecrv@pitrou

Comment threadcpp/src/arrow/compute/expression.cc Outdated
Comment threadcpp/src/arrow/compute/expression.cc Outdated
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Feb 25, 2024
@pitrou

Copy link
Copy Markdown
Member

@bkietz Would you like to take a look at this?

Comment threadcpp/src/arrow/compute/expression.cc Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 26, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 29, 2024

@bkietzbkietz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment threadcpp/src/arrow/compute/expression.cc Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Feb 29, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 29, 2024

@bkietzbkietz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, I'll merge when CI completes

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Feb 29, 2024
@bkietz
bkietz merged commit e295a7e into apache:mainFeb 29, 2024
@bkietzbkietz removed the awaiting merge Awaiting merge label Feb 29, 2024
@conbench-apache-arrow

Copy link
Copy Markdown

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.

@mapleFU

Copy link
Copy Markdown
Member

(so, as mentioned: #40223 (comment) . This pr:

  1. Change ResolveDecimalBinaryOperationOutput to return false when precision match failed
  2. When DispatchExact failed, FinishBind would fail, and code will falls to cast and dispatch best? )

@ZhangHuiGui

Copy link
Copy Markdown
ContributorAuthor

2. When DispatchExact failed, FinishBind would fail, and code will falls to cast and dispatch best? )

Actually, the failure of FinishBind has nothing to do with DispatchExact, they are independent logics. We want FinishBind fail if resolve type failed when scale and precision not correct, so the code will falls dispatch best and cast to correct precision and scale as expected rules.

zanmato1984 added a commit that referenced this pull request Aug 20, 2025
… 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>
Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
…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>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ZhangHuiGui@pitrou@mapleFU@bkietz