You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kumarUjjawal (via GitHub)" <gi...@apache.org> on 2026/01/27 16:06:55 UTC
[PR] perf: Optimize scalar path for ltrim function [datafusion]
kumarUjjawal opened a new pull request, #20032:
URL: https://github.com/apache/datafusion/pull/20032
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Part of https://github.com/apache/datafusion/issues/2984 #.
## Rationale for this change
`ltrim` currently routes scalar inputs through `make_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.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
- Add a match-based scalar fast path in `LtrimFunc::invoke_with_args`
- Handles `Utf8`, `Utf8View`, and `LargeUtf8`
- Handles early null returns for scalar null inputs
| Type | Before | After | Speedup |
|------|--------|-------|---------|
| `ltrim/scalar_utf8` | 653.92 ns | 113.48 ns | **5.8x** |
| `ltrim/scalar_utf8view` | 553.59 ns | 112.99 ns | **4.9x** |
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2736739122
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
+ let arg0 = &args[0];
+ let arg1 = args.get(1);
+
+ let (value, pattern) = match (arg0, arg1) {
+ (ColumnarValue::Scalar(s0), None) => (s0, None),
+ (ColumnarValue::Scalar(s0), Some(ColumnarValue::Scalar(s1))) => {
+ (s0, Some(s1))
+ }
+ _ => {
+ return internal_err!(
+ "Unexpected argument combination in ltrim scalar fast path"
+ );
+ }
+ };
+
+ let trim_chars: Vec<char> = match pattern {
+ None => vec![' '],
+ Some(ScalarValue::Utf8(Some(p)))
+ | Some(ScalarValue::LargeUtf8(Some(p)))
+ | Some(ScalarValue::Utf8View(Some(p))) => p.chars().collect(),
+ Some(other) => {
+ return internal_err!(
+ "Unexpected data type {:?} for ltrim pattern",
+ other.data_type()
+ );
+ }
+ };
+
+ let trimmed = match value {
+ ScalarValue::Utf8(Some(s)) => ScalarValue::Utf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::Utf8View(Some(s)) => ScalarValue::Utf8View(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::LargeUtf8(Some(s)) => ScalarValue::LargeUtf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ other => {
+ return internal_err!(
+ "Unexpected data type {:?} for function ltrim",
+ other.data_type()
+ );
+ }
+ };
+
+ return Ok(ColumnarValue::Scalar(trimmed));
+ }
+
+ // Array path
+ match args[0].data_type() {
DataType::Utf8 | DataType::Utf8View => make_scalar_function(
ltrim::<i32>,
vec![Hint::Pad, Hint::AcceptsSingular],
Review Comment:
Maybe benchmark the null fast path changes separately from this scalar fast path change as well
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2740494312
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
Review Comment:
Would subsequent PRs for other `trim` fundtions a good effort or should I leave those?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2740993661
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
Review Comment:
Thanks for checking this 👍
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#issuecomment-3806653906
Made changes to `common.rs` as I wanted to work on other `trim` functions in seperate PRs and it would be handy to have the shared code.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2740705838
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
Review 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2740153188
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
+ let arg0 = &args[0];
+ let arg1 = args.get(1);
+
+ let (value, pattern) = match (arg0, arg1) {
+ (ColumnarValue::Scalar(s0), None) => (s0, None),
+ (ColumnarValue::Scalar(s0), Some(ColumnarValue::Scalar(s1))) => {
+ (s0, Some(s1))
+ }
+ _ => {
+ return internal_err!(
+ "Unexpected argument combination in ltrim scalar fast path"
+ );
+ }
+ };
+
+ let trim_chars: Vec<char> = match pattern {
+ None => vec![' '],
+ Some(ScalarValue::Utf8(Some(p)))
+ | Some(ScalarValue::LargeUtf8(Some(p)))
+ | Some(ScalarValue::Utf8View(Some(p))) => p.chars().collect(),
+ Some(other) => {
+ return internal_err!(
+ "Unexpected data type {:?} for ltrim pattern",
+ other.data_type()
+ );
+ }
+ };
+
+ let trimmed = match value {
+ ScalarValue::Utf8(Some(s)) => ScalarValue::Utf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::Utf8View(Some(s)) => ScalarValue::Utf8View(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::LargeUtf8(Some(s)) => ScalarValue::LargeUtf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ other => {
+ return internal_err!(
+ "Unexpected data type {:?} for function ltrim",
+ other.data_type()
+ );
+ }
+ };
+
+ return Ok(ColumnarValue::Scalar(trimmed));
+ }
+
+ // Array path
+ match args[0].data_type() {
DataType::Utf8 | DataType::Utf8View => make_scalar_function(
ltrim::<i32>,
vec![Hint::Pad, Hint::AcceptsSingular],
Review Comment:
I did another benchmark, i had messed up the privious one, now it only shows around ~3% of speedup.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2741767173
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
Review Comment:
Should I close this?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2740436865
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
+ let arg0 = &args[0];
+ let arg1 = args.get(1);
+
+ let (value, pattern) = match (arg0, arg1) {
+ (ColumnarValue::Scalar(s0), None) => (s0, None),
+ (ColumnarValue::Scalar(s0), Some(ColumnarValue::Scalar(s1))) => {
+ (s0, Some(s1))
+ }
+ _ => {
+ return internal_err!(
+ "Unexpected argument combination in ltrim scalar fast path"
+ );
+ }
+ };
+
+ let trim_chars: Vec<char> = match pattern {
+ None => vec![' '],
+ Some(ScalarValue::Utf8(Some(p)))
+ | Some(ScalarValue::LargeUtf8(Some(p)))
+ | Some(ScalarValue::Utf8View(Some(p))) => p.chars().collect(),
+ Some(other) => {
+ return internal_err!(
+ "Unexpected data type {:?} for ltrim pattern",
+ other.data_type()
+ );
+ }
+ };
+
+ let trimmed = match value {
+ ScalarValue::Utf8(Some(s)) => ScalarValue::Utf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::Utf8View(Some(s)) => ScalarValue::Utf8View(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::LargeUtf8(Some(s)) => ScalarValue::LargeUtf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ other => {
+ return internal_err!(
+ "Unexpected data type {:?} for function ltrim",
+ other.data_type()
+ );
+ }
+ };
+
+ return Ok(ColumnarValue::Scalar(trimmed));
+ }
+
+ // Array path
+ match args[0].data_type() {
DataType::Utf8 | DataType::Utf8View => make_scalar_function(
ltrim::<i32>,
vec![Hint::Pad, Hint::AcceptsSingular],
Review Comment:
Could you amend the PR body results in that case, since they seem to mislead the performance improvements of this PR
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2744036901
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
Review Comment:
Yes I think that would be best
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2740577743
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
Review 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?)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2744474625
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
Review Comment:
Thanks @Jefffrey for your time. I should have done the benchmark properly since the start.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2736728108
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
+ let arg0 = &args[0];
+ let arg1 = args.get(1);
+
+ let (value, pattern) = match (arg0, arg1) {
+ (ColumnarValue::Scalar(s0), None) => (s0, None),
+ (ColumnarValue::Scalar(s0), Some(ColumnarValue::Scalar(s1))) => {
+ (s0, Some(s1))
+ }
+ _ => {
+ return internal_err!(
+ "Unexpected argument combination in ltrim scalar fast path"
+ );
+ }
+ };
+
+ let trim_chars: Vec<char> = match pattern {
+ None => vec![' '],
+ Some(ScalarValue::Utf8(Some(p)))
+ | Some(ScalarValue::LargeUtf8(Some(p)))
+ | Some(ScalarValue::Utf8View(Some(p))) => p.chars().collect(),
+ Some(other) => {
+ return internal_err!(
+ "Unexpected data type {:?} for ltrim pattern",
+ other.data_type()
+ );
+ }
+ };
+
+ let trimmed = match value {
+ ScalarValue::Utf8(Some(s)) => ScalarValue::Utf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::Utf8View(Some(s)) => ScalarValue::Utf8View(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::LargeUtf8(Some(s)) => ScalarValue::LargeUtf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ other => {
+ return internal_err!(
+ "Unexpected data type {:?} for function ltrim",
+ other.data_type()
+ );
+ }
+ };
+
+ return Ok(ColumnarValue::Scalar(trimmed));
+ }
+
+ // Array path
+ match args[0].data_type() {
DataType::Utf8 | DataType::Utf8View => make_scalar_function(
ltrim::<i32>,
vec![Hint::Pad, Hint::AcceptsSingular],
Review Comment:
I will do another benchmark to ensure i didn't mess up μs and ns.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2740491287
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
Review Comment:
Done
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal closed pull request #20032: perf: Optimize scalar path for ltrim function
URL: https://github.com/apache/datafusion/pull/20032
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Optimize scalar path for ltrim function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20032:
URL: https://github.com/apache/datafusion/pull/20032#discussion_r2736679294
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
Review Comment:
We shouldn't return a columnar array here; it should just be columnar scalar only
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
+ let arg0 = &args[0];
+ let arg1 = args.get(1);
+
+ let (value, pattern) = match (arg0, arg1) {
+ (ColumnarValue::Scalar(s0), None) => (s0, None),
+ (ColumnarValue::Scalar(s0), Some(ColumnarValue::Scalar(s1))) => {
+ (s0, Some(s1))
+ }
+ _ => {
+ return internal_err!(
+ "Unexpected argument combination in ltrim scalar fast path"
+ );
+ }
+ };
+
+ let trim_chars: Vec<char> = match pattern {
+ None => vec![' '],
+ Some(ScalarValue::Utf8(Some(p)))
+ | Some(ScalarValue::LargeUtf8(Some(p)))
+ | Some(ScalarValue::Utf8View(Some(p))) => p.chars().collect(),
+ Some(other) => {
+ return internal_err!(
+ "Unexpected data type {:?} for ltrim pattern",
+ other.data_type()
+ );
+ }
+ };
+
+ let trimmed = match value {
+ ScalarValue::Utf8(Some(s)) => ScalarValue::Utf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::Utf8View(Some(s)) => ScalarValue::Utf8View(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ ScalarValue::LargeUtf8(Some(s)) => ScalarValue::LargeUtf8(Some(
+ s.trim_start_matches(&trim_chars[..]).to_string(),
+ )),
+ other => {
+ return internal_err!(
+ "Unexpected data type {:?} for function ltrim",
+ other.data_type()
+ );
+ }
+ };
+
+ return Ok(ColumnarValue::Scalar(trimmed));
+ }
+
+ // Array path
+ match args[0].data_type() {
DataType::Utf8 | DataType::Utf8View => make_scalar_function(
ltrim::<i32>,
vec![Hint::Pad, Hint::AcceptsSingular],
Review 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 🤔
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -119,18 +119,87 @@ impl ScalarUDFImpl for LtrimFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
- match args.args[0].data_type() {
+ let return_type = args.return_field.data_type();
+ let number_rows = args.number_rows;
+ let args = args.args;
+
+ // If any argument is a scalar NULL, the output is NULL for all rows
+ if args
+ .iter()
+ .any(|v| matches!(v, ColumnarValue::Scalar(s) if s.is_null()))
+ {
+ if args.iter().any(|v| matches!(v, ColumnarValue::Array(_))) {
+ return Ok(ColumnarValue::Array(arrow::array::new_null_array(
+ return_type,
+ number_rows,
+ )));
+ }
+ return Ok(ColumnarValue::Scalar(ScalarValue::try_from(return_type)?));
+ }
+
+ // Scalar fast path
+ if args.iter().all(|v| matches!(v, ColumnarValue::Scalar(_))) {
Review Comment:
Using an iter check on this when we have at most 2 arguments is overkill; just use a match
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org