Skip to content

Commit 9f20f05

Browse files
coderfendercoderfender
committed
address_review_comments
1 parent b64eb6d commit 9f20f05

File tree

6 files changed

+27
-34
lines changed

6 files changed

+27
-34
lines changed

‎dev/diffs/3.4.3.diff‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,13 +194,14 @@ index 41fd4de2a09..44cd244d3b0 100644
194194
--CONFIG_DIM1 spark.sql.codegen.wholeStage=true
195195
--CONFIG_DIM1 spark.sql.codegen.wholeStage=false,spark.sql.codegen.factoryMode=CODEGEN_ONLY
196196
diff --git a/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int4.sql b/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int4.sql
197-
index 3a409eea348..b0a1fa5c20a 100644
197+
index 3a409eea348..38fed024c98 100644
198198
--- a/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int4.sql
199199
+++ b/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int4.sql
200-
@@ -69,6 +69,7 @@ SELECT '' AS one, i.* FROM INT4_TBL i WHERE (i.f1 % smallint('2')) = smallint('1
200+
@@ -69,6 +69,8 @@ SELECT '' AS one, i.* FROM INT4_TBL i WHERE (i.f1 % smallint('2')) = smallint('1
201201
-- any evens
202202
SELECT '' AS three, i.* FROM INT4_TBL i WHERE (i.f1 % int('2')) = smallint('0');
203203

204+
+-- https://github.com/apache/datafusion-comet/issues/2215
204205
+--SET spark.comet.exec.enabled=false
205206
-- [SPARK-28024] Incorrect value when out of range
206207
SELECT '' AS five, i.f1, i.f1 * smallint('2') AS x FROM INT4_TBL i;

‎dev/diffs/3.5.6.diff‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,13 +173,14 @@ index 41fd4de2a09..44cd244d3b0 100644
173173
--CONFIG_DIM1 spark.sql.codegen.wholeStage=true
174174
--CONFIG_DIM1 spark.sql.codegen.wholeStage=false,spark.sql.codegen.factoryMode=CODEGEN_ONLY
175175
diff --git a/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int4.sql b/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int4.sql
176-
index 3a409eea348..b0a1fa5c20a 100644
176+
index 3a409eea348..38fed024c98 100644
177177
--- a/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int4.sql
178178
+++ b/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int4.sql
179-
@@ -69,6 +69,7 @@ SELECT '' AS one, i.* FROM INT4_TBL i WHERE (i.f1 % smallint('2')) = smallint('1
179+
@@ -69,6 +69,8 @@ SELECT '' AS one, i.* FROM INT4_TBL i WHERE (i.f1 % smallint('2')) = smallint('1
180180
-- any evens
181181
SELECT '' AS three, i.* FROM INT4_TBL i WHERE (i.f1 % int('2')) = smallint('0');
182182

183+
+-- https://github.com/apache/datafusion-comet/issues/2215
183184
+--SET spark.comet.exec.enabled=false
184185
-- [SPARK-28024] Incorrect value when out of range
185186
SELECT '' AS five, i.f1, i.f1 * smallint('2') AS x FROM INT4_TBL i;

‎native/core/src/execution/planner.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,8 @@ impl PhysicalPlanner {
283283
)
284284
}
285285
ExprStruct::IntegralDivide(expr) => {
286+
// TODO respect eval mode
287+
// https://github.com/apache/datafusion-comet/issues/533
286288
let eval_mode = from_protobuf_eval_mode(expr.eval_mode)?;
287289
self.create_binary_expr_with_options(
288290
expr.left.as_ref().unwrap(),
@@ -1084,14 +1086,14 @@ impl PhysicalPlanner {
10841086
}
10851087
_ => {
10861088
let data_type = return_type.map(to_arrow_datatype).unwrap();
1087-
if [EvalMode::Try, EvalMode::Ansi].contains(&eval_mode)
1088-
&& (data_type.is_floating() || data_type.is_integer())
1089+
if [EvalMode::Try, EvalMode::Ansi].contains(&eval_mode) && (data_type.is_numeric())
10891090
{
10901091
let op_str = match op {
10911092
DataFusionOperator::Plus => "checked_add",
10921093
DataFusionOperator::Minus => "checked_sub",
10931094
DataFusionOperator::Multiply => "checked_mul",
10941095
DataFusionOperator::Divide => "checked_div",
1096+
DataFusionOperator::IntegerDivide => "checked_div",
10951097
_ => {
10961098
todo!("Operator yet to be implemented!");
10971099
}

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

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ where
4949
Err(_e) => {
5050
if is_ansi_mode {
5151
return Err(SparkError::ArithmeticOverflow {
52-
from_type: String::from("Integer/Float"),
52+
from_type: String::from("integer"),
5353
}
5454
.into());
5555
} else {
@@ -70,7 +70,7 @@ where
7070
Err(_e) => {
7171
if is_ansi_mode {
7272
return Err(SparkError::ArithmeticOverflow {
73-
from_type: String::from("Integer/Float"),
73+
from_type: String::from("integer"),
7474
}
7575
.into());
7676
} else {
@@ -91,7 +91,7 @@ where
9191
Err(_e) => {
9292
if is_ansi_mode {
9393
return Err(SparkError::ArithmeticOverflow {
94-
from_type: String::from("Integer/Float"),
94+
from_type: String::from("integer"),
9595
}
9696
.into());
9797
} else {
@@ -115,7 +115,7 @@ where
115115
Err(divide_by_zero_error().into())
116116
} else {
117117
return Err(SparkError::ArithmeticOverflow {
118-
from_type: String::from("Integer/Float"),
118+
from_type: String::from("integer"),
119119
}
120120
.into());
121121
};
@@ -227,19 +227,20 @@ fn checked_arithmetic_internal(
227227
op,
228228
is_ansi_mode,
229229
),
230-
DataType::Float16 => try_arithmetic_kernel::<Float16Type>(
230+
// Spark always casts division operands to floats
231+
DataType::Float16 if (op == "checked_div") => try_arithmetic_kernel::<Float16Type>(
231232
left_arr.as_primitive::<Float16Type>(),
232233
right_arr.as_primitive::<Float16Type>(),
233234
op,
234235
is_ansi_mode,
235236
),
236-
DataType::Float32 => try_arithmetic_kernel::<Float32Type>(
237+
DataType::Float32 if (op == "checked_div") => try_arithmetic_kernel::<Float32Type>(
237238
left_arr.as_primitive::<Float32Type>(),
238239
right_arr.as_primitive::<Float32Type>(),
239240
op,
240241
is_ansi_mode,
241242
),
242-
DataType::Float64 => try_arithmetic_kernel::<Float64Type>(
243+
DataType::Float64 if (op == "checked_div") => try_arithmetic_kernel::<Float64Type>(
243244
left_arr.as_primitive::<Float64Type>(),
244245
right_arr.as_primitive::<Float64Type>(),
245246
op,

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

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,6 @@ trait MathBase {
8787

8888
object CometAdd extends CometExpressionSerde[Add] with MathBase {
8989

90-
override def getSupportLevel(expr: Add): SupportLevel = {
91-
Compatible(None)
92-
}
93-
9490
override def convert(
9591
expr: Add,
9692
inputs: Seq[Attribute],
@@ -113,10 +109,6 @@ object CometAdd extends CometExpressionSerde[Add] with MathBase {
113109

114110
object CometSubtract extends CometExpressionSerde[Subtract] with MathBase {
115111

116-
override def getSupportLevel(expr: Subtract): SupportLevel = {
117-
Compatible(None)
118-
}
119-
120112
override def convert(
121113
expr: Subtract,
122114
inputs: Seq[Attribute],
@@ -139,10 +131,6 @@ object CometSubtract extends CometExpressionSerde[Subtract] with MathBase {
139131

140132
object CometMultiply extends CometExpressionSerde[Multiply] with MathBase {
141133

142-
override def getSupportLevel(expr: Multiply): SupportLevel = {
143-
Compatible(None)
144-
}
145-
146134
override def convert(
147135
expr: Multiply,
148136
inputs: Seq[Attribute],
@@ -165,10 +153,6 @@ object CometMultiply extends CometExpressionSerde[Multiply] with MathBase {
165153

166154
object CometDivide extends CometExpressionSerde[Divide] with MathBase {
167155

168-
override def getSupportLevel(expr: Divide): SupportLevel = {
169-
Compatible(None)
170-
}
171-
172156
override def convert(
173157
expr: Divide,
174158
inputs: Seq[Attribute],
@@ -197,7 +181,11 @@ object CometDivide extends CometExpressionSerde[Divide] with MathBase {
197181
object CometIntegralDivide extends CometExpressionSerde[IntegralDivide] with MathBase {
198182

199183
override def getSupportLevel(expr: IntegralDivide): SupportLevel = {
200-
Compatible(None)
184+
if (expr.evalMode == EvalMode.ANSI) {
185+
Incompatible(Some("ANSI mode is not supported"))
186+
} else {
187+
Compatible(None)
188+
}
201189
}
202190

203191
override def convert(
@@ -218,7 +206,7 @@ object CometIntegralDivide extends CometExpressionSerde[IntegralDivide] with Mat
218206
if (expr.right.dataType.isInstanceOf[DecimalType]) expr.right
219207
else Cast(expr.right, DecimalType(19, 0))
220208

221-
val rightExpr = nullIfWhenPrimitive(right)
209+
val rightExpr = nullIfWhenPrimitive(expr.right)
222210

223211
val dataType = (left.dataType, right.dataType) match {
224212
case (l: DecimalType, r: DecimalType) =>

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
4747
import testImplicits._
4848

4949
val ARITHMETIC_OVERFLOW_EXCEPTION_MSG =
50-
"""org.apache.comet.CometNativeException: [ARITHMETIC_OVERFLOW] Integer/Float overflow. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error."""
50+
"""org.apache.comet.CometNativeException: [ARITHMETIC_OVERFLOW] integer overflow. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error."""
5151

5252
override protected def test(testName: String, testTags: Tag*)(testFun: => Any)(implicit
5353
pos: Position): Unit = {
@@ -401,7 +401,6 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
401401
val data = Seq((Integer.MAX_VALUE, 1), (Integer.MIN_VALUE, -1))
402402
withSQLConf(SQLConf.ANSI_ENABLED.key -> "true") {
403403
withParquetTable(data, "tbl") {
404-
spark.table("tbl").printSchema()
405404
val res = spark.sql("""
406405
|SELECT
407406
| _1 + _2
@@ -457,7 +456,8 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
457456
}
458457
}
459458

460-
test("ANSI support for divide") {
459+
test("ANSI support for divide (division by zero)") {
460+
// TODO : Support ANSI mode in Integral divide
461461
val data = Seq((Integer.MIN_VALUE, 0))
462462
withSQLConf(SQLConf.ANSI_ENABLED.key -> "true") {
463463
withParquetTable(data, "tbl") {

0 commit comments

Comments
 (0)