You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Brijesh-Thakkar (via GitHub)" <gi...@apache.org> on 2025/12/31 13:39:42 UTC
[PR] perf: optimize octet_length for string arrays [datafusion]
Brijesh-Thakkar opened a new pull request, #19581:
URL: https://github.com/apache/datafusion/pull/19581
## Which issue does this PR close?
- Addresses apache/datafusion-comet#2986
## Rationale for this change
The `octet_length` scalar function showed significant performance degradation in
Spark workloads when executed via Comet, as reported in the Comet performance EPIC.
The existing implementation relied on the generic Arrow `length` kernel for array
inputs, which introduces unnecessary overhead in vectorized execution. Since
`octet_length` semantics require computing the number of bytes in UTF-8 strings,
this can be implemented more efficiently using Arrow’s concrete string array APIs.
Optimizing this function in DataFusion improves performance for downstream projects
such as Comet and Spark without changing behavior or semantics.
## What changes are included in this PR?
- Replaced the use of the generic Arrow `length` kernel for array inputs in
`octet_length`
- Added a specialized implementation for:
- `StringArray`
- `LargeStringArray`
- `StringViewArray`
- Computed byte lengths directly using `value_length`, avoiding unnecessary
indirection and overhead
- Left the scalar execution path unchanged
## Are these changes tested?
Yes.
- Existing unit tests for `octet_length` were executed and pass successfully
- Core integration tests exercising `octet_length` also pass
- No new tests were required, as existing coverage already validates correctness
across scalar and array inputs, including UTF-8 and null handling
## Are there any user-facing changes?
No.
This change is purely a performance optimization and does not affect:
- SQL syntax
- Function semantics
- Return types
- Error behavior
--
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 octet_length for string arrays [datafusion]
Posted by "Brijesh-Thakkar (via GitHub)" <gi...@apache.org>.
Brijesh-Thakkar commented on PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#issuecomment-3704653373
Yes, that makes sense — I’ll add a small benchmark comparing the previous implementation and this one for StringArray and StringViewArray, and update the PR with the results.
Thanks for the guidance.
--
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 octet_length for string arrays [datafusion]
Posted by "Brijesh-Thakkar (via GitHub)" <gi...@apache.org>.
Brijesh-Thakkar commented on code in PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#discussion_r2655468715
##########
datafusion/functions/src/string/octet_length.rs:
##########
@@ -90,7 +94,45 @@ impl ScalarUDFImpl for OctetLengthFunc {
let [array] = take_function_args(self.name(), &args.args)?;
match array {
- ColumnarValue::Array(v) => Ok(ColumnarValue::Array(length(v.as_ref())?)),
+ ColumnarValue::Array(v) => {
+ let arr: ArrayRef = v.clone();
+
+ if let Some(arr) = arr.as_any().downcast_ref::<StringArray>() {
+ let mut builder = Int32Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i32);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
Review Comment:
Keeping the index-based loop here intentionally.
value_length(i) reads directly from Arrow’s offsets buffer and avoids constructing &str, which is more efficient and aligns with Arrow kernel patterns.
Iterator-based len() would regress performance for octet_length.
--
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 octet_length for string arrays [datafusion]
Posted by "Brijesh-Thakkar (via GitHub)" <gi...@apache.org>.
Brijesh-Thakkar commented on code in PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#discussion_r2655469769
##########
datafusion/functions/src/string/octet_length.rs:
##########
@@ -90,7 +94,45 @@ impl ScalarUDFImpl for OctetLengthFunc {
let [array] = take_function_args(self.name(), &args.args)?;
match array {
- ColumnarValue::Array(v) => Ok(ColumnarValue::Array(length(v.as_ref())?)),
+ ColumnarValue::Array(v) => {
+ let arr: ArrayRef = v.clone();
+
+ if let Some(arr) = arr.as_any().downcast_ref::<StringArray>() {
+ let mut builder = Int32Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i32);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
+ } else if let Some(arr) = arr.as_any().downcast_ref::<LargeStringArray>()
+ {
+ let mut builder = Int64Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i64);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
Review Comment:
Keeping the index-based loop intentionally.
value_length(i) reads directly from the Arrow offsets buffer and avoids constructing &str, which is more efficient and appropriate for octet_length
--
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 octet_length for string arrays [datafusion]
Posted by "Brijesh-Thakkar (via GitHub)" <gi...@apache.org>.
Brijesh-Thakkar commented on PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#issuecomment-3708081881
@Jefffrey How can I run benchmarks locally??
--
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 octet_length for string arrays [datafusion]
Posted by "Brijesh-Thakkar (via GitHub)" <gi...@apache.org>.
Brijesh-Thakkar commented on PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#issuecomment-3702312180
@andygrove Please review 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 octet_length for string arrays [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#issuecomment-3708153870
> @Jefffrey How can I run benchmarks locally??
See some examples of microbenchmarks here: https://github.com/apache/datafusion/pull/19551
They should be able to be run using `cargo bench` command (ensuring you select the correct benchmark, etc.)
--
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 octet_length for string arrays [datafusion]
Posted by "Brijesh-Thakkar (via GitHub)" <gi...@apache.org>.
Brijesh-Thakkar commented on PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#issuecomment-3702644565
@andygrove Hi Andy, I’ve addressed the CI failures (rustfmt + clippy warnings) and pushed the fixes.
This PR optimizes `octet_length` by avoiding generic kernels for string arrays and using direct length access.
All tests are passing locally. Would appreciate your review when you have time.
--
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 octet_length for string arrays [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#issuecomment-3704647291
> I don’t have concrete benchmarks yet — the motivation here was based on reducing obvious overhead (type dispatch + generic kernel) and aligning with patterns used in other string functions in the codebase that operate directly on concrete string arrays.
>
> Happy to add a small micro-benchmark comparing the previous implementation vs this one for StringArray / StringViewArray if that would be useful, or adjust the PR if you’d prefer to keep the generic kernel without measured data.
Performance improvement PRs should come with benchmark results (unless the improvement is clearly obvious in code). Would you be able to provide some benchmark results to see if this PR gives us a performance benefit?
--
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 octet_length for string arrays [datafusion]
Posted by "Brijesh-Thakkar (via GitHub)" <gi...@apache.org>.
Brijesh-Thakkar closed pull request #19581: perf: optimize octet_length for string arrays
URL: https://github.com/apache/datafusion/pull/19581
--
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 octet_length for string arrays [datafusion]
Posted by "Brijesh-Thakkar (via GitHub)" <gi...@apache.org>.
Brijesh-Thakkar commented on PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#issuecomment-3704635146
I don’t have concrete benchmarks yet — the motivation here was based on reducing obvious overhead (type dispatch + generic kernel) and aligning with patterns used in other string functions in the codebase that operate directly on concrete string arrays.
Happy to add a small micro-benchmark comparing the previous implementation vs this one for StringArray / StringViewArray if that would be useful, or adjust the PR if you’d prefer to keep the generic kernel without measured data.
--
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 octet_length for string arrays [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#issuecomment-3704355500
> The existing implementation relied on the generic Arrow `length` kernel for array inputs, which introduces unnecessary overhead in vectorized execution. Since `octet_length` semantics require computing the number of bytes in UTF-8 strings, this can be implemented more efficiently using Arrow’s concrete string array APIs.
I'm curious about this claim; do you have benchmarks to show a speedup here?
--
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 octet_length for string arrays [datafusion]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19581:
URL: https://github.com/apache/datafusion/pull/19581#discussion_r2655423711
##########
datafusion/functions/src/string/octet_length.rs:
##########
@@ -90,7 +94,45 @@ impl ScalarUDFImpl for OctetLengthFunc {
let [array] = take_function_args(self.name(), &args.args)?;
match array {
- ColumnarValue::Array(v) => Ok(ColumnarValue::Array(length(v.as_ref())?)),
+ ColumnarValue::Array(v) => {
+ let arr: ArrayRef = v.clone();
Review Comment:
The clone of the ArrayRef on this line is unnecessary. Since ArrayRef is already an Arc, cloning it is cheap but redundant here. You can use the original `v` directly in the downcasts below without cloning.
Consider removing the clone:
```rust
ColumnarValue::Array(v) => {
if let Some(arr) = v.as_any().downcast_ref::<StringArray>() {
// ...
```
This eliminates an unnecessary atomic reference count increment/decrement.
##########
datafusion/functions/src/string/octet_length.rs:
##########
@@ -90,7 +94,45 @@ impl ScalarUDFImpl for OctetLengthFunc {
let [array] = take_function_args(self.name(), &args.args)?;
match array {
- ColumnarValue::Array(v) => Ok(ColumnarValue::Array(length(v.as_ref())?)),
+ ColumnarValue::Array(v) => {
+ let arr: ArrayRef = v.clone();
+
+ if let Some(arr) = arr.as_any().downcast_ref::<StringArray>() {
+ let mut builder = Int32Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i32);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
+ } else if let Some(arr) = arr.as_any().downcast_ref::<LargeStringArray>()
+ {
+ let mut builder = Int64Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i64);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
Review Comment:
The manual loop with index-based access can be replaced with a more idiomatic and potentially more efficient iterator-based approach. The pattern used in this codebase (e.g., levenshtein.rs lines 184-193) uses `.iter().map().collect()` which is cleaner and allows the compiler to optimize better.
Consider replacing this with:
```rust
let result = arr.iter().map(|s| s.map(|s| s.len() as i64)).collect::<Int64Array>();
Ok(ColumnarValue::Array(Arc::new(result)))
```
This approach:
- Eliminates the need for manual capacity management
- Handles nulls automatically through Option mapping
- Is more consistent with similar functions in the codebase
- May enable better compiler optimizations
##########
datafusion/functions/src/string/octet_length.rs:
##########
@@ -90,7 +94,45 @@ impl ScalarUDFImpl for OctetLengthFunc {
let [array] = take_function_args(self.name(), &args.args)?;
match array {
- ColumnarValue::Array(v) => Ok(ColumnarValue::Array(length(v.as_ref())?)),
+ ColumnarValue::Array(v) => {
+ let arr: ArrayRef = v.clone();
+
+ if let Some(arr) = arr.as_any().downcast_ref::<StringArray>() {
+ let mut builder = Int32Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i32);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
+ } else if let Some(arr) = arr.as_any().downcast_ref::<LargeStringArray>()
+ {
+ let mut builder = Int64Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i64);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
+ } else if let Some(arr) = arr.as_any().downcast_ref::<StringViewArray>() {
+ let mut builder = Int32Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value(i).len() as i32);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
Review Comment:
The manual loop with index-based access can be replaced with a more idiomatic and potentially more efficient iterator-based approach. The pattern used in this codebase (e.g., levenshtein.rs lines 154-163) uses `.iter().map().collect()` which is cleaner and allows the compiler to optimize better.
Consider replacing this with:
```rust
let result = arr.iter().map(|s| s.map(|s| s.len() as i32)).collect::<Int32Array>();
Ok(ColumnarValue::Array(Arc::new(result)))
```
This approach:
- Eliminates the need for manual capacity management
- Handles nulls automatically through Option mapping
- Is more consistent with similar functions in the codebase
- May enable better compiler optimizations
##########
datafusion/functions/src/string/octet_length.rs:
##########
@@ -90,7 +94,45 @@ impl ScalarUDFImpl for OctetLengthFunc {
let [array] = take_function_args(self.name(), &args.args)?;
match array {
- ColumnarValue::Array(v) => Ok(ColumnarValue::Array(length(v.as_ref())?)),
+ ColumnarValue::Array(v) => {
+ let arr: ArrayRef = v.clone();
+
+ if let Some(arr) = arr.as_any().downcast_ref::<StringArray>() {
+ let mut builder = Int32Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i32);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
+ } else if let Some(arr) = arr.as_any().downcast_ref::<LargeStringArray>()
+ {
+ let mut builder = Int64Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i64);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
+ } else if let Some(arr) = arr.as_any().downcast_ref::<StringViewArray>() {
+ let mut builder = Int32Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value(i).len() as i32);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
+ } else {
+ unreachable!("octet_length expects string arrays")
+ }
Review Comment:
There's significant code duplication across the three branches for StringArray, LargeStringArray, and StringViewArray. The logic is identical except for the array type and return type. This can be refactored using a generic helper function or by extracting the common logic.
Consider a refactoring similar to how other functions in this codebase handle multiple string types (e.g., repeat.rs uses a generic `repeat_impl` function with StringArrayType trait). This would:
- Reduce code duplication
- Make the code easier to maintain
- Reduce the chance of introducing bugs when updating one branch but not others
The three branches (lines 100-130) could potentially be consolidated into a single implementation using Arrow's iterator pattern combined with generics.
##########
datafusion/functions/src/string/octet_length.rs:
##########
@@ -90,7 +94,45 @@ impl ScalarUDFImpl for OctetLengthFunc {
let [array] = take_function_args(self.name(), &args.args)?;
match array {
- ColumnarValue::Array(v) => Ok(ColumnarValue::Array(length(v.as_ref())?)),
+ ColumnarValue::Array(v) => {
+ let arr: ArrayRef = v.clone();
+
+ if let Some(arr) = arr.as_any().downcast_ref::<StringArray>() {
+ let mut builder = Int32Builder::with_capacity(arr.len());
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ builder.append_null();
+ } else {
+ builder.append_value(arr.value_length(i) as i32);
+ }
+ }
+ Ok(ColumnarValue::Array(Arc::new(builder.finish())))
Review Comment:
The manual loop with index-based access can be replaced with a more idiomatic and potentially more efficient iterator-based approach. The pattern used in this codebase (e.g., levenshtein.rs lines 169-178) uses `.iter().map().collect()` which is cleaner and allows the compiler to optimize better.
Consider replacing this with:
```rust
let result = arr.iter().map(|s| s.map(|s| s.len() as i32)).collect::<Int32Array>();
Ok(ColumnarValue::Array(Arc::new(result)))
```
This approach:
- Eliminates the need for manual capacity management
- Handles nulls automatically through Option mapping
- Is more consistent with similar functions in the codebase
- May enable better compiler optimizations
--
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