alamb commented on code in PR #9297:
URL: https://github.com/apache/arrow-datafusion/pull/9297#discussion_r1504149258
##########
datafusion/optimizer/Cargo.toml:
##########
@@ -44,6 +44,7 @@ async-trait = { workspace = true }
chrono = { workspace = true }
datafusion-common = { workspace = true }
datafusion-expr = { workspace = true }
+datafusion-functions = { workspace = true }
Review Comment:
I don't think we can add this as a dependency as it prevents the crates from
being published to crates.io. See
https://github.com/apache/arrow-datafusion/issues/9277
I think we should move these tests out of the optimizer crate
For this PR, however, perhaps we could simply update this test to use a
function that has not yet been ported and maybe move the tests in another PR
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -545,7 +545,7 @@ message InListNode {
enum ScalarFunction {
Abs = 0;
- Acos = 1;
+ // 1 was Acos
Review Comment:
I agree that what @jonahgao would allow better backwards compatibility
However, given that we don't really provide any guarantee for compatibility
now anyways (I recently made this explicit in the documentation,
https://github.com/apache/arrow-datafusion/blob/8f3d1ef23f93cd4303745eba76c0850b39774d07/datafusion/proto/src/lib.rs#L37-L41)
I don't think we should add complexity for these functions
##########
datafusion/functions/src/math/acos.rs:
##########
@@ -0,0 +1,95 @@
+// 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.
+
+//! Math function: `acos()`.
+
+use arrow::datatypes::DataType;
+use datafusion_common::{internal_err, Result, DataFusionError};
+use datafusion_expr::ColumnarValue;
+
+use datafusion_expr::{ScalarUDFImpl, Signature, Volatility};
+use std::any::Any;
+use std::sync::Arc;
+use arrow::array::{ArrayRef, Float32Array, Float64Array};
+
+#[derive(Debug)]
+pub struct AcosFunc {
+ signature: Signature,
+}
+
+impl AcosFunc {
+ pub fn new() -> Self {
+ use DataType::*;
+ Self {
+ signature:
+ Signature::uniform(1, vec![Float64, Float32],
Volatility::Immutable)
+ }
+ }
+}
+
+impl ScalarUDFImpl for AcosFunc {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+ fn name(&self) -> &str {
+ "acos"
+ }
+
+ fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ let arg_type = &arg_types[0];
+
+ match arg_type {
+ DataType::Float64 => Ok(DataType::Float64),
+ DataType::Float32 => Ok(DataType::Float32),
+
+ // For other types (possible values null/int), use Float 64
+ _ => Ok(DataType::Float64),
+ }
+
+ }
+
+ fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let args = ColumnarValue::values_to_arrays(args)?;
+
+ let arr: ArrayRef = match args[0].data_type() {
+ DataType::Float64 => {
+ Arc::new(make_function_scalar_inputs_return_type!(
+ &args[0],
+ self.name(),
+ Float64Array,
+ Float64Array,
+ { f64::acos }
+ ))
+ },
+ DataType::Float32 => {
+ Arc::new(make_function_scalar_inputs_return_type!(
+ &args[0],
+ self.name(),
+ Float32Array,
+ Float32Array,
+ { f32::acos }
+ ))
+ },
+ other => return internal_err!("Unsupported data type {other:?} for
function {}", self.name()),
Review Comment:
```suggestion
other => return exec_err!("Unsupported data type {other:?} for
function {}", self.name()),
```
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -589,34 +586,6 @@ async fn
roundtrip_parquet_exec_with_table_partition_cols() -> Result<()> {
roundtrip_test(Arc::new(ParquetExec::new(scan_config, None, None)))
}
-#[test]
-fn roundtrip_builtin_scalar_function() -> Result<()> {
Review Comment:
Until we have completed the migration, can you please simply change to use a
different built in function so we don't lose coverage?
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
