`datafusion_proto::physical_plan_to_bytes` has a problem with `characterlength` function

[milenkovicm]

Describe the bug

there is issue when physical plan is deserialized from bytes, it looks like the issue is with specific function, length in this case

To Reproduce

reproduction is simple:

use datafusion::prelude::{SessionContext, CsvReadOptions};
use datafusion::error::Result;
use datafusion_proto::bytes::{physical_plan_to_bytes, physical_plan_from_bytes};

let ctx = SessionContext::new();
ctx.register_csv("example", "example.csv", CsvReadOptions::default()).await?;    
let logical_plan = ctx.state().create_logical_plan("select length(c0) as l from example").await?;
let physical_plan = ctx.state().create_physical_plan(&logical_plan).await?;
let blob = physical_plan_to_bytes(physical_plan)?;

// Error: Plan("There is no UDF named \"characterlength\" in the registry")
let _physical_plan = physical_plan_from_bytes(&blob, &ctx)?;

error "There is no UDF named \"characterlength\" in the registry" is raised. Which is odd as length is not an udf.

wondering if characterlength should be character_length?

Expected behavior

other functions work ok, like trim:

let logical_plan = ctx.state().create_logical_plan("select trim(c0) as l from example").await?;
let physical_plan = ctx.state().create_physical_plan(&logical_plan).await?;

let blob = physical_plan_to_bytes(physical_plan)?;
let _physical_plan = physical_plan_from_bytes(&blob, &ctx)?; // All OK

Additional context

tested with datafusion, and datafusion-proto, version 23 and 24

[alamb]

Thanks for the report @milenkovicm -- I agree this is strange

wondering if characterlength should be character_length?

I agree this sounds likely. A quick look through the code didn't bring up anything obvious though

https://github.com/search?q=repo%3Aapache%2Farrow-datafusion%20characterlength&type=code

[alamb]

I think this looks like a fairly good first issue as there is a clear description and reproducer . Marking it as such

[milenkovicm]

thanks @alamb ,

Got some time to have a look at this.

I believe this is problem with serialization, which serialize it as ScalarUdf thus deserialization can't find it in UDF registry.

to elaborate a bit, at:

https://github.com/apache/arrow-datafusion/blob/412867aa2596a2df93635b6ac31d48aa14111c40/datafusion/proto/src/physical_plan/to_proto.rs#L324

expr.name() for length returns characterlength which is then matched in

https://github.com/apache/arrow-datafusion/blob/98839dbceb835c9e78154a27bb96c9dad58de7c6/datafusion/expr/src/built_in_function.rs#L326

which is expecting character_length, and as it does not match it will jump to else branch

https://github.com/apache/arrow-datafusion/blob/412867aa2596a2df93635b6ac31d48aa14111c40/datafusion/proto/src/physical_plan/to_proto.rs#L339

if i'm not mistaken.

at the end deserialization gets a wrong expression type.

I guess there may be a bit problems with function names with _ in the name, but did not investigate further at this stage.

Update: concat works concat_ws des not (looking for a concatwithseparator)

[alamb]

@milenkovicm your analysis makes sense to me -- maybe we need to store both the original name and the display name on ScalarFunction 🤔

[milenkovicm]

I might say nonsense, we could just match all variants like we have with pow?

[alamb]

I am not quite sure what you mean, but sounds worth a try

[milenkovicm]

Apologies,

What I meant, change

"character_length" => BuiltinScalarFunction::CharacterLength

To

"character_length" | "characterlength"=> BuiltinScalarFunction::CharacterLength

Not sure if it is correct solution but would definitely work 😀

[alamb]

Seems reasonable to be 👍

[milenkovicm]

if we add characterlenght to list of functions in from_str we would probably expose them as sql function

well based on other usage of from_str

https://github.com/apache/arrow-datafusion/blob/19af95253d990283be95544bf7d47530524b59d1/datafusion/sql/src/expr/function.rs#L50

we would be able to do

select characterlength("a") 

and

select character_length("a") 

not sure if it that would be great thing to do.

wdyt @alamb ?

[alamb]

Thanks for the ping @milenkovicm -- I think we have to try some of the solutions and see how they look

[alamb]

I wonder if #6448 may solve this too 🤔

[milenkovicm]

It works for length and concat_ws, just checked it now.

[milenkovicm]

I'm closing this issue, looks like #6448 fixed it, feel free to reopen it if you believe this issue should be addressed in some other way @alamb