Add Decimal support for floor preimage #20099
Conversation
github-actions
bot
added
sqllogictest
|
|
||
| // floor always returns an integer, so if value has a fractional part, there's no solution | ||
| // Check: value % one_scaled != 0 means fractional part exists | ||
| if scale > 0 && value % one_scaled != D::Native::ZERO { |
There was a problem hiding this comment.
What about negative scales ?
Arrow supports them.
There was a problem hiding this comment.
Hi, the logic here comes from the idea that floor(x) = 1.3 has no pre-image solution. We do not want to optimize it via this.
floor(x) will always return a floor data type but it will actually be an integer.
// floor always returns an integer, so if n has a fractional part, there's no solution
if n.fract() != F::zero() {
return None;
}
Same logic is carried over here for Decimal.
scale <= 0 is effectively an integer and hence only possibility is to check for overflow which we check in the next line.
There was a problem hiding this comment.
Code review comment in the last PR: #20059 (comment)
|
|
||
| // Decimal32: i32::MAX | ||
| // For scale=2, we add 100, so i32::MAX - 50 would overflow | ||
| assert_preimage_none(ScalarValue::Decimal32(Some(i32::MAX - 50), 9, 2)); |
There was a problem hiding this comment.
What exactly is the idea of this test ?
2147483647 - 50 => 2147483597, has 10 digits and does not fit into precision=9
I think there is no call to add 100 at all here.
There was a problem hiding this comment.
Hi, you are right.
The comment is misleading here. And the test case should move to test_floor_preimage_decimal_non_integer
Decimal32(Some(i32::MAX - 50), 9, 2) is exactly forcing the case explained in your earlier comment.
2147483597 = 21474835.97 which has fractional part and hence the preimage will be None. This test is not the right place for this.
But, 21474835.97 logically is not representable by Decimal(9,2). That is wrong as well. I didn't think too deep here.
Thanks. Let me fix this.
There was a problem hiding this comment.
I removed a bunch of repetitive tests and moved the tests around. Now it should make sense. Please let me know.
|
|
||
| // Decimal types | ||
| ScalarValue::Decimal32(Some(n), precision, scale) => { | ||
| decimal_preimage_bounds::<Decimal32Type>(*n, *precision, *scale).map( |
There was a problem hiding this comment.
maybe we can do a macros here? 🤔
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks @devanshu0987 maybe we can also add overflow slt tests
3 participants
Which issue does this PR close?
floorpreimage #20080Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
No