feat: widen output decimal type for decimal ceil/floor

[theirix]

Which issue does this PR close?

• Closes ceil, floor on decimal can produce spurious overflow #22511
• Closes FLOOR decimal returns unexpected error #20640

Rationale for this change

In edge cases, an extra decimal point is required to accommodate a result. Currently, ceil/floor just fail with an overflow.
For example, ceil(-999) is -1000, and it cannot fit into Decimal(4,1) with 3 digits, but only into a decimal type with a lower scale and/or different precision (e.g. Decimal(4,0)).

The proposed experimental fix is to widen the input decimal type to zero scale.

How it's done in other engines:

1. It matches Spark ceil/floor behaviour (limited support in the Spark UDF), when the scale drops to zero, precision is recalculated via p-s+1.

2. DuckDB performs slightly differently, just dropping the scale to zero and keeping precision as input:

select floor('-999.9'::DECIMAL(4,1)); -> -1000::DECIMAL(4,0)
select ceil(9.9::DECIMAL(2,1)); -> 10::DECIMAL(2,0)

3. ClickHouse surprisingly keeps the resulting type as is

SELECT floor(CAST('-999.9', 'DECIMAL(4, 1)')) -> -1000::DECIMAL(4,1)

4. Postgres has its own big integer types, not applicable

From three possible behaviours, we can go with either Spark's or DuckDB's behaviour.
I didn't investigate ClickHouse behaviour yet. Since we already have existing Spark logic in place and it uses precision sparingly, I lean towards it.

What changes are included in this PR?

• Change output type of floor/ceil to Decimal(p-s+1, 0) - could be a breaking change
• Change the logic to match Spark's floor/ceil
• Extend apply_decimal_op to specify output scale (could be different from input scale)
• Change preimage logic to consider both argument and literal precision and scale

Are these changes tested?

• Added examples from two linked issues
• A few more SLTs to cover clamping

Are there any user-facing changes?

Changed floor and ceil UDF output type from the exact input type to a rescaled type with the same bit width. For example, for input Decimal32(7,2) floor now returns Decimal32(6,0)

[theirix]

@neilconway, @Jefffrey I'd appreciate your thoughts on this experimental PR

The mentioned Spark UDF PR is #21933 - that logic could be unified into the core UDF later if we choose the Spark behaviour (1).

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.52542% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.52%. Comparing base (61bf6b9) to head (cd83c45).

Files with missing lines	Patch %	Lines
datafusion/functions/src/math/decimal.rs	77.77%	9 Missing and 1 partial ⚠️
datafusion/functions/src/math/floor.rs	95.76%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24703      +/-   ##
==========================================
- Coverage   81.52%   81.52%   -0.01%     
==========================================
  Files        1123     1123              
  Lines      405970   406086     +116     
  Branches   405970   406086     +116     
==========================================
+ Hits       330978   331071      +93     
- Misses      55627    55646      +19     
- Partials    19365    19369       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Jefffrey]

ill try find time to take a look at this, but cc @kumarUjjawal i think you worked on something similar for round?

[kumarUjjawal]

Thank you @theirix for working on this. I have left few comments for your consideration.

[kumarUjjawal]

can we reuse calculate_new_precision_scale with 0 decimal places?

[theirix]

Unfortunately, no. Based on the code, it will provide the same precision as the input one if places = 0. We need to widen it.

[kumarUjjawal]

we could match on the Ok case and return PreimageResult::None otherwise?

[kumarUjjawal]

This changes the output type of a public SQL function. Any query that stores floor(decimal_col) into a fixed schema, or reads arrow_typeof, sees a different type. We should mention in upgrade guide

[theirix]

Thank you for the review! I've fixed edge cases for the preimage.

Regarding the change - I agree, even if it's a different decimal point type in the decimal domain, it is still a different type. Updated the user-facing changes PR section. If this approach is fine, I'll also add a change to the upgrading notes doc for 56.

[kumarUjjawal]

this might be already there before the pr but would a guard that returns None when one_scaled is zero be worth adding?

[Jefffrey]

ive skimmed this but makes sense to me; if we're going to enable this behaviour (of changing scale) we might as well go all the way like spark, unlike duckdb which doesnt tighten it as much as spark does

the clickhouse one seems surprising, is that a bug? 😅

adding a upgrade notice would be good 👍

[theirix]

ive skimmed this but makes sense to me; if we're going to enable this behaviour (of changing scale) we might as well go all the way like spark, unlike duckdb which doesnt tighten it as much as spark does

Yes, this makes sense. I'll try to refactor Spark's UDF implementation to reuse more from the core.

the clickhouse one seems surprising, is that a bug? 😅

It is usually complicated with ClickHouse - the behaviour is documented, but still contradictory. Turns out, all operations on the decimal type are done on a backing type (int32 if precision is 4, as in the example with Decimal(4,1)) regardless of precision: "Internally data is represented as normal signed integers with respective bit width. Real value ranges that can be stored in memory are a bit larger than specified above, which are checked only on conversion from a string.". We do perform precision checks for most operations. So if the result fits int32, ClickHouse won't complain.

ClickHouse is pretty relaxed on overflow checks (works only for 32- and 64-bit decimals but not for wider). Also, it is not universal - you can easily construct an overflown decimal even when it's enabled, so there is a function isDecimalOverflow, that tells you if you have overflown a calculation.

For example, -99999999.9999 is the smallest number that can fit into DECIMAL32(1) aka DECIMAL(9,1), and -100000000 cannot fit and cannot be constructed - expected behaviour. However, overflowing a value via floor on a valid input silently produces an overflowed value, while isDecimalOverflow shows it is bad.

select version() \G
version(): 26.3.17.4

SET decimal_check_overflow = 1;

SELECT floor(CAST('-99999999.9999', 'DECIMAL32(1)')), toTypeName(floor(CAST('-99999999.9999', 'DECIMAL32(1)')))
-100000000 │ Decimal(9, 1)

select isDecimalOverflow(floor(CAST('-99999999.9999', 'DECIMAL32(1)')));
1

SELECT CAST('-100000000', 'DECIMAL32(1)')
DB::Exception: Decimal value is too big

adding a upgrade notice would be good 👍

Done, thank you!