Repository navigation
Conversation
github-actions
Bot
added
sqllogictest
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks @shifluxxc Can we have slt tests for
support for the power() function with negative scale decimals (e.g., 1e4 represented as 1 with scale -4) and log for decimal32 decimal64 .
Currently I can see
SELECT power(1e4 ,2)
which might not be covering all edge cases declared in this PR
Co-authored-by: Oleks V <comphead@users.noreply.github.com>
|
I have added slts for other edge cases , please review . |
| Ok(actual_value.log(base)) | ||
| } else { | ||
| let unscaled_value = decimal32_to_i32(value, scale)?; | ||
| let log_value: u32 = unscaled_value.ilog(base as i32); |
There was a problem hiding this comment.
decimal32_to_i32() could return 0 and then 0.ilog() will fail
There was a problem hiding this comment.
I have added basic check to validate that case . It returns Neg infinity now .
There was a problem hiding this comment.
I thought we return NaN for these cases?
Could we also add a test to check this behaviour
There was a problem hiding this comment.
I do think returning NaN is better , I have also added two sqllogic tests to check this behavior .
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks @shifluxxc nice PR
Thanks @Jefffrey and @martin-g for the review
4 participants
Which issue does this PR close?
Rationale for this change
This PR enables support for the
power()function with negative scale decimals (e.g.,1e4represented as1with scale-4) and log for decimal32 decimal64 .What changes are included in this PR?
pow_decimal_intlogic: Added support for negative scaling factors. When the adjustment factor is negative, the function now multiplies by instead of dividing.Are these changes tested?
Yes.
sqllogictest.SELECT power(1e4, 2)which previously returned a "Negative scale is not supported" error.