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

@ZhangHuiGui ZhangHuiGui 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 #40126 has been automatically assigned in GitHub to PR creator.

@ZhangHuiGui

ZhangHuiGui commented Feb 24, 2024

Copy link
Copy Markdown
Contributor Author

@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 mapleFU 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 thread cpp/src/arrow/compute/expression.cc Outdated
Comment thread cpp/src/arrow/compute/expression.cc Outdated
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Feb 25, 2024
@pitrou

pitrou commented Feb 26, 2024

Copy link
Copy Markdown
Member

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

Comment thread cpp/src/arrow/compute/expression.cc Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 26, 2024
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 29, 2024

@bkietz bkietz 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 thread cpp/src/arrow/compute/expression.cc Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Feb 29, 2024
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 29, 2024

@bkietz bkietz 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-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Feb 29, 2024
@bkietz
bkietz merged commit e295a7e into apache:main Feb 29, 2024
@bkietz bkietz 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

mapleFU commented Mar 1, 2024

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
Contributor Author

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 free to 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