refactor: move acos() to function crate #9297

SteveLauC · 2024-02-21T03:09:52Z

Which issue does this PR close?

Part of #9285

Rationale for this change

What changes are included in this PR?

Move math function acos() to the function crate

Are these changes tested?

Let's try this in CI:<

Are there any user-facing changes?

I guess no

alamb · 2024-02-21T07:46:36Z

Thanks @SteveLauC -- it appears there are some CI failuers

SteveLauC · 2024-02-21T07:47:42Z

it appears there are some CI failuers

Indeed, I am working on the sqllogictest failure

SteveLauC

Self review

SteveLauC · 2024-02-21T09:44:00Z

datafusion/functions/src/math/acos.rs

+            DataType::Float32 => {
+                Arc::new(make_function_scalar_inputs_return_type!(
+                            &args[0],
+                            "x",


Suggested change

"x",

self.name(),

I forgot to change this one, BTW, should this be done? I saw that isnan() is using "x"

SteveLauC · 2024-02-21T09:44:54Z

datafusion/functions/src/math/mod.rs

@@ -15,15 +15,18 @@
 // specific language governing permissions and limitations
 // under the License.

-//! "core" DataFusion functions
+//! "math" DataFusion functions


Seems to be a copy-paste error, so I changed it

SteveLauC · 2024-02-21T09:45:21Z

datafusion/functions/src/math/nans.rs

@@ -15,7 +15,7 @@
 // specific language governing permissions and limitations
 // under the License.

-//! Encoding expressions


Also, seems to be a copy-paste error

SteveLauC · 2024-02-21T09:47:58Z

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<()> {


I removed this test because we are going to elimiate buit-in functions

Until we have completed the migration, can you please simply change to use a different built in function so we don't lose coverage?

SteveLauC · 2024-02-21T09:48:29Z

datafusion/proto/Cargo.toml

@@ -47,6 +47,7 @@ chrono = { workspace = true }
 datafusion = { path = "../core", version = "36.0.0" }
 datafusion-common = { workspace = true }
 datafusion-expr = { workspace = true }
+datafusion-functions = { workspace = true }


I should remove this as it is unused

SteveLauC · 2024-02-21T09:53:17Z

Hi @alamb, I think I might need some help on the sqllogictest CI error, with the acos() function implemented in this PR:

select(acos(NULL) would return a column of type ? rather than R
select(acos(0), acos(1)) would return columns of type II rather than RR

Honestly, I have no idea about the above behavios...

SteveLauC · 2024-02-21T10:10:09Z

Hi @alamb, I think I might need some help on the sqllogictest CI error, with the acos() function implemented in this PR:

select(acos(NULL) would return a column of type ? rather than R

select(acos(0), acos(1)) would return columns of type II rather than RR

Honestly, I have no idea about the above behavios...

Update: I just realized that it is a mistake that I made in <AsocFunc as ScalarUDFImpl>::return_type()

It feels that when we do select(acos(NULL)), AsocFunc::return_type() can see the passed DataType::Null, but AsocFunc::invoke() cannot because I didn't see the following panic message:

Unsupported data type DataType::Null for function asoc

It seems that DataFusion will handle those special values for you.

jonahgao · 2024-02-22T02:21:32Z

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 }


Is it only used in testing? Then it should be placed under dev-dependencies.

Good catch! Will do it.

FYI @Omega359 had a similar challenge when looking at timestamp functions. We are thinking it might be better to move the tests rather than add a new dependency -- see #9291 (comment)

I don't think we can add this as a dependency as it prevents the crates from being published to crates.io. See #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

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

done

jonahgao · 2024-02-22T02:40:50Z

datafusion/proto/proto/datafusion.proto

@@ -545,7 +545,7 @@ message InListNode {

 enum ScalarFunction {
  Abs = 0;
-  Acos = 1;
+  // 1 was Acos


Should we consider keeping it, perhaps by renaming it to Deprecated_Acos, and then decode it into the new ScalarUDF to maintain better backward compatibility?

We might need to mark the BuiltinScalarFunction::Acos as deprecated as well.

hould we consider keeping it, perhaps by renaming it to Deprecated_Acos, and then decode it into the new ScalarUDF to maintain better backward compatibility?

I followed the way that how #9216 handed it, gentle ping on @alamb, maybe you have some thoughts on this

Sorry about the delay in review, I was out last week)

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

jonahgao · 2024-02-22T02:55:24Z

select acos() will panic, but I think it should be checked at a higher level, before the implementation of acos.

DataFusion CLI v36.0.0
❯ select acos();
thread 'main' panicked at datafusion/functions/src/math/acos.rs:57:25:
index out of bounds: the len is 0 but the index is 0

In the main branch, it just throws an error.

DataFusion CLI v36.0.0
❯ select acos();
Error during planning: No function matches the given name and argument types 'acos()'. You might need to add explicit type casts.
	Candidate functions:
	acos(Float64/Float32)

SteveLauC · 2024-02-22T03:04:50Z

select acos() will panic, but I think it should be checked at a higher level, before the implementation of acos.

Huge thanks for catching it!

Just realized isnan() also has this issue, cc @alamb:

$ ./target/debug/datafusion-cli
DataFusion CLI v36.0.0
❯ select isnan();
thread 'main' panicked at /home/steve/Documents/workspace/GitHub/arrow-datafusion/datafusion/functions/src/math/nans.rs:67:39:
index out of bounds: the len is 0 but the index is 0

And cc @yyy1000, abs() also suffers from this.

but I think it should be checked at a higher level, before the implementation of acos.

Yeah, we should do it before invoking the function

yyy1000 · 2024-02-22T03:40:40Z

select acos() will panic, but I think it should be checked at a higher level, before the implementation of acos.

Huge thanks for catching it!

Just realized isnan() also has this issue, cc @alamb:
$ ./target/debug/datafusion-cli
DataFusion CLI v36.0.0
❯ select isnan();
thread 'main' panicked at /home/steve/Documents/workspace/GitHub/arrow-datafusion/datafusion/functions/src/math/nans.rs:67:39:
index out of bounds: the len is 0 but the index is 0
And cc @yyy1000, abs() also suffers from this.

but I think it should be checked at a higher level, before the implementation of acos.

Yeah, we should do it before invoking the function

Yeah, thanks for telling me @SteveLauC
I added the logic just now. :)

alamb

Thank you @SteveLauC and @jonahgao -- I apologize for the delay in reviewing this PR.

I think the new dependency between the optimizer and core crates needs to be fixed prior to merging this PR (I left suggestions on how to do so).

Otherwise, I think this PR is pretty close to ready to go other than some test tweaks

Thank you again so much

alamb · 2024-02-27T12:27:27Z

datafusion/functions/src/math/acos.rs

+                            { f32::acos }
+                        ))
+            },
+            other => return internal_err!("Unsupported data type {other:?} for function {}", self.name()),


Suggested change

other => return internal_err!("Unsupported data type {other:?} for function {}", self.name()),

other => return exec_err!("Unsupported data type {other:?} for function {}", self.name()),

alamb · 2024-02-27T12:29:50Z

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 }


I don't think we can add this as a dependency as it prevents the crates from being published to crates.io. See #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

alamb · 2024-02-27T12:32:26Z

datafusion/proto/proto/datafusion.proto

@@ -545,7 +545,7 @@ message InListNode {

 enum ScalarFunction {
  Abs = 0;
-  Acos = 1;
+  // 1 was Acos


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

alamb · 2024-02-27T12:33:06Z

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<()> {


Until we have completed the migration, can you please simply change to use a different built in function so we don't lose coverage?

alamb

Thank you @SteveLauC -- this looks great to me 🙏

alamb · 2024-02-27T22:33:46Z

datafusion/optimizer/src/analyzer/type_coercion.rs

@@ -889,14 +889,14 @@ mod test {
        // test that automatic argument type coercion for scalar functions work
        let empty = empty();
        let lit_expr = lit(10i64);
-        let fun: BuiltinScalarFunction = BuiltinScalarFunction::Acos;
+        let fun: BuiltinScalarFunction = BuiltinScalarFunction::Floor;


alamb · 2024-02-27T22:37:21Z

I took the liberty of merging up from main and resolving a conflict on this PR

alamb · 2024-02-28T00:44:44Z

Thanks again @SteveLauC

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules labels Feb 21, 2024

alamb marked this pull request as draft February 21, 2024 07:46

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 21, 2024

SteveLauC commented Feb 21, 2024

View reviewed changes

github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Feb 21, 2024

SteveLauC marked this pull request as ready for review February 21, 2024 10:10

jonahgao reviewed Feb 22, 2024

View reviewed changes

alamb mentioned this pull request Feb 26, 2024

DataFusion weekly project plan (Andrew Lamb) - Feb 26, 2024 #9345

Closed

9 tasks

alamb reviewed Feb 27, 2024

View reviewed changes

SteveLauC force-pushed the refactor/move_acos_to_function_crate branch from bed7322 to e455453 Compare February 27, 2024 12:45

alamb approved these changes Feb 27, 2024

View reviewed changes

alamb mentioned this pull request Feb 27, 2024

[Epic] Port BuiltInFunctons to datafusion-functions-* crates #9285

Closed

43 tasks

refactor: move acos() to function crate

b75f1cf

SteveLauC force-pushed the refactor/move_acos_to_function_crate branch from 29909d9 to b75f1cf Compare February 28, 2024 00:06

alamb approved these changes Feb 28, 2024

View reviewed changes

alamb merged commit 935ebca into apache:main Feb 28, 2024
24 checks passed

SteveLauC deleted the refactor/move_acos_to_function_crate branch February 28, 2024 00:56

SteveLauC mentioned this pull request Feb 29, 2024

Minor: Move function signature check to planning stage #9401

Merged

	other => return internal_err!("Unsupported data type {other:?} for function {}", self.name()),
	other => return exec_err!("Unsupported data type {other:?} for function {}", self.name()),

refactor: move acos() to function crate #9297

refactor: move acos() to function crate #9297

Conversation

SteveLauC commented Feb 21, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb commented Feb 21, 2024

SteveLauC commented Feb 21, 2024

SteveLauC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveLauC commented Feb 21, 2024

SteveLauC commented Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

SteveLauC Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgao commented Feb 22, 2024 • edited Loading

SteveLauC commented Feb 22, 2024 • edited Loading

yyy1000 commented Feb 22, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Feb 27, 2024 • edited Loading

alamb commented Feb 28, 2024

SteveLauC commented Feb 21, 2024 •

edited

Loading

SteveLauC Feb 22, 2024 •

edited

Loading

jonahgao commented Feb 22, 2024 •

edited

Loading

SteveLauC commented Feb 22, 2024 •

edited

Loading

alamb commented Feb 27, 2024 •

edited

Loading