ShayanGho commented on PR #24643:
URL: https://github.com/apache/datafusion/pull/24643#issuecomment-5739632023
@dhruvdavest07 Thanks for picking this up, and for the thorough description.
I built this branch locally (head 410eaf4) and compared it against PySpark
4.2.0 with ANSI on and off. The normal decimal cases I tested match Spark,
values and result types, including negative scales, a scale larger than the
input scale, column input and NULLs. The ceil unit tests and ceil.slt pass
locally. Note that CI is still awaiting workflow approval.
One correctness issue: overflow produces silent wrong results.
- ceil(3.1411::decimal(5,4), -40) prints
13181135929278455956213638447872186777. Casting the result to Decimal256(76, 0)
shows the stored value is 131811359292784559562136384478721867776, which is
10^40 wrapped at 2^128.
- ceil(3.1411::decimal(5,4), -38) and
ceil(CAST('99999999999999999999999999999999999999' AS decimal(38,0)), -1) both
store 10^38 in a Decimal128(38, 0). That exceeds the declared precision, and it
prints as a plausible-looking 38-digit number.
Spark raises NUMERIC_VALUE_OUT_OF_RANGE for all three, in both ANSI modes.
The cause is the wrapping arithmetic (pow_wrapping in the decimal128_ceil
helper, pow_wrapping and mul_wrapping in decimal128_ceil_to_scale) plus no
check of the result against the output precision. Could these use checked
arithmetic, validate the result, and return an error? Tests for these cases
would be great.
A smaller gap on the scale argument: it has to be a bare literal. ceiling(x,
CAST(3 AS INT)) and ceiling(x, 1+2) fail with "Function ceil requires a
constant integer scale argument", but both are foldable constants that Spark
accepts. On point 1 of the earlier comment: Spark rejects ceil(x, CAST(2 AS
BIGINT)), and TINYINT and NULL scales too, so I would not widen the accepted
types. In this PR that example fails because it is a cast, and 3::int fails the
same way.
On the decimal-only scope, which the description explains: keeping it out of
this PR seems right to me. In case it helps the follow-up, Spark's
numeric-to-decimal mappings are in DecimalType.forType: Int is decimal(10,0),
Long is decimal(20,0), Float is decimal(14,7), Double is decimal(30,15). The
results I measured are consistent with that: ceil(5, -1) = 10 decimal(11,0),
ceil(CAST(5 AS BIGINT), 1) = 5 decimal(21,0), ceil(3.5D, 0) = 4 decimal(16,0),
ceil(CAST(3.14159 AS FLOAT), 2) = 3.15 decimal(10,2).
On the "something a reviewer should decide" question: I confirmed ceil(x, 3)
is still rejected in the planner on current main, for both CEIL and FLOOR. A
follow-up issue covering both seems cleaner than folding planner changes into
this PR. Not my call, just one reviewer's view.
@comphead this PR targets your #21560 . Could you approve the CI workflows
when you get a chance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
