Move make_date, to_char to datafusion-functions #9601
Conversation
# Conflicts: # datafusion/expr/src/built_in_function.rs # datafusion/expr/src/expr_fn.rs # datafusion/functions/src/datetime/mod.rs # datafusion/physical-expr/src/datetime_expressions.rs # datafusion/physical-expr/src/functions.rs # datafusion/proto/src/logical_plan/from_proto.rs # datafusion/proto/src/logical_plan/to_proto.rs
github-actions
bot
added
logical-expr
| @@ -42,8 +38,8 @@ fn random_date_in_range( | |||
| start_date + TimeDelta::try_days(random_days).unwrap() | |||
| } | |||
|
|
|||
| fn data(rng: &mut ThreadRng) -> Date32Array { | |||
| let mut data: Vec<i32> = vec![]; | |||
| fn data(rng: &mut ThreadRng) -> Vec<Expr> { | |||
There was a problem hiding this comment.
doesn't this change the meaning of the benchmark from testing to_char on a single array to testing to_char on a single row with 1000's of arguments?
Like to_date(v1, v2, v3, .....)
instead of to_date(arr) ?
Is that intentional?
There was a problem hiding this comment.
The benchmark functions iterate on the data and patterns so that shouldn't be the case.
let data = data(&mut rng);
let patterns = patterns(&mut rng);
b.iter(|| {
data.iter().enumerate().for_each(|(idx, i)| {
black_box(to_char(i.clone(), patterns.get(idx).unwrap().clone()));
})
})
There was a problem hiding this comment.
Right, but then the benchmark is simply testing how fast the to_char expr_fn function is creating a Expr::Function call (but not actually evaluting the expression)
data.iter().enumerate().for_each(|(idx, i)| {
black_box(to_char(i.clone(), patterns.get(idx).unwrap().clone()));
})There was a problem hiding this comment.
The numbers looked amazing though :)
| use datafusion_expr::ColumnarValue; | ||
| use datafusion_physical_expr::datetime_expressions::to_char; | ||
| use datafusion_expr::{lit, Expr}; | ||
| use datafusion_functions::expr_fn::to_char; |
There was a problem hiding this comment.
I think this is the wrong function to be benchmarking -- it should be the implementation (not the expr_fn)
# Conflicts: # datafusion/expr/src/built_in_function.rs # datafusion/physical-expr/src/lib.rs
Which issue does this PR close?
Closes #9538
Rationale for this change
Function migration to new crate
What changes are included in this PR?
Code, tests
Are these changes tested?
Yes
Are there any user-facing changes?
No.