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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
