Skip to content

Commit 87d049b

Browse files
kazuyukitanimurakazuyukitanimura
authored
test: ceil and floor works correctly for Decimal128 (#3848)
## Which issue does this PR close? Closes #1729 ## Rationale for this change #1729 was opened due to PR #1728 (comment #1728 (comment)) Later #3675 disabled `ceil` and `floor` for decimals. However, the rust test cases added in #1728 are incorrect in my opinion. When spark `ceil`/`floor`, Spark changes precision and scale. And later result precision and scale are set at https://github.com/apache/datafusion-comet/blob/main/native/spark-expr/src/math_funcs/ceil.rs#L51 The output of `decimal_ceil_f` is intermediate result. Therefore. `decimal_ceil_f` is correct and the expected values of rust test cases are incorrect. ## What changes are included in this PR? Corrected the rust test cases. Enabled `ceil` and `floor` again ## How are these changes tested?
1 parent c3f59a6 commit 87d049b

8 files changed

Lines changed: 72 additions & 107 deletions

File tree

‎docs/source/user-guide/latest/compatibility.md‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,6 @@ the [Comet Supported Expressions Guide](expressions.md) for more information on
8282

8383
### Math Expressions
8484

85-
- **Ceil, Floor**: Incorrect results for Decimal type inputs.
86-
[#1729](https://github.com/apache/datafusion-comet/issues/1729)
8785
- **Tan**: `tan(-0.0)` produces `0.0` instead of `-0.0`.
8886
[#1897](https://github.com/apache/datafusion-comet/issues/1897)
8987

‎docs/source/user-guide/latest/expressions.md‎

Lines changed: 42 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -121,48 +121,48 @@ Expressions that are not Spark-compatible will fall back to Spark by default and
121121

122122
## Math Expressions
123123

124-
| Expression | SQL | Spark-Compatible? | Compatibility Notes |
125-
| -------------- | --------- | ----------------- | ----------------------------------------------------------------------------------------------------------- |
126-
| Abs | `abs` | Yes | |
127-
| Acos | `acos` | Yes | |
128-
| Add | `+` | Yes | |
129-
| Asin | `asin` | Yes | |
130-
| Atan | `atan` | Yes | |
131-
| Atan2 | `atan2` | Yes | |
132-
| BRound | `bround` | Yes | |
133-
| Ceil | `ceil` | No | Incorrect results for Decimal type inputs ([#1729](https://github.com/apache/datafusion-comet/issues/1729)) |
134-
| Cos | `cos` | Yes | |
135-
| Cosh | `cosh` | Yes | |
136-
| Cot | `cot` | Yes | |
137-
| Divide | `/` | Yes | |
138-
| Exp | `exp` | Yes | |
139-
| Expm1 | `expm1` | Yes | |
140-
| Floor | `floor` | No | Incorrect results for Decimal type inputs ([#1729](https://github.com/apache/datafusion-comet/issues/1729)) |
141-
| Hex | `hex` | Yes | |
142-
| IntegralDivide | `div` | Yes | |
143-
| IsNaN | `isnan` | Yes | |
144-
| Log | `log` | Yes | |
145-
| Log2 | `log2` | Yes | |
146-
| Log10 | `log10` | Yes | |
147-
| Multiply | `*` | Yes | |
148-
| Pow | `power` | Yes | |
149-
| Rand | `rand` | Yes | |
150-
| Randn | `randn` | Yes | |
151-
| Remainder | `%` | Yes | |
152-
| Round | `round` | Yes | |
153-
| Signum | `signum` | Yes | |
154-
| Sin | `sin` | Yes | |
155-
| Sinh | `sinh` | Yes | |
156-
| Sqrt | `sqrt` | Yes | |
157-
| Subtract | `-` | Yes | |
158-
| Tan | `tan` | No | tan(-0.0) produces incorrect result ([#1897](https://github.com/apache/datafusion-comet/issues/1897)) |
159-
| Tanh | `tanh` | Yes | |
160-
| TryAdd | `try_add` | Yes | Only integer inputs are supported |
161-
| TryDivide | `try_div` | Yes | Only integer inputs are supported |
162-
| TryMultiply | `try_mul` | Yes | Only integer inputs are supported |
163-
| TrySubtract | `try_sub` | Yes | Only integer inputs are supported |
164-
| UnaryMinus | `-` | Yes | |
165-
| Unhex | `unhex` | Yes | |
124+
| Expression | SQL | Spark-Compatible? | Compatibility Notes |
125+
| -------------- | --------- | ----------------- | ----------------------------------------------------------------------------------------------------- |
126+
| Abs | `abs` | Yes | |
127+
| Acos | `acos` | Yes | |
128+
| Add | `+` | Yes | |
129+
| Asin | `asin` | Yes | |
130+
| Atan | `atan` | Yes | |
131+
| Atan2 | `atan2` | Yes | |
132+
| BRound | `bround` | Yes | |
133+
| Ceil | `ceil` | Yes | |
134+
| Cos | `cos` | Yes | |
135+
| Cosh | `cosh` | Yes | |
136+
| Cot | `cot` | Yes | |
137+
| Divide | `/` | Yes | |
138+
| Exp | `exp` | Yes | |
139+
| Expm1 | `expm1` | Yes | |
140+
| Floor | `floor` | Yes | |
141+
| Hex | `hex` | Yes | |
142+
| IntegralDivide | `div` | Yes | |
143+
| IsNaN | `isnan` | Yes | |
144+
| Log | `log` | Yes | |
145+
| Log2 | `log2` | Yes | |
146+
| Log10 | `log10` | Yes | |
147+
| Multiply | `*` | Yes | |
148+
| Pow | `power` | Yes | |
149+
| Rand | `rand` | Yes | |
150+
| Randn | `randn` | Yes | |
151+
| Remainder | `%` | Yes | |
152+
| Round | `round` | Yes | |
153+
| Signum | `signum` | Yes | |
154+
| Sin | `sin` | Yes | |
155+
| Sinh | `sinh` | Yes | |
156+
| Sqrt | `sqrt` | Yes | |
157+
| Subtract | `-` | Yes | |
158+
| Tan | `tan` | No | tan(-0.0) produces incorrect result ([#1897](https://github.com/apache/datafusion-comet/issues/1897)) |
159+
| Tanh | `tanh` | Yes | |
160+
| TryAdd | `try_add` | Yes | Only integer inputs are supported |
161+
| TryDivide | `try_div` | Yes | Only integer inputs are supported |
162+
| TryMultiply | `try_mul` | Yes | Only integer inputs are supported |
163+
| TrySubtract | `try_sub` | Yes | Only integer inputs are supported |
164+
| UnaryMinus | `-` | Yes | |
165+
| Unhex | `unhex` | Yes | |
166166

167167
## Hashing Functions
168168

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

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -162,9 +162,7 @@ mod test {
162162
Ok(())
163163
}
164164

165-
// https://github.com/apache/datafusion-comet/issues/1729
166165
#[test]
167-
#[ignore]
168166
fn test_ceil_decimal128_array() -> Result<()> {
169167
let array = Decimal128Array::from(vec![
170168
Some(12345), // 123.45
@@ -174,16 +172,16 @@ mod test {
174172
])
175173
.with_precision_and_scale(5, 2)?;
176174
let args = vec![ColumnarValue::Array(Arc::new(array))];
177-
let ColumnarValue::Array(result) = spark_ceil(&args, &DataType::Decimal128(5, 2))? else {
175+
let ColumnarValue::Array(result) = spark_ceil(&args, &DataType::Decimal128(4, 0))? else {
178176
unreachable!()
179177
};
180178
let expected = Decimal128Array::from(vec![
181-
Some(12400), // 124.00
182-
Some(12500), // 125.00
183-
Some(-12900), // -129.00
179+
Some(124), // 124.00
180+
Some(125), // 125.00
181+
Some(-129), // -129.00
184182
None,
185183
])
186-
.with_precision_and_scale(5, 2)?;
184+
.with_precision_and_scale(4, 0)?;
187185
let actual = result.as_any().downcast_ref::<Decimal128Array>().unwrap();
188186
assert_eq!(actual, &expected);
189187
Ok(())
@@ -225,21 +223,19 @@ mod test {
225223
Ok(())
226224
}
227225

228-
// https://github.com/apache/datafusion-comet/issues/1729
229226
#[test]
230-
#[ignore]
231227
fn test_ceil_decimal128_scalar() -> Result<()> {
232228
let args = vec![ColumnarValue::Scalar(ScalarValue::Decimal128(
233229
Some(567),
234230
3,
235231
1,
236232
))]; // 56.7
237-
let ColumnarValue::Scalar(ScalarValue::Decimal128(Some(result), 3, 1)) =
238-
spark_ceil(&args, &DataType::Decimal128(3, 1))?
233+
let ColumnarValue::Scalar(ScalarValue::Decimal128(Some(result), 3, 0)) =
234+
spark_ceil(&args, &DataType::Decimal128(3, 0))?
239235
else {
240236
unreachable!()
241237
};
242-
assert_eq!(result, 570); // 57.0
238+
assert_eq!(result, 57); // 57.0
243239
Ok(())
244240
}
245241
}

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

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -162,9 +162,7 @@ mod test {
162162
Ok(())
163163
}
164164

165-
// https://github.com/apache/datafusion-comet/issues/1729
166165
#[test]
167-
#[ignore]
168166
fn test_floor_decimal128_array() -> Result<()> {
169167
let array = Decimal128Array::from(vec![
170168
Some(12345), // 123.45
@@ -174,16 +172,16 @@ mod test {
174172
])
175173
.with_precision_and_scale(5, 2)?;
176174
let args = vec![ColumnarValue::Array(Arc::new(array))];
177-
let ColumnarValue::Array(result) = spark_floor(&args, &DataType::Decimal128(5, 2))? else {
175+
let ColumnarValue::Array(result) = spark_floor(&args, &DataType::Decimal128(4, 0))? else {
178176
unreachable!()
179177
};
180178
let expected = Decimal128Array::from(vec![
181-
Some(12300), // 123.00
182-
Some(12500), // 125.00
183-
Some(-13000), // -130.00
179+
Some(123), // 123.00
180+
Some(125), // 125.00
181+
Some(-130), // -130.00
184182
None,
185183
])
186-
.with_precision_and_scale(5, 2)?;
184+
.with_precision_and_scale(4, 0)?;
187185
let actual = result.as_any().downcast_ref::<Decimal128Array>().unwrap();
188186
assert_eq!(actual, &expected);
189187
Ok(())
@@ -225,21 +223,19 @@ mod test {
225223
Ok(())
226224
}
227225

228-
// https://github.com/apache/datafusion-comet/issues/1729
229226
#[test]
230-
#[ignore]
231227
fn test_floor_decimal128_scalar() -> Result<()> {
232228
let args = vec![ColumnarValue::Scalar(ScalarValue::Decimal128(
233229
Some(567),
234230
3,
235231
1,
236232
))]; // 56.7
237-
let ColumnarValue::Scalar(ScalarValue::Decimal128(Some(result), 3, 1)) =
238-
spark_floor(&args, &DataType::Decimal128(3, 1))?
233+
let ColumnarValue::Scalar(ScalarValue::Decimal128(Some(result), 3, 0)) =
234+
spark_floor(&args, &DataType::Decimal128(3, 0))?
239235
else {
240236
unreachable!()
241237
};
242-
assert_eq!(result, 560); // 56.0
238+
assert_eq!(result, 56); // 56.0
243239
Ok(())
244240
}
245241
}

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

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -38,18 +38,6 @@ object CometAtan2 extends CometExpressionSerde[Atan2] {
3838
}
3939

4040
object CometCeil extends CometExpressionSerde[Ceil] {
41-
42-
override def getSupportLevel(expr: Ceil): SupportLevel = {
43-
expr.child.dataType match {
44-
case _: DecimalType =>
45-
Incompatible(
46-
Some(
47-
"Incorrect results for Decimal type inputs" +
48-
" (https://github.com/apache/datafusion-comet/issues/1729)"))
49-
case _ => Compatible()
50-
}
51-
}
52-
5341
override def convert(
5442
expr: Ceil,
5543
inputs: Seq[Attribute],
@@ -70,18 +58,6 @@ object CometCeil extends CometExpressionSerde[Ceil] {
7058
}
7159

7260
object CometFloor extends CometExpressionSerde[Floor] {
73-
74-
override def getSupportLevel(expr: Floor): SupportLevel = {
75-
expr.child.dataType match {
76-
case _: DecimalType =>
77-
Incompatible(
78-
Some(
79-
"Incorrect results for Decimal type inputs" +
80-
" (https://github.com/apache/datafusion-comet/issues/1729)"))
81-
case _ => Compatible()
82-
}
83-
}
84-
8561
override def convert(
8662
expr: Floor,
8763
inputs: Seq[Attribute],

‎spark/src/test/resources/sql-tests/expressions/math/ceil.sql‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,17 @@
1515
-- specific language governing permissions and limitations
1616
-- under the License.
1717

18-
-- Config: spark.comet.expression.Ceil.allowIncompatible=true
1918
-- ConfigMatrix: parquet.enable.dictionary=false,true
2019

2120
statement
22-
CREATE TABLE test_ceil(f float, d double) USING parquet
21+
CREATE TABLE test_ceil(f float, d double, dec DECIMAL(5, 2)) USING parquet
2322

2423
statement
25-
INSERT INTO test_ceil VALUES (1.1, 1.1), (-1.1, -1.1), (0.0, 0.0), (1.0, 1.0), (NULL, NULL), (cast('NaN' as float), cast('NaN' as double)), (cast('Infinity' as float), cast('Infinity' as double))
24+
INSERT INTO test_ceil VALUES (1.1, 1.1, 1.10), (-1.1, -1.1, -1.10), (0.0, 0.0, 0.00), (1.0, 1.0, 1.00), (NULL, NULL, NULL), (cast('NaN' as float), cast('NaN' as double), NULL), (cast('Infinity' as float), cast('Infinity' as double), NULL)
2625

2726
query
28-
SELECT ceil(f), ceil(d) FROM test_ceil
27+
SELECT ceil(f), ceil(d), ceil(dec) FROM test_ceil
2928

3029
-- literal arguments
3130
query
32-
SELECT ceil(1.1), ceil(-1.1), ceil(0.0), ceil(NULL)
31+
SELECT ceil(1.1), ceil(-1.1), ceil(0.0), ceil(NULL), ceil(cast(1.10 as DECIMAL(5, 2))), ceil(cast(-1.10 as DECIMAL(5, 2)))

0 commit comments

Comments
 (0)