You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "vaibhawvipul (via GitHub)" <gi...@apache.org> on 2024/05/25 13:36:16 UTC
[PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
vaibhawvipul opened a new pull request, #471:
URL: https://github.com/apache/datafusion-comet/pull/471
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #465 .
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1621480779
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,272 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) <= $min_val {
Review Comment:
When would the `value` be less than `min_val` ?
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,272 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) <= $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Float64 => {
+ check_overflow!(array, arrow::array::Float64Array, f64::MIN, "float")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => check_overflow!(
+ array,
+ arrow::array::IntervalMonthDayNanoArray,
+ i128::MIN,
+ "interval"
+ ),
Review Comment:
Seems Spark has `CalendarInterval`, `DayTimeInterval` and `YearMonthInterval`. Overflow check for `CalendarInterval` seems to be checking overflow for negation of the components of `CalendarInterval` (months, days, microseconds).
I don't think we need to worry about `MonthDayNano`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1616106260
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,408 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.eval_mode == EvalMode::Ansi {
+ match array.data_type() {
+ DataType::Int8 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int8Array>()
+ .unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i8::MIN || int_array.value(i) == i8::MAX {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "integer".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Int16 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int16Array>()
+ .unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i16::MIN || int_array.value(i) == i16::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "integer".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Int32 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int32Array>()
+ .unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i32::MIN || int_array.value(i) == i32::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "integer".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Int64 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int64Array>()
+ .unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i64::MIN || int_array.value(i) == i64::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "integer".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Float16 => {
+ let float_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Float16Array>()
+ .unwrap();
+ for i in 0..float_array.len() {
+ if float_array.value(i) <= half::f16::MIN
+ || float_array.value(i) >= half::f16::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "float".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Float32 => {
+ let float_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Float32Array>()
+ .unwrap();
+ for i in 0..float_array.len() {
+ if float_array.value(i) <= f32::MIN
+ || float_array.value(i) >= f32::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "float".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Float64 => {
+ let float_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Float64Array>()
+ .unwrap();
+ for i in 0..float_array.len() {
+ if float_array.value(i) <= f64::MIN
+ || float_array.value(i) >= f64::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "float".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Interval(value) => {
+ // Downcast the array to the appropriate interval type
+ match value {
+ arrow::datatypes::IntervalUnit::YearMonth => {
+ let interval_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::IntervalYearMonthArray>()
+ .unwrap();
+ for i in 0..interval_array.len() {
+ if interval_array.value(i) == i32::MIN
+ || interval_array.value(i) == i32::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "interval".to_string(),
+ }
Review Comment:
nit: consider adding a function to create the `CometError::ArithmeticOverflow` err to reduce boilerplate of creating the struct.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1620647408
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,54 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ def withAnsiMode(enabled: Boolean)(f: => Unit): Unit = {
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ANSI_MODE_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true")(f)
+ }
+
+ def checkOverflow(query: String): Unit = {
+ checkSparkMaybeThrows(sql(query)) match {
+ case (Some(sparkException), Some(cometException)) =>
+ assert(sparkException.getMessage.contains("integer overflow"))
+ assert(cometException.getMessage.contains("integer overflow"))
+ case (None, None) =>
+ fail("Exception should be thrown")
+ case (None, Some(_)) =>
+ fail("Comet threw an exception but Spark did not")
+ case (Some(_), None) =>
+ fail("Spark threw an exception but Comet did not")
+ }
+ }
+
+ def runArrayTest(query: String, path: String): Unit = {
+ withParquetTable(path, "t") {
+ withAnsiMode(enabled = false) {
+ checkSparkAnswerAndOperator(sql(query))
+ }
+ withAnsiMode(enabled = true) {
+ checkOverflow(query)
+ }
+ }
+ }
+
+ withTempDir { dir =>
+ // Array values test
+ val arrayPath = new Path(dir.toURI.toString, "array_test.parquet").toString
+ Seq(Int.MaxValue, Int.MinValue).toDF("a").write.mode("overwrite").parquet(arrayPath)
Review Comment:
This was a great suggestion. I have added some tests for all the types which raises overflow errors in spark.
https://github.com/apache/spark/blob/69afd4be9c93cb31a840b969ed1984c0b6b92f8e/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L63
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1622921141
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,54 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ def withAnsiMode(enabled: Boolean)(f: => Unit): Unit = {
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ANSI_MODE_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true")(f)
+ }
+
+ def checkOverflow(query: String): Unit = {
+ checkSparkMaybeThrows(sql(query)) match {
+ case (Some(sparkException), Some(cometException)) =>
+ assert(sparkException.getMessage.contains("integer overflow"))
+ assert(cometException.getMessage.contains("integer overflow"))
+ case (None, None) =>
+ fail("Exception should be thrown")
+ case (None, Some(_)) =>
+ fail("Comet threw an exception but Spark did not")
+ case (Some(_), None) =>
+ fail("Spark threw an exception but Comet did not")
+ }
+ }
+
+ def runArrayTest(query: String, path: String): Unit = {
+ withParquetTable(path, "t") {
+ withAnsiMode(enabled = false) {
+ checkSparkAnswerAndOperator(sql(query))
+ }
+ withAnsiMode(enabled = true) {
+ checkOverflow(query)
+ }
+ }
+ }
+
+ withTempDir { dir =>
+ // Array values test
+ val arrayPath = new Path(dir.toURI.toString, "array_test.parquet").toString
+ Seq(Int.MaxValue, Int.MinValue).toDF("a").write.mode("overwrite").parquet(arrayPath)
Review Comment:
Maybe add a case for interval as well?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1621346173
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,272 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) <= $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Float64 => {
+ check_overflow!(array, arrow::array::Float64Array, f64::MIN, "float")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => check_overflow!(
+ array,
+ arrow::array::IntervalMonthDayNanoArray,
+ i128::MIN,
+ "interval"
+ ),
Review Comment:
Hmmm I cannot recall how we are handling interval types... Maybe not used yet?
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,272 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) <= $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Float64 => {
+ check_overflow!(array, arrow::array::Float64Array, f64::MIN, "float")
+ }
Review Comment:
I think `Float64` also does not overflow by unary minus?
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,272 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) <= $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Float64 => {
+ check_overflow!(array, arrow::array::Float64Array, f64::MIN, "float")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => check_overflow!(
+ array,
+ arrow::array::IntervalMonthDayNanoArray,
+ i128::MIN,
+ "interval"
+ ),
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
+ Ok(ColumnarValue::Array(result))
+ }
+ ColumnarValue::Scalar(scalar) => {
+ if self.fail_on_error {
+ match scalar {
+ ScalarValue::Int8(value) => {
+ if value == Some(i8::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int16(value) => {
+ if value == Some(i16::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int32(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("integer").into());
+ }
+ }
+ ScalarValue::Int64(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("long").into());
+ }
+ }
+ ScalarValue::Float64(value) => {
+ if value == Some(f64::MIN) || value == Some(f64::MAX) {
+ return Err(arithmetic_overflow_error("float").into());
+ }
+ }
+ ScalarValue::IntervalDayTime(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ ScalarValue::IntervalYearMonth(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ ScalarValue::IntervalMonthDayNano(value) => {
+ if value == Some(i128::MIN) || value == Some(i128::MAX) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ Ok(ColumnarValue::Scalar((scalar.arithmetic_negate())?))
+ }
+ }
+ }
+
+ fn children(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ vec![self.arg.clone()]
+ }
+
+ fn with_new_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ Ok(Arc::new(NegativeExpr::new(
+ children[0].clone(),
+ self.fail_on_error,
+ )))
+ }
+
+ fn dyn_hash(&self, state: &mut dyn Hasher) {
+ let mut s = state;
+ self.hash(&mut s);
+ }
+
+ /// Given the child interval of a NegativeExpr, it calculates the NegativeExpr's interval.
+ /// It replaces the upper and lower bounds after multiplying them with -1.
+ /// Ex: `(a, b]` => `[-b, -a)`
+ fn evaluate_bounds(&self, children: &[&Interval]) -> Result<Interval> {
+ Interval::try_new(
+ children[0].upper().arithmetic_negate()?,
+ children[0].lower().arithmetic_negate()?,
Review Comment:
Not sure... but wondering if we need the overflow check here
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1616101515
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,408 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.eval_mode == EvalMode::Ansi {
+ match array.data_type() {
+ DataType::Int8 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int8Array>()
+ .unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i8::MIN || int_array.value(i) == i8::MAX {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "integer".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Int16 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int16Array>()
+ .unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i16::MIN || int_array.value(i) == i16::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "integer".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Int32 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int32Array>()
+ .unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i32::MIN || int_array.value(i) == i32::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "integer".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Int64 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int64Array>()
+ .unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i64::MIN || int_array.value(i) == i64::MAX
+ {
+ return Err(CometError::ArithmeticOverflow {
+ from_type: "integer".to_string(),
+ }
+ .into());
+ }
+ }
+ }
+ DataType::Float16 => {
Review Comment:
I don't think we would ever see a `Float16` because Java doesn't have that type?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1623082408
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,268 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => {
+ // Overflow checks are not supported
+ }
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
Review Comment:
no, i checked. Its behaviour is different from spark's. I think it is better to go ahead with neg_wrapping.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1618206450
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,36 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "int.parquet")
+ val df = Seq(Int.MaxValue, Int.MinValue).toDF("a")
+ df.write.mode("overwrite").parquet(path.toString)
+ spark.read.parquet(path.toString).createTempView("t")
+
+ // without ANSI mode
+ withSQLConf(
Review Comment:
@kazuyukitanimura can you please explain this a bit more?
Do you want me to create a new test to check non-overflow values?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2148086351
> Ops, looks like this is a merge conflict, I will fix it. Sorry for the inconvenience.
@kazuyukitanimura @vaibhawvipul This is already fixed in main. I fixed it as part of https://github.com/apache/datafusion-comet/pull/505
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1615266173
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,397 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::hash::{Hash, Hasher};
+use std::sync::Arc;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion_common::{Result, ScalarValue};
+use datafusion::logical_expr::interval_arithmetic::Interval;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion::physical_expr::PhysicalExpr;
+use datafusion_physical_expr::aggregate::utils::down_cast_any_ref;
+use datafusion_physical_expr::sort_properties::SortProperties;
+use crate::errors::CometError;
+use crate::execution::datafusion::expressions::cast::EvalMode;
+
+pub fn create_negate_expr(expr: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.eval_mode == EvalMode::Ansi {
+ match array.data_type() {
+ DataType::Int8 => {
+ let int_array = array.as_any().downcast_ref::<arrow::array::Int8Array>().unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) <= i8::MIN || int_array.value(i) >= i8::MAX {
+ return Err(CometError::ArithmeticOverflow{
+ from_type: "integer".to_string(),
+ }.into());
+ }
+ }
+ }
+ DataType::Int16 => {
+ let int_array = array.as_any().downcast_ref::<arrow::array::Int16Array>().unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) <= i16::MIN || int_array.value(i) >= i16::MAX {
+ return Err(CometError::ArithmeticOverflow{
+ from_type: "integer".to_string(),
+ }.into());
+ }
+ }
+ }
+ DataType::Int32 => {
+ let int_array = array.as_any().downcast_ref::<arrow::array::Int32Array>().unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) <= i32::MIN || int_array.value(i) >= i32::MAX {
+ return Err(CometError::ArithmeticOverflow{
+ from_type: "integer".to_string(),
+ }.into());
+ }
+ }
+ }
+ DataType::Int64 => {
+ let int_array = array.as_any().downcast_ref::<arrow::array::Int64Array>().unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) <= i64::MIN || int_array.value(i) >= i64::MAX {
+ return Err(CometError::ArithmeticOverflow{
+ from_type: "integer".to_string(),
+ }.into());
+ }
+ }
+ }
+ DataType::UInt8 => {
Review Comment:
Hmm... you are right. I added these checks in order to handle all the types present in `ScalarValue`. But it seems like we might not need unsigned types.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1617522298
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,38 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ val df = Seq(Int.MaxValue, Int.MinValue).toDF("a")
+ df.write.mode("overwrite").parquet("/tmp/int.parquet")
Review Comment:
It would be better to use `withTempDir`. There are examples of this in other tests in this suite,
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "planga82 (via GitHub)" <gi...@apache.org>.
planga82 commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2147513072
Hi @vaibhawvipul ,
I don't know why, but it seems that a test included in this PR is failing in the main branch in spak 4.0 tests. Any thoughts on this? Thanks in advance!
(- unary negative integer overflow test *** FAILED *** (739 milliseconds)
"[ARITHMETIC_OVERFLOW] short overflow. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error. SQLSTATE: 22003" did not contain " caused overflow" (CometExpressionSuite.scala:1563))
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1620738030
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $max_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val || typed_array.value(i) == $max_val {
Review Comment:
thanks for pointing this out, it lead me to explore spark's codebase. Learnt a lot!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1617524600
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,38 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ val df = Seq(Int.MaxValue, Int.MinValue).toDF("a")
+ df.write.mode("overwrite").parquet("/tmp/int.parquet")
+ spark.read.parquet("/tmp/int.parquet").createTempView("t")
+
+ // without ANSI mode
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> "false",
+ CometConf.COMET_ANSI_MODE_ENABLED.key -> "false",
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true") {
+ checkSparkMaybeThrows(sql("select a, -a from t")) match {
+ case (None, None) => // no exception
+ case _ =>
+ fail("No exception should be thrown")
+ }
Review Comment:
I think you can just use `checkSparkAnswerAndOperator` here to make sure Comet returns the correct results and does not throw an exception.
```suggestion
checkSparkAnswerAndOperator(sql("select a, -a from t"))
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1618206450
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,36 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "int.parquet")
+ val df = Seq(Int.MaxValue, Int.MinValue).toDF("a")
+ df.write.mode("overwrite").parquet(path.toString)
+ spark.read.parquet(path.toString).createTempView("t")
+
+ // without ANSI mode
+ withSQLConf(
Review Comment:
@kazuyukitanimura can you please explain this a bit more?
Do you want me to create a new test to check non-overflow values?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1624946391
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,103 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ def withAnsiMode(enabled: Boolean)(f: => Unit): Unit = {
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ANSI_MODE_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true")(f)
+ }
+
+ def checkOverflow(query: String, dtype: String): Unit = {
+ checkSparkMaybeThrows(sql(query)) match {
+ case (Some(sparkException), Some(cometException)) =>
+ assert(sparkException.getMessage.contains(dtype + " overflow"))
+ assert(cometException.getMessage.contains(dtype + " overflow"))
+ case (None, None) => assert(true) // got same outputs
+ case (None, Some(ex)) =>
+ fail("Comet threw an exception but Spark did not " + ex.getMessage)
+ case (Some(_), None) =>
+ fail("Spark threw an exception but Comet did not")
+ }
+ }
+
+ def runArrayTest(query: String, dtype: String, path: String): Unit = {
+ withParquetTable(path, "t") {
+ withAnsiMode(enabled = false) {
+ checkSparkAnswerAndOperator(sql(query))
+ }
+ withAnsiMode(enabled = true) {
+ checkOverflow(query, dtype)
+ }
+ }
+ }
+ withTempDir { dir =>
+ // Array values test
+ val arrayPath = new Path(dir.toURI.toString, "array_test.parquet").toString
+ Seq(Int.MaxValue, Int.MinValue).toDF("a").write.mode("overwrite").parquet(arrayPath)
+ val arrayQuery = "select a, -a from t"
+ runArrayTest(arrayQuery, "integer", arrayPath)
+
+ // long values test
+ val longArrayPath = new Path(dir.toURI.toString, "long_array_test.parquet").toString
+ Seq(Long.MaxValue, Long.MinValue)
+ .toDF("a")
+ .write
+ .mode("overwrite")
+ .parquet(longArrayPath)
+ val longArrayQuery = "select a, -a from t"
+ runArrayTest(longArrayQuery, "long", longArrayPath)
+
+ // short values test
+ val shortArrayPath = new Path(dir.toURI.toString, "short_array_test.parquet").toString
+ Seq(Short.MaxValue, Short.MinValue)
+ .toDF("a")
+ .write
+ .mode("overwrite")
+ .parquet(shortArrayPath)
+ val shortArrayQuery = "select a, -a from t"
+ runArrayTest(shortArrayQuery, " caused", shortArrayPath)
+
+ // byte values test
+ val byteArrayPath = new Path(dir.toURI.toString, "byte_array_test.parquet").toString
+ Seq(Byte.MaxValue, Byte.MinValue)
+ .toDF("a")
+ .write
+ .mode("overwrite")
+ .parquet(byteArrayPath)
+ val byteArrayQuery = "select a, -a from t"
+ runArrayTest(byteArrayQuery, " caused", byteArrayPath)
+
+ // interval values test
+ withTable("t_interval") {
+ spark.sql("CREATE TABLE t_interval(a STRING) USING PARQUET")
+ spark.sql("INSERT INTO t_interval VALUES ('INTERVAL 10000000000 YEAR')")
+ withAnsiMode(enabled = true) {
+ spark
+ .sql("SELECT CAST(a AS INTERVAL) AS a FROM t_interval")
+ .createOrReplaceTempView("t_interval_casted")
+ checkOverflow("SELECT a, -a FROM t_interval_casted", "interval")
+ }
+ }
Review Comment:
It looks this does not hit native code as `CAST(a AS INTERVAL)` is not supported yet. So it will fall back to Spark and `checkOverflow` is comparing Spark results on both sides.
Perhaps there is no good way of creating interval now...
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,103 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ def withAnsiMode(enabled: Boolean)(f: => Unit): Unit = {
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ANSI_MODE_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true")(f)
+ }
+
+ def checkOverflow(query: String, dtype: String): Unit = {
+ checkSparkMaybeThrows(sql(query)) match {
+ case (Some(sparkException), Some(cometException)) =>
+ assert(sparkException.getMessage.contains(dtype + " overflow"))
+ assert(cometException.getMessage.contains(dtype + " overflow"))
+ case (None, None) => assert(true) // got same outputs
+ case (None, Some(ex)) =>
+ fail("Comet threw an exception but Spark did not " + ex.getMessage)
+ case (Some(_), None) =>
+ fail("Spark threw an exception but Comet did not")
+ }
+ }
+
+ def runArrayTest(query: String, dtype: String, path: String): Unit = {
+ withParquetTable(path, "t") {
+ withAnsiMode(enabled = false) {
+ checkSparkAnswerAndOperator(sql(query))
+ }
+ withAnsiMode(enabled = true) {
+ checkOverflow(query, dtype)
+ }
+ }
+ }
+ withTempDir { dir =>
+ // Array values test
+ val arrayPath = new Path(dir.toURI.toString, "array_test.parquet").toString
+ Seq(Int.MaxValue, Int.MinValue).toDF("a").write.mode("overwrite").parquet(arrayPath)
+ val arrayQuery = "select a, -a from t"
+ runArrayTest(arrayQuery, "integer", arrayPath)
+
+ // long values test
+ val longArrayPath = new Path(dir.toURI.toString, "long_array_test.parquet").toString
+ Seq(Long.MaxValue, Long.MinValue)
+ .toDF("a")
+ .write
+ .mode("overwrite")
+ .parquet(longArrayPath)
+ val longArrayQuery = "select a, -a from t"
+ runArrayTest(longArrayQuery, "long", longArrayPath)
+
+ // short values test
+ val shortArrayPath = new Path(dir.toURI.toString, "short_array_test.parquet").toString
+ Seq(Short.MaxValue, Short.MinValue)
+ .toDF("a")
+ .write
+ .mode("overwrite")
+ .parquet(shortArrayPath)
+ val shortArrayQuery = "select a, -a from t"
+ runArrayTest(shortArrayQuery, " caused", shortArrayPath)
+
+ // byte values test
+ val byteArrayPath = new Path(dir.toURI.toString, "byte_array_test.parquet").toString
+ Seq(Byte.MaxValue, Byte.MinValue)
+ .toDF("a")
+ .write
+ .mode("overwrite")
+ .parquet(byteArrayPath)
+ val byteArrayQuery = "select a, -a from t"
+ runArrayTest(byteArrayQuery, " caused", byteArrayPath)
+
+ // interval values test
+ withTable("t_interval") {
+ spark.sql("CREATE TABLE t_interval(a STRING) USING PARQUET")
+ spark.sql("INSERT INTO t_interval VALUES ('INTERVAL 10000000000 YEAR')")
+ withAnsiMode(enabled = true) {
+ spark
+ .sql("SELECT CAST(a AS INTERVAL) AS a FROM t_interval")
+ .createOrReplaceTempView("t_interval_casted")
+ checkOverflow("SELECT a, -a FROM t_interval_casted", "interval")
+ }
+ }
+
+ withTable("t") {
+ sql("create table t(a int) using parquet")
+ sql("insert into t values (-2147483648)")
+ withAnsiMode(enabled = true) {
+ checkOverflow("select a, -a from t", "integer")
+ }
+ }
+
+ withTable("t_float") {
+ sql("create table t_float(a float) using parquet")
+ sql("insert into t_float values (3.4128235E38)")
+ withAnsiMode(enabled = true) {
+ checkOverflow("select a, -a from t_float", "float")
+ }
+ }
Review Comment:
BTW this will not test scalar case unless we do something like
```
withSQLConf(
"spark.sql.optimizer.excludedRules" ->
"org.apache.spark.sql.catalyst.optimizer.ConstantFolding") {
checkOverflow("select a, -(a) from t_float", "float")
```
I think the current test creates a single item array.
That said, unless that option is set, it is unlikely to hit the scalar scenario. It is ideal to test scalar cases because user jobs may use that option, but it can be a follow up fix.
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,270 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::{compute::kernels::numeric::neg_wrapping, datatypes::IntervalDayTimeType};
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
Review Comment:
I was expecting to see that testing this fails because DataFusion `neg_wrapping` breaks the `i64` into two `i32`.
Then I realized there is no good way of testing right now as mentioned above.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1620648755
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $max_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val || typed_array.value(i) == $max_val {
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.eval_mode == EvalMode::Ansi {
+ match array.data_type() {
+ DataType::Int8 => check_overflow!(
+ array,
+ arrow::array::Int8Array,
+ i8::MIN,
+ i8::MAX,
+ "integer"
+ ),
+ DataType::Int16 => check_overflow!(
+ array,
+ arrow::array::Int16Array,
+ i16::MIN,
+ i16::MAX,
+ "integer"
+ ),
+ DataType::Int32 => check_overflow!(
+ array,
+ arrow::array::Int32Array,
+ i32::MIN,
+ i32::MAX,
+ "integer"
+ ),
+ DataType::Int64 => check_overflow!(
+ array,
+ arrow::array::Int64Array,
+ i64::MIN,
+ i64::MAX,
+ "integer"
+ ),
+ DataType::Float32 => check_overflow!(
+ array,
+ arrow::array::Float32Array,
+ f32::MIN,
+ f32::MAX,
+ "float"
+ ),
+ DataType::Float64 => check_overflow!(
+ array,
+ arrow::array::Float64Array,
+ f64::MIN,
+ f64::MAX,
+ "float"
+ ),
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ i32::MAX,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ i64::MAX,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => check_overflow!(
+ array,
+ arrow::array::IntervalMonthDayNanoArray,
+ i128::MIN,
+ i128::MAX,
+ "interval"
+ ),
+ },
+ _ => unimplemented!(
Review Comment:
Hmm, i have change it to not raise a panic.
There is no overflow checks for decimals for unary minus in spark.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1623002590
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,268 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => {
+ // Overflow checks are not supported
+ }
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
Review Comment:
I just realized that we have `neg_checked` in arrow-rs. Should we simply use it?
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,54 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ def withAnsiMode(enabled: Boolean)(f: => Unit): Unit = {
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ANSI_MODE_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true")(f)
+ }
+
+ def checkOverflow(query: String): Unit = {
+ checkSparkMaybeThrows(sql(query)) match {
+ case (Some(sparkException), Some(cometException)) =>
+ assert(sparkException.getMessage.contains("integer overflow"))
+ assert(cometException.getMessage.contains("integer overflow"))
+ case (None, None) =>
+ fail("Exception should be thrown")
+ case (None, Some(_)) =>
+ fail("Comet threw an exception but Spark did not")
+ case (Some(_), None) =>
+ fail("Spark threw an exception but Comet did not")
+ }
+ }
+
+ def runArrayTest(query: String, path: String): Unit = {
+ withParquetTable(path, "t") {
+ withAnsiMode(enabled = false) {
+ checkSparkAnswerAndOperator(sql(query))
+ }
+ withAnsiMode(enabled = true) {
+ checkOverflow(query)
+ }
+ }
+ }
+
+ withTempDir { dir =>
+ // Array values test
+ val arrayPath = new Path(dir.toURI.toString, "array_test.parquet").toString
+ Seq(Int.MaxValue, Int.MinValue).toDF("a").write.mode("overwrite").parquet(arrayPath)
Review Comment:
+1 for testing intervals
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,268 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => {
+ // Overflow checks are not supported
+ }
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
+ Ok(ColumnarValue::Array(result))
+ }
+ ColumnarValue::Scalar(scalar) => {
+ if self.fail_on_error {
+ match scalar {
+ ScalarValue::Int8(value) => {
+ if value == Some(i8::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int16(value) => {
+ if value == Some(i16::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int32(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("integer").into());
+ }
+ }
+ ScalarValue::Int64(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("long").into());
+ }
+ }
+ ScalarValue::IntervalDayTime(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
Review Comment:
I looked into `arithmetic_negate`
```
ScalarValue::IntervalDayTime(Some(v)) => {
let (days, ms) = IntervalDayTimeType::to_parts(*v);
let val = IntervalDayTimeType::make_value(-days, -ms);
Ok(ScalarValue::IntervalDayTime(Some(val)))
}
```
So we should break `i64` and need to check two values?
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,268 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => {
+ // Overflow checks are not supported
+ }
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
+ Ok(ColumnarValue::Array(result))
+ }
+ ColumnarValue::Scalar(scalar) => {
+ if self.fail_on_error {
+ match scalar {
+ ScalarValue::Int8(value) => {
+ if value == Some(i8::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int16(value) => {
+ if value == Some(i16::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int32(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("integer").into());
+ }
+ }
+ ScalarValue::Int64(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("long").into());
+ }
+ }
+ ScalarValue::IntervalDayTime(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ ScalarValue::IntervalYearMonth(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ Ok(ColumnarValue::Scalar((scalar.arithmetic_negate())?))
Review Comment:
Ideally we should implement `arithmetic_negate_checked` in DataFusion.
DataFusion has `add_checked`, so I feel it is natural to have `arithmetic_negate_checked`. It is okay to keep the current logic until it gets implemented in DataFusion, but we should upstream in my opinion
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1620022028
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $max_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val || typed_array.value(i) == $max_val {
Review Comment:
I think we only care about min case not max.
E.g.
```
-(127i8) => -127i8
-(-128i8)` => overflow
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1620012522
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1959,9 +1959,19 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim
case UnaryMinus(child, _) =>
Review Comment:
Hmmm, should we use the second member `failOnError` instead of reading `SQLConf.get.ansiEnabled`?
Looks like `EvalMode::Try` is not really used, if I understand correctly.
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $max_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val || typed_array.value(i) == $max_val {
Review Comment:
I think we only care about min case not max.
E.g.
```
127i8 => -127i8
`-(-128i8)` => overflow
```
##########
core/src/execution/datafusion/planner.rs:
##########
@@ -566,8 +567,20 @@ impl PhysicalPlanner {
Ok(Arc::new(NotExpr::new(child)))
}
ExprStruct::Negative(expr) => {
- let child = self.create_expr(expr.child.as_ref().unwrap(), input_schema)?;
- Ok(Arc::new(NegativeExpr::new(child)))
+ let child: Arc<dyn PhysicalExpr> =
+ self.create_expr(expr.child.as_ref().unwrap(), input_schema.clone())?;
+ let eval_mode = match expr.eval_mode.as_str() {
+ "ANSI" => EvalMode::Ansi,
+ "TRY" => EvalMode::Try,
+ "LEGACY" => EvalMode::Legacy,
+ other => {
+ return Err(ExecutionError::GeneralError(format!(
+ "Invalid EvalMode: \"{other}\""
+ )))
+ }
+ };
+ let result = negative::create_negate_expr(child, eval_mode);
Review Comment:
Wondering if there are different approaches to avoid fully re-implementing `NegativeExpr`
A. implement this in DataFusion
B. wrap with another operator like `NegativeExpr::new(IsEqualToMin::new(child))`
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,54 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ def withAnsiMode(enabled: Boolean)(f: => Unit): Unit = {
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ANSI_MODE_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true")(f)
+ }
+
+ def checkOverflow(query: String): Unit = {
+ checkSparkMaybeThrows(sql(query)) match {
+ case (Some(sparkException), Some(cometException)) =>
+ assert(sparkException.getMessage.contains("integer overflow"))
+ assert(cometException.getMessage.contains("integer overflow"))
+ case (None, None) =>
+ fail("Exception should be thrown")
+ case (None, Some(_)) =>
+ fail("Comet threw an exception but Spark did not")
+ case (Some(_), None) =>
+ fail("Spark threw an exception but Comet did not")
+ }
+ }
+
+ def runArrayTest(query: String, path: String): Unit = {
+ withParquetTable(path, "t") {
+ withAnsiMode(enabled = false) {
+ checkSparkAnswerAndOperator(sql(query))
+ }
+ withAnsiMode(enabled = true) {
+ checkOverflow(query)
+ }
+ }
+ }
+
+ withTempDir { dir =>
+ // Array values test
+ val arrayPath = new Path(dir.toURI.toString, "array_test.parquet").toString
+ Seq(Int.MaxValue, Int.MinValue).toDF("a").write.mode("overwrite").parquet(arrayPath)
Review Comment:
Hmm it may be a good idea to test with more types
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $max_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val || typed_array.value(i) == $max_val {
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.eval_mode == EvalMode::Ansi {
+ match array.data_type() {
+ DataType::Int8 => check_overflow!(
+ array,
+ arrow::array::Int8Array,
+ i8::MIN,
+ i8::MAX,
+ "integer"
+ ),
+ DataType::Int16 => check_overflow!(
+ array,
+ arrow::array::Int16Array,
+ i16::MIN,
+ i16::MAX,
+ "integer"
+ ),
+ DataType::Int32 => check_overflow!(
+ array,
+ arrow::array::Int32Array,
+ i32::MIN,
+ i32::MAX,
+ "integer"
+ ),
+ DataType::Int64 => check_overflow!(
+ array,
+ arrow::array::Int64Array,
+ i64::MIN,
+ i64::MAX,
+ "integer"
+ ),
+ DataType::Float32 => check_overflow!(
+ array,
+ arrow::array::Float32Array,
+ f32::MIN,
+ f32::MAX,
+ "float"
+ ),
+ DataType::Float64 => check_overflow!(
+ array,
+ arrow::array::Float64Array,
+ f64::MIN,
+ f64::MAX,
+ "float"
+ ),
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ i32::MAX,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ i64::MAX,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => check_overflow!(
+ array,
+ arrow::array::IntervalMonthDayNanoArray,
+ i128::MIN,
+ i128::MAX,
+ "interval"
+ ),
+ },
+ _ => unimplemented!(
Review Comment:
So if other types come in like decimal, this will fail?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2148105329
Ah thank you @andygrove cc @planga82
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1617835890
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,350 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
Review Comment:
I correct myself, probably `unpack_dictionary_type` unpacks early, so dictionary is already unpacked at this point I think
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1617526204
##########
core/src/execution/datafusion/planner.rs:
##########
@@ -566,8 +567,23 @@ impl PhysicalPlanner {
Ok(Arc::new(NotExpr::new(child)))
}
ExprStruct::Negative(expr) => {
- let child = self.create_expr(expr.child.as_ref().unwrap(), input_schema)?;
- Ok(Arc::new(NegativeExpr::new(child)))
+ let child: Arc<dyn PhysicalExpr> =
+ self.create_expr(expr.child.as_ref().unwrap(), input_schema.clone())?;
+ let eval_mode = match expr.eval_mode.as_str() {
+ "ANSI" => EvalMode::Ansi,
+ "TRY" => EvalMode::Try,
+ "LEGACY" => EvalMode::Legacy,
+ other => {
+ return Err(ExecutionError::GeneralError(format!(
+ "Invalid EvalMode: \"{other}\""
+ )))
+ }
+ };
Review Comment:
Oh, my mistake, we won't have this until https://github.com/apache/datafusion-comet/pull/415 is merged.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1616098866
##########
core/src/execution/datafusion/planner.rs:
##########
@@ -566,8 +567,23 @@ impl PhysicalPlanner {
Ok(Arc::new(NotExpr::new(child)))
}
ExprStruct::Negative(expr) => {
- let child = self.create_expr(expr.child.as_ref().unwrap(), input_schema)?;
- Ok(Arc::new(NegativeExpr::new(child)))
+ let child: Arc<dyn PhysicalExpr> =
+ self.create_expr(expr.child.as_ref().unwrap(), input_schema.clone())?;
+ let eval_mode = match expr.eval_mode.as_str() {
+ "ANSI" => EvalMode::Ansi,
+ "TRY" => EvalMode::Try,
+ "LEGACY" => EvalMode::Legacy,
+ other => {
+ return Err(ExecutionError::GeneralError(format!(
+ "Invalid EvalMode: \"{other}\""
+ )))
+ }
+ };
Review Comment:
We can make use of the generated protobuf enum here rather than hard-code the strings. This would also allow us to remove the "other" error handling case.
```rust
let eval_mode = match spark_expression::EvalMode::try_from(expr.eval_mode)? {
spark_expression::EvalMode::Ansi => EvalMode::Ansi,
spark_expression::EvalMode::Try => EvalMode::Try,
spark_expression::EvalMode::Legacy => EvalMode::Legacy,
};
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2141305753
I have made all the requested changes, to me it looks ready for CI.
Please let me know. cc @kazuyukitanimura @parthchandra
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1623069437
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,268 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => {
+ // Overflow checks are not supported
+ }
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
Review Comment:
Ah right, sorry I meant `neg()` can be used instead of `neg_wrapping`?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2137819580
@kazuyukitanimura requesting a review.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1620022028
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $max_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val || typed_array.value(i) == $max_val {
Review Comment:
I think we only care about min case not max.
E.g.
```
-(127i8) => -127i8
`-(-128i8)` => overflow
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1623082473
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,268 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => {
+ // Overflow checks are not supported
+ }
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
+ Ok(ColumnarValue::Array(result))
+ }
+ ColumnarValue::Scalar(scalar) => {
+ if self.fail_on_error {
+ match scalar {
+ ScalarValue::Int8(value) => {
+ if value == Some(i8::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int16(value) => {
+ if value == Some(i16::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int32(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("integer").into());
+ }
+ }
+ ScalarValue::Int64(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("long").into());
+ }
+ }
+ ScalarValue::IntervalDayTime(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ ScalarValue::IntervalYearMonth(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ Ok(ColumnarValue::Scalar((scalar.arithmetic_negate())?))
Review Comment:
noted.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1615254711
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,397 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::hash::{Hash, Hasher};
+use std::sync::Arc;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion_common::{Result, ScalarValue};
+use datafusion::logical_expr::interval_arithmetic::Interval;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion::physical_expr::PhysicalExpr;
+use datafusion_physical_expr::aggregate::utils::down_cast_any_ref;
+use datafusion_physical_expr::sort_properties::SortProperties;
+use crate::errors::CometError;
+use crate::execution::datafusion::expressions::cast::EvalMode;
+
+pub fn create_negate_expr(expr: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.eval_mode == EvalMode::Ansi {
+ match array.data_type() {
+ DataType::Int8 => {
+ let int_array = array.as_any().downcast_ref::<arrow::array::Int8Array>().unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) <= i8::MIN || int_array.value(i) >= i8::MAX {
+ return Err(CometError::ArithmeticOverflow{
+ from_type: "integer".to_string(),
+ }.into());
+ }
+ }
+ }
+ DataType::Int16 => {
+ let int_array = array.as_any().downcast_ref::<arrow::array::Int16Array>().unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) <= i16::MIN || int_array.value(i) >= i16::MAX {
+ return Err(CometError::ArithmeticOverflow{
+ from_type: "integer".to_string(),
+ }.into());
+ }
+ }
+ }
+ DataType::Int32 => {
+ let int_array = array.as_any().downcast_ref::<arrow::array::Int32Array>().unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) <= i32::MIN || int_array.value(i) >= i32::MAX {
+ return Err(CometError::ArithmeticOverflow{
+ from_type: "integer".to_string(),
+ }.into());
+ }
+ }
+ }
+ DataType::Int64 => {
+ let int_array = array.as_any().downcast_ref::<arrow::array::Int64Array>().unwrap();
+ for i in 0..int_array.len() {
+ if int_array.value(i) <= i64::MIN || int_array.value(i) >= i64::MAX {
+ return Err(CometError::ArithmeticOverflow{
+ from_type: "integer".to_string(),
+ }.into());
+ }
+ }
+ }
+ DataType::UInt8 => {
Review Comment:
Do we ever hit the path for Uint types? Java only has signed integers, so was surprised to see that we need to handle UInt.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1618086496
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,350 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.eval_mode == EvalMode::Ansi {
+ match array.data_type() {
+ DataType::Int8 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int8Array>()
+ .expect("Int8Array");
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i8::MIN || int_array.value(i) == i8::MAX {
+ return Err(arithmetic_overflow_error("integer").into());
+ }
+ }
Review Comment:
This is a good suggestion! Implemented.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1619176000
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,36 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "int.parquet")
+ val df = Seq(Int.MaxValue, Int.MinValue).toDF("a")
+ df.write.mode("overwrite").parquet(path.toString)
+ spark.read.parquet(path.toString).createTempView("t")
+
+ // without ANSI mode
+ withSQLConf(
Review Comment:
@kazuyukitanimura I have added a Scalar test.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1616099527
##########
core/src/execution/datafusion/planner.rs:
##########
@@ -566,8 +567,23 @@ impl PhysicalPlanner {
Ok(Arc::new(NotExpr::new(child)))
}
ExprStruct::Negative(expr) => {
- let child = self.create_expr(expr.child.as_ref().unwrap(), input_schema)?;
- Ok(Arc::new(NegativeExpr::new(child)))
+ let child: Arc<dyn PhysicalExpr> =
+ self.create_expr(expr.child.as_ref().unwrap(), input_schema.clone())?;
+ let eval_mode = match expr.eval_mode.as_str() {
+ "ANSI" => EvalMode::Ansi,
+ "TRY" => EvalMode::Try,
+ "LEGACY" => EvalMode::Legacy,
+ other => {
+ return Err(ExecutionError::GeneralError(format!(
+ "Invalid EvalMode: \"{other}\""
+ )))
+ }
+ };
+ let result = negative::create_negate_expr(child, eval_mode);
+ match result {
+ Ok(expr) => Ok(expr),
+ Err(e) => Err(ExecutionError::GeneralError(e.to_string())),
+ }
Review Comment:
I think you could use `map_err` here rather than have a match statement for changing the error.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1620841169
##########
core/src/execution/datafusion/planner.rs:
##########
@@ -566,8 +567,20 @@ impl PhysicalPlanner {
Ok(Arc::new(NotExpr::new(child)))
}
ExprStruct::Negative(expr) => {
- let child = self.create_expr(expr.child.as_ref().unwrap(), input_schema)?;
- Ok(Arc::new(NegativeExpr::new(child)))
+ let child: Arc<dyn PhysicalExpr> =
+ self.create_expr(expr.child.as_ref().unwrap(), input_schema.clone())?;
+ let eval_mode = match expr.eval_mode.as_str() {
+ "ANSI" => EvalMode::Ansi,
+ "TRY" => EvalMode::Try,
+ "LEGACY" => EvalMode::Legacy,
+ other => {
+ return Err(ExecutionError::GeneralError(format!(
+ "Invalid EvalMode: \"{other}\""
+ )))
+ }
+ };
+ let result = negative::create_negate_expr(child, eval_mode);
Review Comment:
Hmm.. In case we want to do this, I would like to probably go with the second approach.
I would like to also hear what @andygrove has to say.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1620839777
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1959,9 +1959,19 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim
case UnaryMinus(child, _) =>
Review Comment:
This was nice, thanks.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2147590729
> Hi @vaibhawvipul ,
>
> I don't know why, but it seems that a test included in this PR is failing in the main branch in spak 4.0 tests. Any thoughts on this? Thanks in advance!
>
> (- unary negative integer overflow test *** FAILED *** (739 milliseconds) "[ARITHMETIC_OVERFLOW] short overflow. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error. SQLSTATE: 22003" did not contain " caused overflow" (CometExpressionSuite.scala:1563))
Hmm.. I think there was another PR which got merged after mine, that is breaking the tests.
<img width="681" alt="Screenshot 2024-06-04 at 7 19 10 PM" src="https://github.com/apache/datafusion-comet/assets/10995936/f9f6cee2-1c1f-4af6-b4d3-8e74d9a2e7ad">
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2132224792
Adding evidence that we have parity with spark with and without ANSI mode.
Comet and Spark outputs with ANSI false -
<img width="1437" alt="Screenshot 2024-05-26 at 6 55 47 PM" src="https://github.com/apache/datafusion-comet/assets/10995936/507a7bd7-b56a-4224-8211-7b6817c4224a">
Comet and Spark outputs with ANSI true -
Spark -
<img width="1440" alt="Screenshot 2024-05-26 at 6 56 25 PM" src="https://github.com/apache/datafusion-comet/assets/10995936/b2417920-88ad-45d6-a7f8-24a68e9f630f">
Comet -
<img width="1440" alt="Screenshot 2024-05-26 at 6 56 53 PM" src="https://github.com/apache/datafusion-comet/assets/10995936/ee54590d-4e66-41ad-9b21-4f9ac32f77c4">
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2134509094
@andygrove we have a test case now which shows that we have parity.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1617816507
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,36 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "int.parquet")
+ val df = Seq(Int.MaxValue, Int.MinValue).toDF("a")
+ df.write.mode("overwrite").parquet(path.toString)
+ spark.read.parquet(path.toString).createTempView("t")
Review Comment:
I think `withParquetTable()` can simplify these lines
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,36 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "int.parquet")
+ val df = Seq(Int.MaxValue, Int.MinValue).toDF("a")
+ df.write.mode("overwrite").parquet(path.toString)
+ spark.read.parquet(path.toString).createTempView("t")
+
+ // without ANSI mode
+ withSQLConf(
Review Comment:
I would say let's add scalar and dictionary tests
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,350 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.eval_mode == EvalMode::Ansi {
+ match array.data_type() {
+ DataType::Int8 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int8Array>()
+ .expect("Int8Array");
+ for i in 0..int_array.len() {
+ if int_array.value(i) == i8::MIN || int_array.value(i) == i8::MAX {
+ return Err(arithmetic_overflow_error("integer").into());
+ }
+ }
Review Comment:
nit: these parts are repeating, so macro might be helpful
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1959,9 +1959,19 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim
case UnaryMinus(child, _) =>
val childExpr = exprToProtoInternal(child, inputs)
+ val evalMode = SQLConf.get.ansiEnabled
+ val evalModeStr = evalMode match {
+ case bool: Boolean =>
+ // Spark 3.2 & 3.3 has ansiEnabled boolean
+ if (bool) "ANSI" else "LEGACY"
+ case _ =>
+ // Spark 3.4+ has EvalMode enum with values LEGACY, ANSI, and TRY
+ evalMode.toString
Review Comment:
Ideally this should go to shims
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,350 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
Review Comment:
Wondering if this works with dictionary...
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1616100740
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,408 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::{errors::CometError, execution::datafusion::expressions::cast::EvalMode};
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, eval_mode)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ eval_mode: EvalMode,
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, eval_mode: EvalMode) -> Self {
+ Self { arg, eval_mode }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.eval_mode == EvalMode::Ansi {
+ match array.data_type() {
+ DataType::Int8 => {
+ let int_array = array
+ .as_any()
+ .downcast_ref::<arrow::array::Int8Array>()
+ .unwrap();
Review Comment:
I think it would be better to use `expect` rather than `unwrap` when downcasting arrays.
```rust
.expect("Int8Array");
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1617820865
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,36 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "int.parquet")
+ val df = Seq(Int.MaxValue, Int.MinValue).toDF("a")
+ df.write.mode("overwrite").parquet(path.toString)
+ spark.read.parquet(path.toString).createTempView("t")
+
+ // without ANSI mode
+ withSQLConf(
Review Comment:
I would say let's add scalar tests
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1621791279
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,272 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) <= $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Float64 => {
+ check_overflow!(array, arrow::array::Float64Array, f64::MIN, "float")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => check_overflow!(
+ array,
+ arrow::array::IntervalMonthDayNanoArray,
+ i128::MIN,
+ "interval"
+ ),
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
+ Ok(ColumnarValue::Array(result))
+ }
+ ColumnarValue::Scalar(scalar) => {
+ if self.fail_on_error {
+ match scalar {
+ ScalarValue::Int8(value) => {
+ if value == Some(i8::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int16(value) => {
+ if value == Some(i16::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int32(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("integer").into());
+ }
+ }
+ ScalarValue::Int64(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("long").into());
+ }
+ }
+ ScalarValue::Float64(value) => {
+ if value == Some(f64::MIN) || value == Some(f64::MAX) {
+ return Err(arithmetic_overflow_error("float").into());
+ }
+ }
+ ScalarValue::IntervalDayTime(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ ScalarValue::IntervalYearMonth(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ ScalarValue::IntervalMonthDayNano(value) => {
+ if value == Some(i128::MIN) || value == Some(i128::MAX) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ Ok(ColumnarValue::Scalar((scalar.arithmetic_negate())?))
+ }
+ }
+ }
+
+ fn children(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ vec![self.arg.clone()]
+ }
+
+ fn with_new_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ Ok(Arc::new(NegativeExpr::new(
+ children[0].clone(),
+ self.fail_on_error,
+ )))
+ }
+
+ fn dyn_hash(&self, state: &mut dyn Hasher) {
+ let mut s = state;
+ self.hash(&mut s);
+ }
+
+ /// Given the child interval of a NegativeExpr, it calculates the NegativeExpr's interval.
+ /// It replaces the upper and lower bounds after multiplying them with -1.
+ /// Ex: `(a, b]` => `[-b, -a)`
+ fn evaluate_bounds(&self, children: &[&Interval]) -> Result<Interval> {
+ Interval::try_new(
+ children[0].upper().arithmetic_negate()?,
+ children[0].lower().arithmetic_negate()?,
Review Comment:
added checks, i think it will be in parity with what we are doing above in the code.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2143496623
@kazuyukitanimura this is ready for review.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1618170030
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1959,9 +1959,19 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim
case UnaryMinus(child, _) =>
val childExpr = exprToProtoInternal(child, inputs)
+ val evalMode = SQLConf.get.ansiEnabled
+ val evalModeStr = evalMode match {
+ case bool: Boolean =>
+ // Spark 3.2 & 3.3 has ansiEnabled boolean
+ if (bool) "ANSI" else "LEGACY"
+ case _ =>
+ // Spark 3.4+ has EvalMode enum with values LEGACY, ANSI, and TRY
+ evalMode.toString
Review Comment:
I will do that in a different PR. That PR will move this logic to shims for both Cast and UnaryMinus.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1623243501
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1469,5 +1469,54 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("unary negative integer overflow test") {
+ def withAnsiMode(enabled: Boolean)(f: => Unit): Unit = {
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ANSI_MODE_ENABLED.key -> enabled.toString,
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true")(f)
+ }
+
+ def checkOverflow(query: String): Unit = {
+ checkSparkMaybeThrows(sql(query)) match {
+ case (Some(sparkException), Some(cometException)) =>
+ assert(sparkException.getMessage.contains("integer overflow"))
+ assert(cometException.getMessage.contains("integer overflow"))
+ case (None, None) =>
+ fail("Exception should be thrown")
+ case (None, Some(_)) =>
+ fail("Comet threw an exception but Spark did not")
+ case (Some(_), None) =>
+ fail("Spark threw an exception but Comet did not")
+ }
+ }
+
+ def runArrayTest(query: String, path: String): Unit = {
+ withParquetTable(path, "t") {
+ withAnsiMode(enabled = false) {
+ checkSparkAnswerAndOperator(sql(query))
+ }
+ withAnsiMode(enabled = true) {
+ checkOverflow(query)
+ }
+ }
+ }
+
+ withTempDir { dir =>
+ // Array values test
+ val arrayPath = new Path(dir.toURI.toString, "array_test.parquet").toString
+ Seq(Int.MaxValue, Int.MinValue).toDF("a").write.mode("overwrite").parquet(arrayPath)
Review Comment:
This is done, we have an interval test case.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1616794656
##########
core/src/execution/datafusion/planner.rs:
##########
@@ -566,8 +567,23 @@ impl PhysicalPlanner {
Ok(Arc::new(NotExpr::new(child)))
}
ExprStruct::Negative(expr) => {
- let child = self.create_expr(expr.child.as_ref().unwrap(), input_schema)?;
- Ok(Arc::new(NegativeExpr::new(child)))
+ let child: Arc<dyn PhysicalExpr> =
+ self.create_expr(expr.child.as_ref().unwrap(), input_schema.clone())?;
+ let eval_mode = match expr.eval_mode.as_str() {
+ "ANSI" => EvalMode::Ansi,
+ "TRY" => EvalMode::Try,
+ "LEGACY" => EvalMode::Legacy,
+ other => {
+ return Err(ExecutionError::GeneralError(format!(
+ "Invalid EvalMode: \"{other}\""
+ )))
+ }
+ };
Review Comment:
This is nice but I don't see EvalMode in spark_expressions.
I took inspiration from here, Cast expression does similar match - https://github.com/apache/datafusion-comet/pull/471/files#diff-264772aa6994f16ba436ff47d7d15a126e58e78a46d617f22107b376dfbd31e8L351
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1623064056
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,268 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => {
+ // Overflow checks are not supported
+ }
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
+ Ok(ColumnarValue::Array(result))
+ }
+ ColumnarValue::Scalar(scalar) => {
+ if self.fail_on_error {
+ match scalar {
+ ScalarValue::Int8(value) => {
+ if value == Some(i8::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int16(value) => {
+ if value == Some(i16::MIN) {
+ return Err(arithmetic_overflow_error(" caused").into());
+ }
+ }
+ ScalarValue::Int32(value) => {
+ if value == Some(i32::MIN) {
+ return Err(arithmetic_overflow_error("integer").into());
+ }
+ }
+ ScalarValue::Int64(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("long").into());
+ }
+ }
+ ScalarValue::IntervalDayTime(value) => {
+ if value == Some(i64::MIN) {
+ return Err(arithmetic_overflow_error("interval").into());
+ }
+ }
Review Comment:
this is a good idea. implemented.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2143175760
> Sorry I asked many things, but a few more comments
No problem, it greatly improved my contribution. I learnt a lot and I am happy. This exercise will ensure that my next PR won't have this much back-and-forth :)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "vaibhawvipul (via GitHub)" <gi...@apache.org>.
vaibhawvipul commented on code in PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#discussion_r1623048962
##########
core/src/execution/datafusion/expressions/negative.rs:
##########
@@ -0,0 +1,268 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::errors::CometError;
+use arrow::compute::kernels::numeric::neg_wrapping;
+use arrow_array::RecordBatch;
+use arrow_schema::{DataType, Schema};
+use datafusion::{
+ logical_expr::{interval_arithmetic::Interval, ColumnarValue},
+ physical_expr::PhysicalExpr,
+};
+use datafusion_common::{Result, ScalarValue};
+use datafusion_physical_expr::{
+ aggregate::utils::down_cast_any_ref, sort_properties::SortProperties,
+};
+use std::{
+ any::Any,
+ hash::{Hash, Hasher},
+ sync::Arc,
+};
+
+pub fn create_negate_expr(
+ expr: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+) -> Result<Arc<dyn PhysicalExpr>, CometError> {
+ Ok(Arc::new(NegativeExpr::new(expr, fail_on_error)))
+}
+
+/// Negative expression
+#[derive(Debug, Hash)]
+pub struct NegativeExpr {
+ /// Input expression
+ arg: Arc<dyn PhysicalExpr>,
+ fail_on_error: bool,
+}
+
+fn arithmetic_overflow_error(from_type: &str) -> CometError {
+ CometError::ArithmeticOverflow {
+ from_type: from_type.to_string(),
+ }
+}
+
+macro_rules! check_overflow {
+ ($array:expr, $array_type:ty, $min_val:expr, $type_name:expr) => {{
+ let typed_array = $array
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .expect(concat!(stringify!($array_type), " expected"));
+ for i in 0..typed_array.len() {
+ if typed_array.value(i) == $min_val {
+ if $type_name == "byte" || $type_name == "short" {
+ let value = typed_array.value(i).to_string() + " caused";
+ return Err(arithmetic_overflow_error(value.as_str()).into());
+ }
+ return Err(arithmetic_overflow_error($type_name).into());
+ }
+ }
+ }};
+}
+
+impl NegativeExpr {
+ /// Create new not expression
+ pub fn new(arg: Arc<dyn PhysicalExpr>, fail_on_error: bool) -> Self {
+ Self { arg, fail_on_error }
+ }
+
+ /// Get the input expression
+ pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
+ &self.arg
+ }
+}
+
+impl std::fmt::Display for NegativeExpr {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "(- {})", self.arg)
+ }
+}
+
+impl PhysicalExpr for NegativeExpr {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.arg.data_type(input_schema)
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.arg.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ let arg = self.arg.evaluate(batch)?;
+
+ // overflow checks only apply in ANSI mode
+ // datatypes supported are byte, short, integer, long, float, interval
+ match arg {
+ ColumnarValue::Array(array) => {
+ if self.fail_on_error {
+ match array.data_type() {
+ DataType::Int8 => {
+ check_overflow!(array, arrow::array::Int8Array, i8::MIN, "byte")
+ }
+ DataType::Int16 => {
+ check_overflow!(array, arrow::array::Int16Array, i16::MIN, "short")
+ }
+ DataType::Int32 => {
+ check_overflow!(array, arrow::array::Int32Array, i32::MIN, "integer")
+ }
+ DataType::Int64 => {
+ check_overflow!(array, arrow::array::Int64Array, i64::MIN, "long")
+ }
+ DataType::Interval(value) => match value {
+ arrow::datatypes::IntervalUnit::YearMonth => check_overflow!(
+ array,
+ arrow::array::IntervalYearMonthArray,
+ i32::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::DayTime => check_overflow!(
+ array,
+ arrow::array::IntervalDayTimeArray,
+ i64::MIN,
+ "interval"
+ ),
+ arrow::datatypes::IntervalUnit::MonthDayNano => {
+ // Overflow checks are not supported
+ }
+ },
+ _ => {
+ // Overflow checks are not supported for other datatypes
+ }
+ }
+ }
+ let result = neg_wrapping(array.as_ref())?;
Review Comment:
neg wrapping, internally calls neg_checked. I think neg_checked is not directly importable.
```
[`DataType::is_integer`]
pub fn neg_wrapping(array: &dyn Array) -> Result<ArrayRef, ArrowError> {
downcast_integer! {
array.data_type() => (neg_wrapping, array),
_ => neg(array),
}
}
```
neg fn calls neg_checked
```pub fn neg(array: &dyn Array) -> Result<ArrayRef, ArrowError> {
use DataType::*;
use IntervalUnit::*;
use TimeUnit::*;
match array.data_type() {
Int8 => neg_checked!(Int8Type, array),
Int16 => neg_checked!(Int16Type, array),
Int32 => neg_checked!(Int32Type, array),
Int64 => neg_checked!(Int64Type, array),
Float16 => neg_wrapping!(Float16Type, array),
Float32 => neg_wrapping!(Float32Type, array),
Float64 => neg_wrapping!(Float64Type, array),
.....
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement ANSI support for UnaryMinus [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #471:
URL: https://github.com/apache/datafusion-comet/pull/471#issuecomment-2148054852
Ops, looks like this is a merge conflict, I will fix it. Sorry for the inconvenience.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org