refactor character_length impl by unifying null handling logic
#16877
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
alamb
reviewed
Jul 24, 2025
| } else { | ||
| let values: Vec<_> = (0..array.len()) | ||
| .map(|i| { | ||
| let value = array.value(i); | ||
| // Safety: we are iterating with array.len() so the index is always valid |
There was a problem hiding this comment.
Another idea here is if you know the values are always ascii, you can avoid making a &str at all -- and instead simply compute the character lengths based on the offsets array (for StringArray and LargeStringArray) or the views for `StringViewArray)
| @@ -136,56 +136,31 @@ where | |||
| // string is ASCII only is relatively cheap. | |||
| // If strings are ASCII only, count bytes instead. | |||
| let is_array_ascii_only = array.is_ascii(); | |||
| let array = if array.null_count() == 0 { | |||
| let nulls = array.nulls().cloned(); | |||
There was a problem hiding this comment.
Is the idea to remove the no-nulls optimization because it doesn't make things faster?
There was a problem hiding this comment.
I think avoiding the null check (array.is_null(i)) makes things faster and it seems we don't need 0 as we always cloned array.nulls().cloned()
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
|
I do find revert the null checking logic on non-ascii branch will eliminate regression on those two cases, but it would slow down other cases in the same time. Still not sure why the get value operation is slower only for 32-length UTF-8... Maybe we can make it faster by branching using offsets (like check if start equals to end), but However I find checking if the value is empty (zero length) in advance will make it a bit faster, the regression changes from 10% to 5% while keeping other cases unchanged: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
character_lengthfunction #13696, but I came up with this change when reading Optimize performance ofcharacter_lengthfunction #13696,Rationale for this change
Simplify the implementation by unifying the non-null and nullable branches.
T::default_value()is just 0, which is the same as get.len()from an empty slice returned bystring_array.value(). And in both way, the NULL in result array is marked by null buffer, not.push_null().And I also find a performance improvement by doing it this way (and I verified this is not from
value_unchecked):But there are two outliers
character_length_StringArray_utf8_str_len_32andcharacter_length_StringViewArray_utf8_str_len_32that become 10% slower, for some reason I don't know 😞.What changes are included in this PR?
simplify
character_length's implementationAre these changes tested?
with existing test case
Are there any user-facing changes?