viirya commented on code in PR #1577: URL: https://github.com/apache/arrow-rs/pull/1577#discussion_r851800150
########## arrow/src/compute/kernels/substring.rs: ########## @@ -35,21 +36,39 @@ fn generic_substring<OffsetSize: StringOffsetSizeTrait>( let data = values.as_slice(); let zero = OffsetSize::zero(); - let cal_new_start: Box<dyn Fn(OffsetSize, OffsetSize) -> OffsetSize> = if start - >= zero - { - // count from the start of string - Box::new(|old_start: OffsetSize, end: OffsetSize| (old_start + start).min(end)) - } else { - // count from the end of string - Box::new(|old_start: OffsetSize, end: OffsetSize| (end + start).max(old_start)) + // check the `idx` is a valid char boundary or not + // If yes, return `idx`, else return error + let check_char_boundary = |idx: OffsetSize| { + let idx_usize = idx.to_usize().unwrap(); + if idx_usize == data.len() || data[idx_usize] >> 6 != 0b10_u8 { + Ok(idx) + } else { + Err(ArrowError::ComputeError(format!("The index {} is invalid because {:#010b} is not the head byte of a char.", idx_usize, data[idx_usize]))) Review Comment: The error message may not be understood by end-users. Maybe just say "The substring begins at index {} is an invalid utf8 char because it is not the first byte in a utf8 code point sequence"? -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org