Skip to content

Commit 3356f85

Browse files
parthchandraparthchandra
authored
fix: fix decimal div and add tests (apache#3952)
* fix: fix decimal div and add tests
1 parent 27a89f2 commit 3356f85

5 files changed

Lines changed: 400 additions & 18 deletions

File tree

‎native/spark-expr/src/math_funcs/div.rs‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,17 @@ fn spark_decimal_div_internal(
8888
arrow::compute::kernels::arity::try_binary(left, right, |l, r| {
8989
let l = BigInt::from(l) * &l_mul;
9090
let r = BigInt::from(r) * &r_mul;
91-
if eval_mode == EvalMode::Ansi && is_integral_div && r.is_zero() {
91+
// Previously this check included `&& is_integral_div`, so regular decimal `/`
92+
// silently returned 0 for a zero divisor in ANSI mode instead of throwing.
93+
// Spark throws DIVIDE_BY_ZERO for both `/` and `div` when ANSI is enabled, so
94+
// the `is_integral_div` guard was wrong and has been removed.
95+
if eval_mode == EvalMode::Ansi && r.is_zero() {
9296
return Err(ArrowError::ComputeError(divide_by_zero_error().to_string()));
9397
}
98+
// Non-ANSI: zero divisors have already been replaced with null by the
99+
// `nullIfWhenPrimitive` wrapper applied in the Scala serde layer, so
100+
// `try_binary` will never invoke this closure for a zero `r` in legacy/try mode.
101+
// The fallback `zero.clone()` is therefore unreachable in practice.
94102
let div = if r.eq(&zero) { zero.clone() } else { &l / &r };
95103
let res = if is_integral_div {
96104
div
@@ -107,9 +115,17 @@ fn spark_decimal_div_internal(
107115
arrow::compute::kernels::arity::try_binary(left, right, |l, r| {
108116
let l = l * l_mul;
109117
let r = r * r_mul;
110-
if eval_mode == EvalMode::Ansi && is_integral_div && r.is_zero() {
118+
// Previously this check included `&& is_integral_div`, so regular decimal `/`
119+
// silently returned 0 for a zero divisor in ANSI mode instead of throwing.
120+
// Spark throws DIVIDE_BY_ZERO for both `/` and `div` when ANSI is enabled, so
121+
// the `is_integral_div` guard was wrong and has been removed.
122+
if eval_mode == EvalMode::Ansi && r == 0 {
111123
return Err(ArrowError::ComputeError(divide_by_zero_error().to_string()));
112124
}
125+
// Non-ANSI: zero divisors have already been replaced with null by the
126+
// `nullIfWhenPrimitive` wrapper applied in the Scala serde layer, so
127+
// `try_binary` will never invoke this closure for a zero `r` in legacy/try mode.
128+
// The fallback `0` is therefore unreachable in practice.
113129
let div = if r == 0 { 0 } else { l / r };
114130
let res = if is_integral_div {
115131
div

‎spark/src/main/scala/org/apache/comet/serde/arithmetic.scala‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ object CometDivide extends CometExpressionSerde[Divide] with MathBase {
166166
withInfo(expr, s"Unsupported datatype ${expr.left.dataType}")
167167
return None
168168
}
169-
createMathExpression(
169+
val divideExpr = createMathExpression(
170170
expr,
171171
expr.left,
172172
rightExpr,
@@ -175,6 +175,21 @@ object CometDivide extends CometExpressionSerde[Divide] with MathBase {
175175
expr.dataType,
176176
expr.evalMode,
177177
(builder, mathExpr) => builder.setDivide(mathExpr))
178+
179+
// For decimal division Spark applies CheckOverflow after dividing: in ANSI mode overflow
180+
// throws NUMERIC_VALUE_OUT_OF_RANGE; in legacy/try mode it returns null. The Rust
181+
// spark_decimal_div_internal uses i128::MAX as a sentinel for overflow, so without this
182+
// wrapper an ANSI overflow would silently return a garbage value instead of throwing.
183+
if (divideExpr.isDefined && expr.dataType.isInstanceOf[DecimalType] &&
184+
serializeDataType(expr.dataType).isDefined) {
185+
val builder = ExprOuterClass.CheckOverflow.newBuilder()
186+
builder.setChild(divideExpr.get)
187+
builder.setFailOnError(expr.evalMode == EvalMode.ANSI)
188+
builder.setDatatype(serializeDataType(expr.dataType).get)
189+
Some(ExprOuterClass.Expr.newBuilder().setCheckOverflow(builder).build())
190+
} else {
191+
divideExpr
192+
}
178193
}
179194
}
180195

‎spark/src/test/resources/sql-tests/expressions/decimal/decimal_div.sql‎

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
-- Licensed to the Apache Software Foundation (ASF) under one
2+
-- or more contributor license agreements. See the NOTICE file
3+
-- distributed with this work for additional information
4+
-- regarding copyright ownership. The ASF licenses this file
5+
-- to you under the Apache License, Version 2.0 (the
6+
-- "License"); you may not use this file except in compliance
7+
-- with the License. You may obtain a copy of the License at
8+
--
9+
-- http://www.apache.org/licenses/LICENSE-2.0
10+
--
11+
-- Unless required by applicable law or agreed to in writing,
12+
-- software distributed under the License is distributed on an
13+
-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
-- KIND, either express or implied. See the License for the
15+
-- specific language governing permissions and limitations
16+
-- under the License.
17+
18+
-- Decimal division (/) and integral division (div) in legacy (non-ANSI) mode.
19+
-- Also covers try_divide (TRY mode semantics).
20+
-- See decimal_div_ansi.sql for ANSI mode tests.
21+
22+
-- ConfigMatrix: parquet.enable.dictionary=false,true
23+
24+
-- ============================================================================
25+
-- Basic divide and integral divide
26+
-- ============================================================================
27+
28+
statement
29+
CREATE TABLE test_decimal_div(a decimal(18,6), b decimal(18,6)) USING parquet
30+
31+
statement
32+
INSERT INTO test_decimal_div VALUES
33+
(10.000000, 3.000000),
34+
(7.500000, 2.500000),
35+
(-10.000000, 3.000000),
36+
(10.000000, -3.000000),
37+
(-10.000000,-3.000000),
38+
(0.000000, 5.000000),
39+
(1.000000, 3.000000),
40+
(NULL, 3.000000),
41+
(10.000000, NULL)
42+
43+
-- column / column
44+
query
45+
SELECT a / b FROM test_decimal_div
46+
47+
-- column div column (integral division: drops fractional part)
48+
query
49+
SELECT a div b FROM test_decimal_div
50+
51+
-- ============================================================================
52+
-- Divide by zero: legacy mode returns NULL, not an error
53+
-- ============================================================================
54+
55+
statement
56+
CREATE TABLE test_decimal_div_zero(a decimal(18,6)) USING parquet
57+
58+
statement
59+
INSERT INTO test_decimal_div_zero VALUES
60+
(10.000000),
61+
(-5.000000),
62+
(0.000000),
63+
(NULL)
64+
65+
-- a / 0 returns null in legacy mode
66+
query
67+
SELECT a / cast(0 as decimal(18,6)) FROM test_decimal_div_zero
68+
69+
-- a div 0 returns null in legacy mode
70+
query
71+
SELECT a div cast(0 as decimal(18,6)) FROM test_decimal_div_zero
72+
73+
-- ============================================================================
74+
-- TRY mode: try_divide always returns NULL on zero divisor
75+
-- ============================================================================
76+
77+
-- try_divide by zero returns null even when ANSI mode is on externally
78+
query
79+
SELECT try_divide(a, cast(0.000000 as decimal(18,6))) FROM test_decimal_div_zero
80+
81+
-- try_divide with non-zero divisor returns the quotient
82+
query
83+
SELECT try_divide(a, b) FROM test_decimal_div
84+
85+
-- try_divide literal decimal by zero
86+
query
87+
SELECT try_divide(cast(10.5 as decimal(10,2)), cast(0.0 as decimal(10,2)))
88+
89+
-- try_divide with NULL inputs
90+
query
91+
SELECT try_divide(NULL, cast(3.0 as decimal(10,2))),
92+
try_divide(cast(10.0 as decimal(10,2)), NULL)
93+
94+
-- ============================================================================
95+
-- Overflow in legacy mode produces NULL
96+
-- ============================================================================
97+
98+
statement
99+
CREATE TABLE test_decimal_overflow(a decimal(38,0)) USING parquet
100+
101+
statement
102+
INSERT INTO test_decimal_overflow VALUES
103+
(99999999999999999999999999999999999999),
104+
(-99999999999999999999999999999999999999)
105+
106+
-- Dividing max decimal(38,0) by a small fraction overflows the result type -> null
107+
query
108+
SELECT a / cast(0.01 as decimal(10,2)) FROM test_decimal_overflow
109+
110+
-- Integral divide overflow -> null
111+
query
112+
SELECT a div cast(0.40 as decimal(2,2)) FROM test_decimal_overflow
113+
114+
-- ============================================================================
115+
-- Literal arguments (constant folding is disabled by the test runner)
116+
-- ============================================================================
117+
118+
-- literal / literal
119+
query
120+
SELECT cast(10.5 as decimal(10,2)) / cast(3.5 as decimal(10,2))
121+
122+
-- literal div literal
123+
query
124+
SELECT cast(10 as decimal(10,0)) div cast(3 as decimal(10,0))
125+
126+
-- literal / zero
127+
query
128+
SELECT cast(7.0 as decimal(10,2)) / cast(0.0 as decimal(10,2))
129+
130+
-- literal div zero
131+
query
132+
SELECT cast(7 as decimal(10,0)) div cast(0 as decimal(10,0))
133+
134+
-- NULL literal
135+
query
136+
SELECT cast(NULL as decimal(10,2)) / cast(3.0 as decimal(10,2)),
137+
cast(5.0 as decimal(10,2)) / cast(NULL as decimal(10,2))
138+
139+
-- ============================================================================
140+
-- Mixed precision and scale
141+
-- ============================================================================
142+
143+
statement
144+
CREATE TABLE test_decimal_mixed(a decimal(18,6), b decimal(10,2)) USING parquet
145+
146+
statement
147+
INSERT INTO test_decimal_mixed VALUES
148+
(123456.789012, 99.99),
149+
(0.000001, 1.00),
150+
(-123456.789012, 0.01),
151+
(NULL, NULL)
152+
153+
-- spark_answer_only: mixed-precision division can trigger the precision-loss path in Spark's
154+
-- decimal arithmetic which Comet does not yet handle natively; see
155+
-- https://github.com/apache/datafusion-comet/issues/1526
156+
query spark_answer_only
157+
SELECT a / b FROM test_decimal_mixed
158+
159+
query
160+
SELECT a div b FROM test_decimal_mixed
161+
162+
-- ============================================================================
163+
-- Various precision/scale combinations
164+
-- ============================================================================
165+
166+
statement
167+
CREATE TABLE test_decimal_prec(
168+
a5_2 decimal(5,2),
169+
b5_2 decimal(5,2),
170+
a38_4 decimal(38,4),
171+
b38_4 decimal(38,4)
172+
) USING parquet
173+
174+
statement
175+
INSERT INTO test_decimal_prec VALUES
176+
(10.50, 3.25, 9999999999999999.1234, 3.0001),
177+
(-10.50, 3.25, -9999999999999999.1234, 3.0001),
178+
(0.00, 1.00, 0.0000, 1.0000),
179+
(NULL, NULL, NULL, NULL)
180+
181+
query
182+
SELECT a5_2 / b5_2 FROM test_decimal_prec
183+
184+
query
185+
SELECT a5_2 div b5_2 FROM test_decimal_prec
186+
187+
-- spark_answer_only: decimal(38,4) / decimal(38,4) produces a result type that exceeds
188+
-- Decimal(38,x) precision, triggering a precision-loss path Comet does not yet handle natively;
189+
-- see https://github.com/apache/datafusion-comet/issues/1526
190+
query spark_answer_only
191+
SELECT a38_4 / b38_4 FROM test_decimal_prec
192+
193+
query
194+
SELECT a38_4 div b38_4 FROM test_decimal_prec

0 commit comments

Comments
 (0)