You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "neilconway (via GitHub)" <gi...@apache.org> on 2026/10/04 19:58:08 UTC
[PR] perf: Optimize `left`, `right` [datafusion]
neilconway opened a new pull request, #26039:
URL: https://github.com/apache/datafusion/pull/26039
## Which issue does this PR close?
- N/A
## Rationale for this change
#23762 added an ASCII fast-path for `left` and `right`. That improves performance in many scenarios, but the implementation calls `is_ascii` on every input string. That is expensive, particularly for the common case that `left` and `right` are used to fetch a small prefix/suffix from a much longer string. That check is also overly conservative: for example, we can take the ASCII fast-path for `left(s, k)` if the first `k` bytes in the string are ASCII, even if there are multibyte characters elsewhere in the string.
This PR implements two optimizations:
1. Only call `is_ascii` on the bytes necessary to determine if we can take the fast-path, not the entire input string, as described above.
2. Benchmarking identified that for `Utf8View` inputs, the first optimization regressed some benchmark cases (e.g., `negative_n`) on both ARM and x86. Claude's theory is that `make_view` is out-of-line and does an indirect jump on the length of the result string; it seems that after implementing the first optimization, this jump was not well-handled by the branch predictor. Instead, we add a helper `sub_view` that returns a view that is a substring of an existing view. This can be inlined and avoids the indirect jump incurred by `make_view`; it can also construct the new view from the old view with bitwise ops, rather than building the new view on the stack.
Benchmarks: (separate PR #26026)
x86 (AMD EPYC Milan)
- left Utf8 long_result: 101.7 → 93.4 (−8%)
- left Utf8 n_exceeds_len: 91.7 → 93.8 (+2%)
- left Utf8 negative_n: 90.4 → 83.2 (−8%)
- left Utf8 per_row_n: 93.9 → 89.5 (−5%)
- left Utf8 short_result: 91.0 → 82.6 (−9%)
- left Utf8 short_result_long_input: 310.4 → 89.4 (−71%)
- left Utf8View long_result: 53.6 → 40.9 (−24%)
- left Utf8View n_exceeds_len: 55.9 → 50.7 (−9%)
- left Utf8View negative_n: 58.8 → 45.2 (−23%)
- left Utf8View per_row_n: 89.0 → 45.2 (−49%)
- left Utf8View short_result: 54.2 → 47.5 (−12%)
- left Utf8View short_result_long_input: 249.1 → 52.7 (−79%)
- right Utf8 long_result: 109.4 → 91.8 (−16%)
- right Utf8 n_exceeds_len: 97.9 → 98.5 (+1%)
- right Utf8 negative_n: 91.5 → 84.7 (−7%)
- right Utf8 per_row_n: 100.7 → 88.1 (−13%)
- right Utf8 short_result: 96.8 → 87.2 (−10%)
- right Utf8 short_result_long_input: 261.2 → 95.5 (−63%)
- right Utf8View long_result: 64.2 → 53.8 (−16%)
- right Utf8View n_exceeds_len: 63.9 → 56.0 (−12%)
- right Utf8View negative_n: 62.3 → 55.8 (−10%)
- right Utf8View per_row_n: 101.0 → 54.9 (−46%)
- right Utf8View short_result: 62.9 → 58.7 (−7%)
- right Utf8View short_result_long_input: 202.2 → 67.7 (−67%)
ARM (Apple M4 Max)
- left Utf8 long_result: 91.3 → 62.6 (−31%)
- left Utf8 n_exceeds_len: 69.4 → 71.4 (+3%)
- left Utf8 negative_n: 66.8 → 69.5 (+4%)
- left Utf8 per_row_n: 66.6 → 60.6 (−9%)
- left Utf8 short_result: 69.1 → 62.7 (−9%)
- left Utf8 short_result_long_input: 91.1 → 59.6 (−35%)
- left Utf8View long_result: 45.4 → 26.0 (−43%)
- left Utf8View n_exceeds_len: 37.6 → 34.4 (−9%)
- left Utf8View negative_n: 37.8 → 25.9 (−31%)
- left Utf8View per_row_n: 68.1 → 29.4 (−57%)
- left Utf8View short_result: 37.9 → 25.0 (−34%)
- left Utf8View short_result_long_input: 61.1 → 28.9 (−53%)
- right Utf8 long_result: 104.2 → 64.7 (−38%)
- right Utf8 n_exceeds_len: 80.1 → 79.1 (−1%)
- right Utf8 negative_n: 71.4 → 66.0 (−8%)
- right Utf8 per_row_n: 74.0 → 64.3 (−13%)
- right Utf8 short_result: 80.2 → 65.4 (−18%)
- right Utf8 short_result_long_input: 101.1 → 63.4 (−37%)
- right Utf8View long_result: 62.7 → 32.0 (−49%)
- right Utf8View n_exceeds_len: 42.0 → 38.5 (−8%)
- right Utf8View negative_n: 41.3 → 28.5 (−31%)
- right Utf8View per_row_n: 72.0 → 33.5 (−53%)
- right Utf8View short_result: 43.9 → 30.0 (−32%)
- right Utf8View short_result_long_input: 63.7 → 33.3 (−48%)
## What changes are included in this PR?
See above.
## What is the testing strategy for this PR?
Existing tests pass; no functional changes.
## Are there any user-facing changes?
No.
--
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 `left`, `right` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#discussion_r4186481784
##########
datafusion/functions/src/strings.rs:
##########
@@ -1217,6 +1219,73 @@ pub(crate) fn append_view(
views_buffer.push(sub_view);
}
+/// Values of at most this many bytes are stored inline in their view.
+const MAX_INLINE_LEN: usize = MAX_INLINE_VIEW_LEN as usize;
+
+/// Returns the view for a substring of the value an existing view refers to.
+///
+/// # Arguments
+/// - view: The original view value
+/// - source: The bytes the original view refers to
+/// - range: The byte range within `source` of the substring to return a view for
+///
+/// Substrings longer than 12 bytes point into the same data buffer as `view`;
+/// shorter ones are stored inline in the new view.
+///
+/// This uses shifts and masks rather than [`make_view`], which picks copy code
+/// based on the substring's length; the CPU often mispredicts that choice when
+/// lengths vary from row to row.
+#[inline]
+pub(crate) fn sub_view(view: u128, source: &[u8], range: Range<usize>) -> u128 {
+ debug_assert!(range.start <= range.end && range.end <= source.len());
+
+ // The substring's first bytes, in the low-order bits. Any bits past the
+ // end of the substring are masked off by `inline_view`.
+ let leading_bytes = if source.len() <= MAX_INLINE_LEN {
+ // `source` is stored in `view` itself, after its 4-byte length.
+ (view >> 32) >> (8 * range.start)
+ } else {
+ // `source` has more than 12 bytes, so read the 12 bytes starting at
+ // `range.start`, or the last 12 bytes if that would run past the end,
+ // and skip any that come before `range.start`.
+ let window_start = range.start.min(source.len() - MAX_INLINE_LEN);
+ let window = source[window_start..window_start + MAX_INLINE_LEN]
+ .try_into()
+ .unwrap();
+ read_12_bytes(window) >> (8 * (range.start - window_start))
+ };
+
+ let len = range.len();
+ if len <= MAX_INLINE_LEN {
+ inline_view(leading_bytes, len)
+ } else {
+ let original = ByteView::from(view);
+ ByteView {
+ length: len as u32,
+ prefix: leading_bytes as u32,
+ offset: original.offset + range.start as u32,
+ ..original
+ }
+ .as_u128()
+ }
+}
Review Comment:
Yep, makes sense!
--
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 `left`, `right` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#issuecomment-5999011002
On the other comments @comphead:
* I agree that multi-byte benchmarks would be useful, although the same could be said of a lot of the string UDFs :) I'll defer that for now.
* Personally I think leaving `inline_view` as a separate function makes the implementation more readable, so I'm inclined to leave that as-is.
--
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 `left`, `right` [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#discussion_r4186045565
##########
datafusion/functions/src/unicode/common.rs:
##########
@@ -223,7 +233,8 @@ fn general_left_right_view<F: LeftRightSlicer>(
};
// SAFETY:
- // - Each view is produced by `make_view` with correct bytes and offset
+ // - Each view is produced by `sub_view` from the input view and a range
+ // within the input string, as returned by `F::slice`
Review Comment:
`new_unchecked` also requires every result to be valid UTF-8. That holds because `F::slice` returns char boundaries, and with this change that rests on the ASCII end checks in `left_right_byte_length`. Worth listing here.
##########
datafusion/functions/src/unicode/common.rs:
##########
@@ -126,21 +127,34 @@ pub(crate) enum StringCharLen {
#[inline]
fn left_right_byte_length(string: &str, n: i64) -> usize {
let abs = n.unsigned_abs().min(usize::MAX as u64) as usize;
- // For ASCII input every character is exactly one byte, so the byte offset of
- // the n-th codepoint is just the (clamped) character count. This avoids the
- // per-character `char_indices()` scan of the general path.
+ let bytes = string.as_bytes();
+ // ASCII bytes are never part of a multi-byte UTF-8 sequence, so if the
+ // `abs` bytes at the relevant end of the string are ASCII, they are exactly
+ // the `abs` characters at that end. Checking only those bytes is cheaper
+ // than either a `char_indices()` scan or checking the whole string.
match n.cmp(&0) {
Ordering::Equal => 0,
- // `abs` chars trimmed from the end: keep the leading `len - abs`.
- Ordering::Less if string.is_ascii() => string.len().saturating_sub(abs),
- Ordering::Less => string
- .char_indices()
- .nth_back(abs - 1)
- .map(|(index, _)| index)
- .unwrap_or(0),
- // First `abs` chars, but never past the end of the string.
- Ordering::Greater if string.is_ascii() => abs.min(string.len()),
- Ordering::Greater => byte_offset_of_char(string, abs),
+ // Byte offset of the `abs`-th character from the end.
+ Ordering::Less => {
+ let start = bytes.len().saturating_sub(abs);
+ if bytes[start..].is_ascii() {
Review Comment:
When `abs >= bytes.len()` no byte needs inspecting. The result is `0` here (`bytes.len()` in the `Greater` arm) whatever the content, since a string has at most `bytes.len()` chars. Today that case scans the whole string with `is_ascii`, and non-ASCII input then also pays a full `char_indices` pass to reach the same value. That matches `n_exceeds_len` being flat for `Utf8` in the table (-1% to +3%) while `short_result_long_input` drops 35% to 71%.
An early exit per arm would skip it, for example `start == 0 || bytes[start..].is_ascii()` here and `end == bytes.len() || bytes[..end].is_ascii()` below. It adds a length-dependent branch, so `short_result` and `per_row_n`, where only some rows have `abs >= len`, are worth rechecking next to `n_exceeds_len`. I have not measured any of this.
##########
datafusion/functions/src/strings.rs:
##########
@@ -1217,6 +1219,73 @@ pub(crate) fn append_view(
views_buffer.push(sub_view);
}
+/// Values of at most this many bytes are stored inline in their view.
+const MAX_INLINE_LEN: usize = MAX_INLINE_VIEW_LEN as usize;
+
+/// Returns the view for a substring of the value an existing view refers to.
+///
+/// # Arguments
+/// - view: The original view value
+/// - source: The bytes the original view refers to
+/// - range: The byte range within `source` of the substring to return a view for
+///
+/// Substrings longer than 12 bytes point into the same data buffer as `view`;
+/// shorter ones are stored inline in the new view.
+///
+/// This uses shifts and masks rather than [`make_view`], which picks copy code
+/// based on the substring's length; the CPU often mispredicts that choice when
+/// lengths vary from row to row.
+#[inline]
+pub(crate) fn sub_view(view: u128, source: &[u8], range: Range<usize>) -> u128 {
+ debug_assert!(range.start <= range.end && range.end <= source.len());
+
+ // The substring's first bytes, in the low-order bits. Any bits past the
+ // end of the substring are masked off by `inline_view`.
+ let leading_bytes = if source.len() <= MAX_INLINE_LEN {
+ // `source` is stored in `view` itself, after its 4-byte length.
+ (view >> 32) >> (8 * range.start)
+ } else {
+ // `source` has more than 12 bytes, so read the 12 bytes starting at
+ // `range.start`, or the last 12 bytes if that would run past the end,
+ // and skip any that come before `range.start`.
+ let window_start = range.start.min(source.len() - MAX_INLINE_LEN);
+ let window = source[window_start..window_start + MAX_INLINE_LEN]
+ .try_into()
+ .unwrap();
+ read_12_bytes(window) >> (8 * (range.start - window_start))
+ };
+
+ let len = range.len();
+ if len <= MAX_INLINE_LEN {
+ inline_view(leading_bytes, len)
+ } else {
+ let original = ByteView::from(view);
+ ByteView {
+ length: len as u32,
+ prefix: leading_bytes as u32,
+ offset: original.offset + range.start as u32,
+ ..original
+ }
+ .as_u128()
+ }
+}
Review Comment:
The same logic also lives as a private `substr_view` in `string/split_part.rs:509` and `unicode/substrindex.rs:583`. Neither calls `append_view`, so the follow-up can retire three definitions.
--
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 `left`, `right` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway merged PR #26039:
URL: https://github.com/apache/datafusion/pull/26039
--
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 `left`, `right` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#issuecomment-5983833050
FYI @andygrove @comphead
--
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 `left`, `right` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#discussion_r4186491422
##########
datafusion/functions/src/strings.rs:
##########
@@ -1217,6 +1219,73 @@ pub(crate) fn append_view(
views_buffer.push(sub_view);
}
+/// Values of at most this many bytes are stored inline in their view.
+const MAX_INLINE_LEN: usize = MAX_INLINE_VIEW_LEN as usize;
Review Comment:
Thanks, 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 `left`, `right` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#discussion_r4179113579
##########
datafusion/functions/src/strings.rs:
##########
@@ -1217,6 +1219,73 @@ pub(crate) fn append_view(
views_buffer.push(sub_view);
}
+/// Values of at most this many bytes are stored inline in their view.
+const MAX_INLINE_LEN: usize = MAX_INLINE_VIEW_LEN as usize;
+
+/// Returns the view for a substring of the value an existing view refers to.
+///
+/// # Arguments
+/// - view: The original view value
+/// - source: The bytes the original view refers to
+/// - range: The byte range within `source` of the substring to return a view for
+///
+/// Substrings longer than 12 bytes point into the same data buffer as `view`;
+/// shorter ones are stored inline in the new view.
+///
+/// This uses shifts and masks rather than [`make_view`], which picks copy code
+/// based on the substring's length; the CPU often mispredicts that choice when
+/// lengths vary from row to row.
+#[inline]
+pub(crate) fn sub_view(view: u128, source: &[u8], range: Range<usize>) -> u128 {
+ debug_assert!(range.start <= range.end && range.end <= source.len());
+
+ // The substring's first bytes, in the low-order bits. Any bits past the
+ // end of the substring are masked off by `inline_view`.
+ let leading_bytes = if source.len() <= MAX_INLINE_LEN {
+ // `source` is stored in `view` itself, after its 4-byte length.
+ (view >> 32) >> (8 * range.start)
+ } else {
+ // `source` has more than 12 bytes, so read the 12 bytes starting at
+ // `range.start`, or the last 12 bytes if that would run past the end,
+ // and skip any that come before `range.start`.
+ let window_start = range.start.min(source.len() - MAX_INLINE_LEN);
+ let window = source[window_start..window_start + MAX_INLINE_LEN]
+ .try_into()
+ .unwrap();
+ read_12_bytes(window) >> (8 * (range.start - window_start))
+ };
+
+ let len = range.len();
+ if len <= MAX_INLINE_LEN {
+ inline_view(leading_bytes, len)
+ } else {
+ let original = ByteView::from(view);
+ ByteView {
+ length: len as u32,
+ prefix: leading_bytes as u32,
+ offset: original.offset + range.start as u32,
+ ..original
+ }
+ .as_u128()
+ }
+}
Review Comment:
This could potentially be used in other places (e.g., `substr`), but that will require more careful evaluation; I'll defer that for now.
--
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 `left`, `right` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#discussion_r4211562595
##########
datafusion/functions/src/strings.rs:
##########
@@ -1217,6 +1219,73 @@ pub(crate) fn append_view(
views_buffer.push(sub_view);
}
+/// Values of at most this many bytes are stored inline in their view.
+const MAX_INLINE_LEN: usize = MAX_INLINE_VIEW_LEN as usize;
+
+/// Returns the view for a substring of the value an existing view refers to.
+///
+/// # Arguments
+/// - view: The original view value
+/// - source: The bytes the original view refers to
+/// - range: The byte range within `source` of the substring to return a view for
+///
+/// Substrings longer than 12 bytes point into the same data buffer as `view`;
+/// shorter ones are stored inline in the new view.
+///
+/// This uses shifts and masks rather than [`make_view`], which picks copy code
+/// based on the substring's length; the CPU often mispredicts that choice when
+/// lengths vary from row to row.
+#[inline]
+pub(crate) fn sub_view(view: u128, source: &[u8], range: Range<usize>) -> u128 {
+ debug_assert!(range.start <= range.end && range.end <= source.len());
+
+ // The substring's first bytes, in the low-order bits. Any bits past the
+ // end of the substring are masked off by `inline_view`.
+ let leading_bytes = if source.len() <= MAX_INLINE_LEN {
+ // `source` is stored in `view` itself, after its 4-byte length.
+ (view >> 32) >> (8 * range.start)
+ } else {
+ // `source` has more than 12 bytes, so read the 12 bytes starting at
+ // `range.start`, or the last 12 bytes if that would run past the end,
+ // and skip any that come before `range.start`.
+ let window_start = range.start.min(source.len() - MAX_INLINE_LEN);
+ let window = source[window_start..window_start + MAX_INLINE_LEN]
+ .try_into()
+ .unwrap();
+ read_12_bytes(window) >> (8 * (range.start - window_start))
+ };
+
+ let len = range.len();
+ if len <= MAX_INLINE_LEN {
+ inline_view(leading_bytes, len)
+ } else {
+ let original = ByteView::from(view);
+ ByteView {
+ length: len as u32,
+ prefix: leading_bytes as u32,
+ offset: original.offset + range.start as u32,
+ ..original
+ }
+ .as_u128()
+ }
+}
Review Comment:
FYI, the PR for replacing these other definitions with `sub_view` is here: #26121
--
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 `left`, `right` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#issuecomment-5999017139
Thanks for the reviews @jayzhan211 @comphead !
--
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 `left`, `right` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#discussion_r4184841985
##########
datafusion/functions/src/strings.rs:
##########
@@ -1217,6 +1219,73 @@ pub(crate) fn append_view(
views_buffer.push(sub_view);
}
+/// Values of at most this many bytes are stored inline in their view.
+const MAX_INLINE_LEN: usize = MAX_INLINE_VIEW_LEN as usize;
Review Comment:
nit: `MAX_INLINE_LEN` could be `pub(crate)` and reused in `unicode/common.rs`, which currently re-casts `MAX_INLINE_VIEW_LEN as usize` (rustfmt/clippy clean):
```diff
-const MAX_INLINE_LEN: usize = MAX_INLINE_VIEW_LEN as usize;
+pub(crate) const MAX_INLINE_LEN: usize = MAX_INLINE_VIEW_LEN as usize;
```
```diff
-use crate::strings::sub_view;
+use crate::strings::{MAX_INLINE_LEN, sub_view};
use arrow::array::{
- Array, ArrayRef, GenericStringArray, Int64Array, MAX_INLINE_VIEW_LEN,
- OffsetSizeTrait, StringViewArray,
+ Array, ArrayRef, GenericStringArray, Int64Array, OffsetSizeTrait, StringViewArray,
};
@@
- if range.len() > MAX_INLINE_VIEW_LEN as usize {
+ if range.len() > MAX_INLINE_LEN {
```
--
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 `left`, `right` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#discussion_r4186499467
##########
datafusion/functions/src/unicode/common.rs:
##########
@@ -126,21 +127,34 @@ pub(crate) enum StringCharLen {
#[inline]
fn left_right_byte_length(string: &str, n: i64) -> usize {
let abs = n.unsigned_abs().min(usize::MAX as u64) as usize;
- // For ASCII input every character is exactly one byte, so the byte offset of
- // the n-th codepoint is just the (clamped) character count. This avoids the
- // per-character `char_indices()` scan of the general path.
+ let bytes = string.as_bytes();
+ // ASCII bytes are never part of a multi-byte UTF-8 sequence, so if the
+ // `abs` bytes at the relevant end of the string are ASCII, they are exactly
+ // the `abs` characters at that end. Checking only those bytes is cheaper
+ // than either a `char_indices()` scan or checking the whole string.
match n.cmp(&0) {
Ordering::Equal => 0,
- // `abs` chars trimmed from the end: keep the leading `len - abs`.
- Ordering::Less if string.is_ascii() => string.len().saturating_sub(abs),
- Ordering::Less => string
- .char_indices()
- .nth_back(abs - 1)
- .map(|(index, _)| index)
- .unwrap_or(0),
- // First `abs` chars, but never past the end of the string.
- Ordering::Greater if string.is_ascii() => abs.min(string.len()),
- Ordering::Greater => byte_offset_of_char(string, abs),
+ // Byte offset of the `abs`-th character from the end.
+ Ordering::Less => {
+ let start = bytes.len().saturating_sub(abs);
+ if bytes[start..].is_ascii() {
Review Comment:
I agree that that could be a win, but it merits separate evaluation / benchmarking. I'd rather land this and then take a look at that subsequent optimization in a followup.
##########
datafusion/functions/src/unicode/common.rs:
##########
@@ -223,7 +233,8 @@ fn general_left_right_view<F: LeftRightSlicer>(
};
// SAFETY:
- // - Each view is produced by `make_view` with correct bytes and offset
+ // - Each view is produced by `sub_view` from the input view and a range
+ // within the input string, as returned by `F::slice`
Review Comment:
Thanks, 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 `left`, `right` [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26039:
URL: https://github.com/apache/datafusion/pull/26039#issuecomment-5984042096
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26039?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 82.66%. Comparing base ([`9547b09`](https://app.codecov.io/gh/apache/datafusion/commit/9547b091bc99fbcd52c37d67f9ccee645cffc2ae?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b3e5014`](https://app.codecov.io/gh/apache/datafusion/commit/b3e5014c0292a825afe237a486623875000e002c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26039 +/- ##
========================================
Coverage 82.65% 82.66%
========================================
Files 1147 1147
Lines 446087 446404 +317
Branches 446087 446404 +317
========================================
+ Hits 368721 369010 +289
- Misses 54980 55001 +21
- Partials 22386 22393 +7
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26039?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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