GH-41011: [C++][Compute] Fix the issue that comparison function could not handle decimal arguments with different scales#47459
Conversation
…ents with different scale
| auto exec = GenerateDecimal<applicator::ScalarBinaryEqualTypes, BooleanType, Op>(id); | ||
| DCHECK_OK( | ||
| func->AddKernel({InputType(id), InputType(id)}, boolean(), std::move(exec))); | ||
| DCHECK_OK(func->AddKernel({InputType(id), InputType(id)}, boolean(), std::move(exec), |
There was a problem hiding this comment.
Yep, the fix is as simple as this. Thanks to #47297.
| // decimal int | ||
| ExpectBindsTo(cmp(field_ref("dec128_3_2"), field_ref("i64")), | ||
| cmp(field_ref("dec128_3_2"), cast(field_ref("i64"), decimal128(21, 2))), | ||
| /*bound_out=*/nullptr, *exciting_schema); | ||
| ExpectBindsTo(cmp(field_ref("i64"), field_ref("dec128_3_2")), | ||
| cmp(cast(field_ref("i64"), decimal128(21, 2)), field_ref("dec128_3_2")), | ||
| /*bound_out=*/nullptr, *exciting_schema); | ||
|
|
||
| // decimal128 decimal256 with different scales | ||
| ExpectBindsTo( | ||
| cmp(field_ref("dec128_3_2"), field_ref("dec256_5_3")), | ||
| cmp(cast(field_ref("dec128_3_2"), decimal256(4, 3)), field_ref("dec256_5_3")), | ||
| /*bound_out=*/nullptr, *exciting_schema); | ||
| ExpectBindsTo( | ||
| cmp(field_ref("dec256_5_3"), field_ref("dec128_3_2")), | ||
| cmp(field_ref("dec256_5_3"), cast(field_ref("dec128_3_2"), decimal256(4, 3))), | ||
| /*bound_out=*/nullptr, *exciting_schema); | ||
| ExpectBindsTo(cmp(field_ref("dec128_5_3"), field_ref("dec256_3_2")), | ||
| cmp(cast(field_ref("dec128_5_3"), decimal256(5, 3)), | ||
| cast(field_ref("dec256_3_2"), decimal256(4, 3))), | ||
| /*bound_out=*/nullptr, *exciting_schema); | ||
| ExpectBindsTo(cmp(field_ref("dec256_3_2"), field_ref("dec128_5_3")), | ||
| cmp(cast(field_ref("dec256_3_2"), decimal256(4, 3)), | ||
| cast(field_ref("dec128_5_3"), decimal256(5, 3))), | ||
| /*bound_out=*/nullptr, *exciting_schema); |
There was a problem hiding this comment.
These cases will actually pass w/o this fix. Just added them for intact coverage of all decimal casting paths.
| // decimal decimal with different scales | ||
| ExpectBindsTo( | ||
| cmp(field_ref("dec128_3_2"), field_ref("dec128_5_3")), | ||
| cmp(cast(field_ref("dec128_3_2"), decimal128(4, 3)), field_ref("dec128_5_3")), | ||
| /*bound_out=*/nullptr, *exciting_schema); | ||
| ExpectBindsTo( | ||
| cmp(field_ref("dec128_5_3"), field_ref("dec128_3_2")), | ||
| cmp(field_ref("dec128_5_3"), cast(field_ref("dec128_3_2"), decimal128(4, 3))), | ||
| /*bound_out=*/nullptr, *exciting_schema); |
There was a problem hiding this comment.
These two will fail w/o the fix.
github-actions
bot
added
awaiting committer review
| } | ||
|
|
||
| TEST(Expression, BindWithImplicitCasts) { | ||
| auto exciting_schema = schema( |
| cmp(cast(field_ref("i64"), decimal128(21, 2)), field_ref("dec128_3_2")), | ||
| /*bound_out=*/nullptr, *exciting_schema); | ||
|
|
||
| // decimal128 decimal256 with different scales |
There was a problem hiding this comment.
What happens with the same scale and different precisions, btw? Ideally no cast would occur since the raw values can be compared directly, but regardless we might add tests for that situation as well?
There was a problem hiding this comment.
You are right, no casts applied for different precisions but same scale.
Added the cases. Thank you!
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
LGTM on the principle, some small suggestions
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
|
I'll merge. Thanks for reviewing @pitrou ! |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2987165. There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
2 participants
Rationale for this change
We used to be not able to suppress the exact matching for decimal arguments with different scales, when a decimal comparison kernel who actually requires the scales to be the same. This caused issue like #41011.
The "match constraint" introduced in #47297 is exactly for fixing issues like this, by simply adding a proper constraint.
What changes are included in this PR?
Added match constraint that requires all decimal inputs have the same scale (like for decimal addition and subtract).
Are these changes tested?
Yes.
Are there any user-facing changes?
None.