perf: Optimize scalar path for ltrim function#20032
perf: Optimize scalar path for ltrim function#20032kumarUjjawal wants to merge 4 commits intoapache:mainfrom
Conversation
|
Made changes to |
| .iter() | ||
| .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null())) | ||
| { | ||
| if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) { |
There was a problem hiding this comment.
We shouldn't return a columnar array here; it should just be columnar scalar only
| match args[0].data_type() { | ||
| DataType::Utf8 | DataType::Utf8View => make_scalar_function( | ||
| ltrim::<i32>, | ||
| vec![Hint::Pad, Hint::AcceptsSingular], |
There was a problem hiding this comment.
It seems a bit surprising we get such a speedup when make_scalar_function with hints should already ensure we don't expand the arrays 🤔
There was a problem hiding this comment.
I will do another benchmark to ensure i didn't mess up μs and ns.
There was a problem hiding this comment.
Maybe benchmark the null fast path changes separately from this scalar fast path change as well
There was a problem hiding this comment.
I did another benchmark, i had messed up the privious one, now it only shows around ~3% of speedup.
There was a problem hiding this comment.
Could you amend the PR body results in that case, since they seem to mislead the performance improvements of this PR
| } | ||
|
|
||
| // Scalar fast path | ||
| if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) { |
There was a problem hiding this comment.
Using an iter check on this when we have at most 2 arguments is overkill; just use a match
There was a problem hiding this comment.
Would subsequent PRs for other trim fundtions a good effort or should I leave those?
There was a problem hiding this comment.
Can we identify in this PR which changes actually lead to speedup? If its the null handling or the new scalar fast path? I'm hesistant on this PR since though we have small gains, the scalar fast path in particular is confusing since it tacks on a new scalar fast path when the existing fast path still remains (but would remain unused?)
There was a problem hiding this comment.
I did all the possible combination benchmark and the scalar path provides negligible speedup also the common.rs changes are net negative. We are better droping of this PR.
There was a problem hiding this comment.
Thanks for checking this 👍
There was a problem hiding this comment.
Should I close this?
There was a problem hiding this comment.
Yes I think that would be best
There was a problem hiding this comment.
Thanks @Jefffrey for your time. I should have done the benchmark properly since the start.
2 participants
Which issue does this PR close?
Rationale for this change
ltrimcurrently routes scalar inputs throughmake_scalar_function, which converts scalar values into size-1 arrays and then converts the result back. This adds avoidable overhead in constant-folding / scalar evaluation scenarios.What changes are included in this PR?
LtrimFunc::invoke_with_argsUtf8,Utf8View, andLargeUtf8ltrim/scalar_utf8ltrim/scalar_utf8viewAre these changes tested?
Yes
Are there any user-facing changes?
No