erratic-pattern commented on code in PR #10454:
URL: https://github.com/apache/datafusion/pull/10454#discussion_r1597448118
##########
datafusion/expr/src/expr.rs:
##########
@@ -1654,28 +1654,42 @@ fn fmt_function(
write!(f, "{}({}{})", fun, distinct_str, args.join(", "))
}
-fn create_function_name(fun: &str, distinct: bool, args: &[Expr]) ->
Result<String> {
- let names: Vec<String> =
args.iter().map(create_name).collect::<Result<_>>()?;
- let distinct_str = match distinct {
- true => "DISTINCT ",
- false => "",
- };
- Ok(format!("{}({}{})", fun, distinct_str, names.join(",")))
+fn write_function_name<'a>(
+ w: &'a mut (dyn Write + 'a),
Review Comment:
> Rather than using dynamic dispatch I think you can make this just a normal
generic function and make the code simpler and likely more performant
>
> Something like
>
> ```rust
> fn write_function_name<W: Write>(
> w: &mut W,
> ```
>
> I actually tried it locally and it seems to work well. Here is a PR
[erratic-pattern#1](https://github.com/erratic-pattern/arrow-datafusion/pull/1)
to this branch for your consideration
I didn't find a meaningful difference in performance here vs monomorphic
types (I tried `&mut String` explicitly in my testing, which is similar code to
the generic here). In fact
[`Formatter`](https://doc.rust-lang.org/src/core/fmt/mod.rs.html#254) in the
standard library also has an internal `dyn Write`, though it has an actual need
for ad-hoc polymorphism whereas we don't. I am guessing it gets optimized in
most cases, but it certainly wouldn't hurt to make it explicitly generic so I
agree with this change.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
