Add Decimal support for floor preimage

[devanshu0987]

Which issue does this PR close?

• Closes Add Decimal support for floor preimage #20080

Rationale for this change

What changes are included in this PR?

• Decimal support
• SLT Tests for Floor preimage

Are these changes tested?

• Unit Tests
• SLT Tests

Are there any user-facing changes?

No

[devanshu0987]

@comphead @sdf-jkl Would request your review when you get time. Thanks.

[martin-g]

What about negative scales ?
Arrow supports them.

[devanshu0987]

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.

[devanshu0987]

Code review comment in the last PR: #20059 (comment)

[martin-g]

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.

[devanshu0987]

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.

[devanshu0987]

I removed a bunch of repetitive tests and moved the tests around. Now it should make sense. Please let me know.

[comphead]

Thanks @devanshu0987 maybe we can also add overflow slt tests

[devanshu0987]

Thanks @devanshu0987 maybe we can also add overflow slt tests

Hi @comphead , after checking, the situation is following

• floor only accepts Decimal/Float64/Float32
• The result is either Float64/Float32/Decimal

---

• Hence, all Ints on the right hand side are coerced to Floats/Decimal. Hence, they cannot be tested via SLT tests.
• For Float precision loss, I have added a SLT test case.
• For Decimal, via SLT/SQL path, all Decimals are either Decimal128/Decimal256
  • As per rules here: https://datafusion.apache.org/user-guide/sql/data_types.html#numeric-types
• Before reaching preimage stage, the Decimal values are validated against MAX_DECIMAL128_FOR_EACH_PRECISION and MAX_DECIMAL256_FOR_EACH_PRECISION
  • They reject i128:MAX and i256:MAX (which are the only values which can overflow by adding 1) before reaching preimage stage.
• Due to this, Decimal path is not testable via SQL/SLT path. Hence, no SLT tests.

Please let me know, if the reasoning is valid.

[comphead]

Thanks @devanshu0987 it is LGTM, I'll wait for @martin-g if they have anything else to add

[alamb]

@devanshu0987 it seems like a similar technique could be used for ceil -- if you agree I will file a ticket to track

[sdf-jkl]

@alamb in #20051 (comment) you mention that it's better to keep slt tests in floor.rs instead of floor_preimage.rs, but the original floor slt tests are in a different file for multiple scalar functions:

datafusion/datafusion/sqllogictest/test_files/scalar.slt

Line 484 in b80bf2c

## floor

Should we move all the new slt tests there?

[devanshu0987]

@devanshu0987 it seems like a similar technique could be used for ceil -- if you agree I will file a ticket to track

Hi @alamb , I would like to attempt this technique for the following functions

• ceil and round
  • ceil(x) = 5 -> (4, 5]
  • round(x) = 5 -> [4.5, 5.5)
• String Prefix Functions
  • left(s, 2) = 'AB' -> Lexicographically ['AB', 'AC')
    • Not sure if this is already implemented.
    • Not sure how complex this will be for Non latin strings etc.