tustvold commented on code in PR #1832:
URL: https://github.com/apache/arrow-rs/pull/1832#discussion_r894273291
##########
arrow/src/compute/kernels/substring.rs:
##########
@@ -182,24 +182,66 @@ pub fn substring_by_char<OffsetSize: OffsetSizeTrait>(
start: i64,
length: Option<u64>,
) -> Result<GenericStringArray<OffsetSize>> {
- Ok(array
- .iter()
- .map(|val| {
- val.map(|val| {
- let char_count = val.chars().count();
- let start = if start >= 0 {
- start.to_usize().unwrap().min(char_count)
- } else {
- char_count - (-start).to_usize().unwrap().min(char_count)
- };
- let length = length.map_or(char_count - start, |length| {
- length.to_usize().unwrap().min(char_count - start)
- });
+ let mut vals = BufferBuilder::<u8>::new(array.value_data().len());
+ let mut offsets = BufferBuilder::<OffsetSize>::new(array.len() + 1);
+ offsets.append(OffsetSize::zero());
+ let length = length.map(|len| len.to_usize().unwrap());
+
+ array.iter().for_each(|val| {
+ if let Some(val) = val {
+ let char_count = val.chars().count();
+ let start = if start >= 0 {
+ start.to_usize().unwrap()
+ } else {
+ char_count - (-start).to_usize().unwrap().min(char_count)
+ };
+ let (start_offset, end_offset) = get_start_end_offset(val, start,
length);
+ vals.append_slice(&val.as_bytes()[start_offset..end_offset]);
+ }
+ offsets.append(OffsetSize::from_usize(vals.len()).unwrap());
+ });
+ let data = unsafe {
+ ArrayData::new_unchecked(
+ GenericStringArray::<OffsetSize>::get_data_type(),
+ array.len(),
+ None,
+ array
+ .data_ref()
+ .null_buffer()
+ .map(|b| b.bit_slice(array.offset(), array.len())),
Review Comment:
:ok_hand:
##########
arrow/src/compute/kernels/substring.rs:
##########
@@ -182,24 +182,66 @@ pub fn substring_by_char<OffsetSize: OffsetSizeTrait>(
start: i64,
length: Option<u64>,
) -> Result<GenericStringArray<OffsetSize>> {
- Ok(array
- .iter()
- .map(|val| {
- val.map(|val| {
- let char_count = val.chars().count();
- let start = if start >= 0 {
- start.to_usize().unwrap().min(char_count)
- } else {
- char_count - (-start).to_usize().unwrap().min(char_count)
- };
- let length = length.map_or(char_count - start, |length| {
- length.to_usize().unwrap().min(char_count - start)
- });
+ let mut vals = BufferBuilder::<u8>::new(array.value_data().len());
Review Comment:
If you wanted to improve this estimate you could compute the difference
between the first and last offset. It would still be an over-estimate, but
would over-estimate sliced arrays less
##########
arrow/src/compute/kernels/substring.rs:
##########
@@ -182,24 +182,66 @@ pub fn substring_by_char<OffsetSize: OffsetSizeTrait>(
start: i64,
length: Option<u64>,
) -> Result<GenericStringArray<OffsetSize>> {
- Ok(array
- .iter()
- .map(|val| {
- val.map(|val| {
- let char_count = val.chars().count();
- let start = if start >= 0 {
- start.to_usize().unwrap().min(char_count)
- } else {
- char_count - (-start).to_usize().unwrap().min(char_count)
- };
- let length = length.map_or(char_count - start, |length| {
- length.to_usize().unwrap().min(char_count - start)
- });
+ let mut vals = BufferBuilder::<u8>::new(array.value_data().len());
+ let mut offsets = BufferBuilder::<OffsetSize>::new(array.len() + 1);
+ offsets.append(OffsetSize::zero());
+ let length = length.map(|len| len.to_usize().unwrap());
+
+ array.iter().for_each(|val| {
Review Comment:
A further improvement might be to iterate the offsets directly, as we don't
actually care about skipping over nulls
##########
arrow/src/compute/kernels/substring.rs:
##########
@@ -182,24 +182,66 @@ pub fn substring_by_char<OffsetSize: OffsetSizeTrait>(
start: i64,
length: Option<u64>,
) -> Result<GenericStringArray<OffsetSize>> {
- Ok(array
- .iter()
- .map(|val| {
- val.map(|val| {
- let char_count = val.chars().count();
- let start = if start >= 0 {
- start.to_usize().unwrap().min(char_count)
- } else {
- char_count - (-start).to_usize().unwrap().min(char_count)
- };
- let length = length.map_or(char_count - start, |length| {
- length.to_usize().unwrap().min(char_count - start)
- });
+ let mut vals = BufferBuilder::<u8>::new(array.value_data().len());
+ let mut offsets = BufferBuilder::<OffsetSize>::new(array.len() + 1);
+ offsets.append(OffsetSize::zero());
+ let length = length.map(|len| len.to_usize().unwrap());
+
+ array.iter().for_each(|val| {
+ if let Some(val) = val {
+ let char_count = val.chars().count();
+ let start = if start >= 0 {
+ start.to_usize().unwrap()
+ } else {
+ char_count - (-start).to_usize().unwrap().min(char_count)
+ };
+ let (start_offset, end_offset) = get_start_end_offset(val, start,
length);
+ vals.append_slice(&val.as_bytes()[start_offset..end_offset]);
+ }
+ offsets.append(OffsetSize::from_usize(vals.len()).unwrap());
+ });
+ let data = unsafe {
+ ArrayData::new_unchecked(
+ GenericStringArray::<OffsetSize>::get_data_type(),
+ array.len(),
+ None,
+ array
+ .data_ref()
+ .null_buffer()
+ .map(|b| b.bit_slice(array.offset(), array.len())),
+ 0,
+ vec![offsets.finish(), vals.finish()],
+ vec![],
+ )
+ };
+ Ok(GenericStringArray::<OffsetSize>::from(data))
Review Comment:
Panicking if an invariant is violated I think makes perfect sense, errors
only really make sense imo if you expect some higher level system to catch and
handle that error.
--
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
