Skip to content

Commit ce602ac

Browse files
andygroveandygrove
authored
feat: remove native cast from boolean to decimal (#5185)
* feat: remove native cast from boolean to decimal Boolean -> Decimal is an edge case that nobody uses in practice, and the native implementation has to reproduce Spark's precision/scale and overflow semantics for it. That is not worth the complexity, so mark the cast unsupported in `CometCast` and let the `CodegenDispatchFallback` mixin route it through Spark's own generated code inside the Comet pipeline. The projection still runs natively; only the cast itself is evaluated by Spark's codegen. Adds SQL file tests covering non-ANSI and ANSI behavior, including the value-dependent overflow edge cases. * docs: link issue #5186 from the ignored boolean-to-decimal cast test Explain in the test comment that the ignore is a limitation of CometCastSuite rather than of the cast itself, and point at the tracking issue for re-enabling it.
1 parent 4b60dca commit ce602ac

7 files changed

Lines changed: 192 additions & 64 deletions

File tree

‎native/spark-expr/benches/cast_from_boolean.rs‎

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -69,20 +69,7 @@ fn criterion_benchmark(c: &mut Criterion) {
6969
None,
7070
None,
7171
);
72-
let cast_to_str = Cast::new(
73-
expr.clone(),
74-
DataType::Utf8,
75-
spark_cast_options.clone(),
76-
None,
77-
None,
78-
);
79-
let cast_to_decimal = Cast::new(
80-
expr,
81-
DataType::Decimal128(10, 4),
82-
spark_cast_options,
83-
None,
84-
None,
85-
);
72+
let cast_to_str = Cast::new(expr, DataType::Utf8, spark_cast_options, None, None);
8673

8774
let mut group = c.benchmark_group("cast_bool".to_string());
8875
group.bench_function("i8", |b| {
@@ -106,9 +93,6 @@ fn criterion_benchmark(c: &mut Criterion) {
10693
group.bench_function("str", |b| {
10794
b.iter(|| cast_to_str.evaluate(&boolean_batch).unwrap());
10895
});
109-
group.bench_function("decimal", |b| {
110-
b.iter(|| cast_to_decimal.evaluate(&boolean_batch).unwrap());
111-
});
11296
}
11397

11498
fn create_boolean_batch() -> RecordBatch {

‎native/spark-expr/src/conversion_funcs/boolean.rs‎

Lines changed: 14 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,13 @@
1515
// specific language governing permissions and limitations
1616
// under the License.
1717

18-
use crate::{SparkError, SparkResult};
19-
use arrow::array::{Array, ArrayRef, AsArray, Decimal128Array, TimestampMicrosecondBuilder};
18+
use crate::SparkResult;
19+
use arrow::array::{Array, ArrayRef, AsArray, TimestampMicrosecondBuilder};
2020
use arrow::datatypes::DataType;
2121
use std::sync::Arc;
2222

23+
/// Boolean -> Decimal is intentionally absent: it has no native path and is routed through the
24+
/// JVM codegen dispatcher on the Scala side (see `CometCast.canCastFromBoolean`).
2325
pub fn is_df_cast_from_bool_spark_compatible(to_type: &DataType) -> bool {
2426
use DataType::*;
2527
matches!(
@@ -28,35 +30,6 @@ pub fn is_df_cast_from_bool_spark_compatible(to_type: &DataType) -> bool {
2830
)
2931
}
3032

31-
pub fn cast_boolean_to_decimal(
32-
array: &ArrayRef,
33-
precision: u8,
34-
scale: i8,
35-
) -> SparkResult<ArrayRef> {
36-
let bool_array = array.as_boolean();
37-
let scaled_val = 10_i128.pow(scale as u32);
38-
let result: Decimal128Array = bool_array
39-
.iter()
40-
.map(|v| v.map(|b| if b { scaled_val } else { 0 }))
41-
.collect();
42-
43-
// Convert Arrow decimal overflow errors to SparkError
44-
let decimal_array = result
45-
.with_precision_and_scale(precision, scale)
46-
.map_err(|e| {
47-
if matches!(e, arrow::error::ArrowError::InvalidArgumentError(_))
48-
&& e.to_string().contains("too large to store in a Decimal128")
49-
{
50-
// Use the scaled value as it's the only non-zero value that could overflow
51-
crate::error::decimal_overflow_error(scaled_val, precision, scale)
52-
} else {
53-
SparkError::Arrow(Arc::new(e))
54-
}
55-
})?;
56-
57-
Ok(Arc::new(decimal_array))
58-
}
59-
6033
pub(crate) fn cast_boolean_to_timestamp(
6134
array_ref: &ArrayRef,
6235
target_tz: &Option<Arc<str>>,
@@ -212,20 +185,20 @@ mod tests {
212185
}
213186

214187
#[test]
215-
fn test_bool_to_decimal_cast() {
216-
let result = cast_array(
188+
fn test_bool_to_decimal_cast_is_not_supported() {
189+
// Boolean -> Decimal has no native path; the Scala planner routes it through the JVM
190+
// codegen dispatcher, so reaching the native cast at all is a bug.
191+
let err = cast_array(
217192
test_input_bool_array(),
218193
&Decimal128(10, 4),
219194
&test_input_spark_opts(),
220195
)
221-
.unwrap();
222-
let expected_arr = Decimal128Array::from(vec![10000_i128, 0_i128])
223-
.with_precision_and_scale(10, 4)
224-
.unwrap();
225-
let arr = result.as_any().downcast_ref::<Decimal128Array>().unwrap();
226-
assert_eq!(arr.value(0), expected_arr.value(0));
227-
assert_eq!(arr.value(1), expected_arr.value(1));
228-
assert!(arr.is_null(2));
196+
.expect_err("expected boolean -> decimal to be unsupported");
197+
assert!(
198+
err.to_string()
199+
.contains("Native cast invoked for unsupported cast"),
200+
"unexpected error: {err}"
201+
);
229202
}
230203

231204
#[test]

‎native/spark-expr/src/conversion_funcs/cast.rs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
// under the License.
1717

1818
use crate::conversion_funcs::boolean::{
19-
cast_boolean_to_decimal, cast_boolean_to_timestamp, is_df_cast_from_bool_spark_compatible,
19+
cast_boolean_to_timestamp, is_df_cast_from_bool_spark_compatible,
2020
};
2121
use crate::conversion_funcs::numeric::{
2222
cast_decimal128_to_utf8, cast_decimal_to_timestamp, cast_float32_to_decimal128,
@@ -438,9 +438,6 @@ pub(crate) fn cast_array(
438438
(Int64, Binary) if (eval_mode == Legacy) => {
439439
cast_whole_num_to_binary!(&array, Int64Array, 8)
440440
}
441-
(Boolean, Decimal128(precision, scale)) => {
442-
cast_boolean_to_decimal(&array, *precision, *scale)
443-
}
444441
(Int8 | Int16 | Int32 | Int64, Timestamp(_, tz)) => cast_int_to_timestamp(&array, tz),
445442
(Float32 | Float64, Timestamp(_, tz)) => cast_float_to_timestamp(&array, tz, eval_mode),
446443
(Boolean, Timestamp(_, tz)) => cast_boolean_to_timestamp(&array, tz),

‎spark/src/main/scala/org/apache/comet/expressions/CometCast.scala‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,10 +355,14 @@ object CometCast
355355
private def canCastFromBoolean(toType: DataType, evalMode: CometEvalMode.Value): SupportLevel =
356356
toType match {
357357
case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType | DataTypes.LongType |
358-
DataTypes.FloatType | DataTypes.DoubleType | _: DecimalType =>
358+
DataTypes.FloatType | DataTypes.DoubleType =>
359359
Compatible()
360360
case _: TimestampType if evalMode == CometEvalMode.LEGACY =>
361361
Compatible()
362+
// Boolean -> Decimal has no native path. It is a rare cast and getting the
363+
// precision/scale/overflow behavior right in native code is not worth the complexity, so
364+
// the `CodegenDispatchFallback` mixin routes it through Spark's own generated code inside
365+
// the Comet pipeline instead.
362366
case _ => unsupported(DataTypes.BooleanType, toType)
363367
}
364368

‎spark/src/test/resources/sql-tests/expressions/cast/cast_boolean_to_decimal.sql‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
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+
-- Boolean -> Decimal has no native path in Comet. `CometCast` reports it as unsupported so the
19+
-- `CodegenDispatchFallback` mixin routes it through Spark's own generated code inside the Comet
20+
-- pipeline. The default `query` mode therefore still asserts fully native execution.
21+
22+
statement
23+
CREATE TABLE test_cast_bool_to_decimal(id int, b boolean) USING parquet
24+
25+
statement
26+
INSERT INTO test_cast_bool_to_decimal VALUES (1, true), (2, false), (3, NULL)
27+
28+
-- basic precision/scale combinations, including the long fast path (precision <= 18) and the
29+
-- BigDecimal path (precision > 18)
30+
query
31+
SELECT id, cast(b as decimal(10,2)), cast(b as decimal(14,4)), cast(b as decimal(30,0))
32+
FROM test_cast_bool_to_decimal ORDER BY id
33+
34+
-- smallest precision/scale that can represent 1
35+
query
36+
SELECT id, cast(b as decimal(1,0)), cast(b as decimal(2,1))
37+
FROM test_cast_bool_to_decimal ORDER BY id
38+
39+
-- maximum precision, with and without scale
40+
query
41+
SELECT id, cast(b as decimal(38,0)), cast(b as decimal(38,37))
42+
FROM test_cast_bool_to_decimal ORDER BY id
43+
44+
-- overflow: decimal(1,1) holds at most 0.9, so true does not fit and returns NULL in non-ANSI
45+
-- mode while false and NULL are unaffected
46+
query
47+
SELECT id, cast(b as decimal(1,1)), cast(b as decimal(2,2)), cast(b as decimal(38,38))
48+
FROM test_cast_bool_to_decimal ORDER BY id
49+
50+
-- literal arguments
51+
query
52+
SELECT cast(true as decimal(10,2)), cast(false as decimal(10,2)),
53+
cast(cast(NULL as boolean) as decimal(10,2))
54+
55+
-- literal overflow
56+
query
57+
SELECT cast(true as decimal(1,1)), cast(false as decimal(1,1))
58+
59+
-- try_cast returns NULL on overflow rather than throwing
60+
query
61+
SELECT id, try_cast(b as decimal(10,2)), try_cast(b as decimal(1,1))
62+
FROM test_cast_bool_to_decimal ORDER BY id
63+
64+
-- comparison predicates on the result of the cast
65+
query
66+
SELECT id FROM test_cast_bool_to_decimal
67+
WHERE cast(b as decimal(10,2)) > 0.5 ORDER BY id
68+
69+
-- the cast feeding an aggregate
70+
query
71+
SELECT sum(cast(b as decimal(10,2))), count(cast(b as decimal(1,1)))
72+
FROM test_cast_bool_to_decimal

‎spark/src/test/resources/sql-tests/expressions/cast/cast_boolean_to_decimal_ansi.sql‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
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+
-- ANSI edge cases for Boolean -> Decimal. Comet has no native path for this cast; the
19+
-- `CodegenDispatchFallback` mixin runs Spark's own generated code inside the Comet pipeline, so
20+
-- the ANSI overflow errors must match Spark exactly. The non-error queries act as sentinels
21+
-- proving the cast really executed natively rather than falling the whole plan back to Spark.
22+
23+
-- Config: spark.sql.ansi.enabled=true
24+
25+
statement
26+
CREATE TABLE test_cast_bool_to_decimal_ansi(id int, b boolean) USING parquet
27+
28+
statement
29+
INSERT INTO test_cast_bool_to_decimal_ansi VALUES (1, true), (2, false), (3, NULL)
30+
31+
-- sentinel: a cast that always fits must run natively under ANSI mode
32+
query
33+
SELECT id, cast(b as decimal(10,2)), cast(b as decimal(38,0))
34+
FROM test_cast_bool_to_decimal_ansi ORDER BY id
35+
36+
-- sentinel: decimal(1,0) is the tightest type that can hold 1
37+
query
38+
SELECT id, cast(b as decimal(1,0)), cast(b as decimal(2,1))
39+
FROM test_cast_bool_to_decimal_ansi ORDER BY id
40+
41+
-- decimal(1,1) holds at most 0.9, so casting true overflows and must throw under ANSI mode
42+
query expect_error(NUMERIC_VALUE_OUT_OF_RANGE)
43+
SELECT cast(b as decimal(1,1)) FROM test_cast_bool_to_decimal_ansi WHERE id = 1
44+
45+
-- decimal(2,2) holds at most 0.99: same overflow, larger precision
46+
query expect_error(NUMERIC_VALUE_OUT_OF_RANGE)
47+
SELECT cast(b as decimal(2,2)) FROM test_cast_bool_to_decimal_ansi WHERE id = 1
48+
49+
-- all-scale decimal at maximum precision still cannot represent 1
50+
query expect_error(NUMERIC_VALUE_OUT_OF_RANGE)
51+
SELECT cast(b as decimal(38,38)) FROM test_cast_bool_to_decimal_ansi WHERE id = 1
52+
53+
-- literal true overflows the same way
54+
query expect_error(NUMERIC_VALUE_OUT_OF_RANGE)
55+
SELECT cast(true as decimal(1,1))
56+
57+
-- overflow is value dependent: false scales to 0, which fits every decimal type, so rows that
58+
-- only contain false must not throw
59+
query
60+
SELECT cast(b as decimal(1,1)), cast(b as decimal(38,38))
61+
FROM test_cast_bool_to_decimal_ansi WHERE id = 2
62+
63+
-- NULL input short-circuits before the precision check, so it must not throw either
64+
query
65+
SELECT cast(b as decimal(1,1)), cast(b as decimal(38,38))
66+
FROM test_cast_bool_to_decimal_ansi WHERE id = 3
67+
68+
-- try_cast suppresses the ANSI overflow and yields NULL
69+
query
70+
SELECT id, try_cast(b as decimal(1,1)), try_cast(b as decimal(38,38))
71+
FROM test_cast_bool_to_decimal_ansi ORDER BY id
72+
73+
-- overflow raised from inside an aggregate input
74+
query expect_error(NUMERIC_VALUE_OUT_OF_RANGE)
75+
SELECT sum(cast(b as decimal(1,1))) FROM test_cast_bool_to_decimal_ansi
76+
77+
-- overflow raised from inside a filter predicate
78+
query expect_error(NUMERIC_VALUE_OUT_OF_RANGE)
79+
SELECT id FROM test_cast_bool_to_decimal_ansi WHERE cast(b as decimal(1,1)) > 0.5

‎spark/src/test/scala/org/apache/comet/CometNativeCastSuite.scala‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,10 +198,29 @@ class CometNativeCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
198198
castTest(generateBools(), DataTypes.DoubleType)
199199
}
200200

201-
test("cast BooleanType to DecimalType(10,2)") {
201+
// Boolean -> Decimal has no native path and is routed through the JVM codegen dispatcher, so
202+
// `CometCast.isSupported` reports it as unsupported and the matrix-consistency test above
203+
// requires this one to be ignored. The test would actually pass if un-ignored; the suite
204+
// predates codegen dispatch and cannot yet express "runs in Comet without a native kernel".
205+
// https://github.com/apache/datafusion-comet/issues/5186 tracks teaching the suite that
206+
// distinction, at which point this test can be re-enabled. Execution coverage in the meantime,
207+
// including the ANSI overflow edge cases, lives in
208+
// `sql-tests/expressions/cast/cast_boolean_to_decimal{,_ansi}.sql`.
209+
ignore("cast BooleanType to DecimalType(10,2)") {
202210
castTest(generateBools(), DataTypes.createDecimalType(10, 2))
203211
}
204212

213+
test("cast BooleanType to DecimalType is not supported natively") {
214+
Seq(
215+
DataTypes.createDecimalType(10, 2),
216+
DataTypes.createDecimalType(14, 4),
217+
DataTypes.createDecimalType(30, 0)).foreach { toType =>
218+
assert(
219+
CometCast.isSupported(BooleanType, toType, None, CometEvalMode.LEGACY) ==
220+
Unsupported(Some(s"Cast from $BooleanType to $toType is not supported")))
221+
}
222+
}
223+
205224
test("cast BooleanType to DecimalType(14,4)") {
206225
castTest(generateBools(), DataTypes.createDecimalType(14, 4))
207226
}

0 commit comments

Comments
 (0)