perf: Optimize trunc scalar performance#19788
Conversation
| match &args.args[1] { | ||
| ColumnarValue::Scalar(Int64(Some(p))) => *p, | ||
| ColumnarValue::Scalar(Int64(None)) => { | ||
| return Ok(ColumnarValue::Scalar(ScalarValue::Float64(None))); |
There was a problem hiding this comment.
I think this should check the scalar type to decide whether to return Float64 or Float32
|
|
||
| fn compute_truncate32(x: f32, y: i64) -> f32 { | ||
| let factor = 10.0_f32.powi(y as i32); | ||
| (x * factor).round() / factor |
There was a problem hiding this comment.
Not introduced in this PR but why f32::round() is used here instead of f32::trunc() ?
Same for f64 below.
fn main() {
let factor = 10_f64;
let r = (3.76_f64 * factor).round() / factor;
let t = (3.76_f64 * factor).trunc() / factor;
println!("round: {r}\ntrunc: {t}");
}prints:
round: 3.8
trunc: 3.7
There was a problem hiding this comment.
Yeah it does seem like a bug. I will file an issue
| }; | ||
|
|
||
| match scalar { | ||
| ScalarValue::Float64(v) => { |
There was a problem hiding this comment.
Fast path for ScalarValue::Null too ?!
| ScalarValue::Float64(v) => { | ||
| let result = v.map(|x| { | ||
| if precision == 0 { | ||
| if x == 0.0 { 0.0 } else { x.trunc() } |
There was a problem hiding this comment.
| if x == 0.0 { 0.0 } else { x.trunc() } | |
| x.trunc() |
| ScalarValue::Float32(v) => { | ||
| let result = v.map(|x| { | ||
| if precision == 0 { | ||
| if x == 0.0 { 0.0 } else { x.trunc() } |
There was a problem hiding this comment.
| if x == 0.0 { 0.0 } else { x.trunc() } | |
| x.trunc() |
| )]; | ||
| let scalar_arg_fields = vec![Field::new("a", DataType::Float64, false).into()]; | ||
| let scalar_return_field = Field::new("f", DataType::Float64, false).into(); | ||
| let config_options = Arc::new(ConfigOptions::default()); |
There was a problem hiding this comment.
nit: This variable shadows the same one from line 40
|
Thanks for the feedback @martin-g, incorporated the changes. |
| return make_scalar_function(trunc, vec![])(&args.args); | ||
| } | ||
| None => Some(0), // default precision | ||
| _ => Some(0), |
There was a problem hiding this comment.
This catch all arm should return an internal error, unless theres a case I'm missing?
There was a problem hiding this comment.
Yes it should. Made changes
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
|
Thanks @kumarUjjawal & @martin-g |
3 participants
Which issue does this PR close?
Rationale for this change
The current
truncimplementation always converts scalar inputs to arrays viamake_scalar_function, which introduces unnecessary overhead when processing single values.What changes are included in this PR?
truncfunction to process Float32/Float64 scalar inputs directlyAre these changes tested?
Yes all sqllogictest pass
Benchmark Results
Are there any user-facing changes?
No