From 23069fd138fa142f6e05be38ff4fcab0d0a326bd Mon Sep 17 00:00:00 2001 From: Jefffrey Date: Wed, 1 Jul 2026 22:58:06 +0900 Subject: [PATCH 1/5] Validate short view strings in separate buffer in arrow-row --- arrow-row/src/lib.rs | 60 ------------------------------------- arrow-row/src/variable.rs | 62 +++++++++++++++++++++------------------ 2 files changed, 33 insertions(+), 89 deletions(-) diff --git a/arrow-row/src/lib.rs b/arrow-row/src/lib.rs index 182481f42b4d..73023a633c77 100644 --- a/arrow-row/src/lib.rs +++ b/arrow-row/src/lib.rs @@ -4819,66 +4819,6 @@ mod tests { } } - #[test] - fn test_values_buffer_smaller_when_utf8_validation_disabled() { - fn get_values_buffer_len(col: ArrayRef) -> (usize, usize) { - // 1. Convert cols into rows - let converter = RowConverter::new(vec![SortField::new(DataType::Utf8View)]).unwrap(); - - // 2a. Convert rows into colsa (validate_utf8 = false) - let rows = converter.convert_columns(&[col]).unwrap(); - let converted = converter.convert_rows(&rows).unwrap(); - let unchecked_values_len = converted[0].as_string_view().data_buffers()[0].len(); - - // 2b. Convert rows into cols (validate_utf8 = true since Row is initialized through RowParser) - let rows = rows.try_into_binary().expect("reasonable size"); - let parser = converter.parser(); - let converted = converter - .convert_rows(rows.iter().map(|b| parser.parse(b.expect("valid bytes")))) - .unwrap(); - let checked_values_len = converted[0].as_string_view().data_buffers()[0].len(); - (unchecked_values_len, checked_values_len) - } - - // Case1. StringViewArray with inline strings - let col = Arc::new(StringViewArray::from_iter([ - Some("hello"), // short(5) - None, // null - Some("short"), // short(5) - Some("tiny"), // short(4) - ])) as ArrayRef; - - let (unchecked_values_len, checked_values_len) = get_values_buffer_len(col); - // Since there are no long (>12) strings, len of values buffer is 0 - assert_eq!(unchecked_values_len, 0); - // When utf8 validation enabled, values buffer includes inline strings (5+5+4) - assert_eq!(checked_values_len, 14); - - // Case2. StringViewArray with long(>12) strings - let col = Arc::new(StringViewArray::from_iter([ - Some("this is a very long string over 12 bytes"), - Some("another long string to test the buffer"), - ])) as ArrayRef; - - let (unchecked_values_len, checked_values_len) = get_values_buffer_len(col); - // Since there are no inline strings, expected length of values buffer is the same - assert!(unchecked_values_len > 0); - assert_eq!(unchecked_values_len, checked_values_len); - - // Case3. StringViewArray with both short and long strings - let col = Arc::new(StringViewArray::from_iter([ - Some("tiny"), // 4 (short) - Some("thisisexact13"), // 13 (long) - None, - Some("short"), // 5 (short) - ])) as ArrayRef; - - let (unchecked_values_len, checked_values_len) = get_values_buffer_len(col); - // Since there is single long string, len of values buffer is 13 - assert_eq!(unchecked_values_len, 13); - assert!(checked_values_len > unchecked_values_len); - } - #[test] fn test_sparse_union() { // create a sparse union with Int32 (type_id = 0) and Utf8 (type_id = 1) diff --git a/arrow-row/src/variable.rs b/arrow-row/src/variable.rs index aeb7f888b00e..9ad0eaa77fb1 100644 --- a/arrow-row/src/variable.rs +++ b/arrow-row/src/variable.rs @@ -309,42 +309,40 @@ pub fn decode_binary( } } -fn decode_binary_view_inner( +fn decode_binary_view_inner( rows: &mut [&[u8]], options: SortOptions, - validate_utf8: bool, ) -> BinaryViewArray { let len = rows.len(); let inline_str_max_len = MAX_INLINE_VIEW_LEN as usize; let nulls = decode_nulls_sentinel(rows, options); - // If we are validating UTF-8, decode all string values (including short strings) - // into the values buffer and validate UTF-8 once. If not validating, - // we save memory by only copying long strings to the values buffer, as short strings - // will be inlined into the view and do not need to be stored redundantly. - let values_capacity = if validate_utf8 { - // Capacity for all long and short strings - rows.iter().map(|row| decoded_len(row, options)).sum() + // Capacity for all long strings plus room for one short string + let mut values_capacity = inline_str_max_len; + let mut inline_capacity = 0; + for row in rows.iter() { + let len = decoded_len(row, options); + if len > inline_str_max_len { + values_capacity += len; + } else if VALIDATE_UTF8 { + inline_capacity += len; + } + } + let mut values = MutableBuffer::new(values_capacity); + let mut view_utf8_validation_buffer = if VALIDATE_UTF8 { + Vec::with_capacity(inline_capacity) } else { - // Capacity for all long strings plus room for one short string - rows.iter().fold(0, |acc, row| { - let len = decoded_len(row, options); - if len > inline_str_max_len { - acc + len - } else { - acc - } - }) + inline_str_max_len + Vec::new() }; - let mut values = MutableBuffer::new(values_capacity); let mut views = BufferBuilder::::new(len); for row in rows { let start_offset = values.len(); let offset = decode_blocks(row, options, |b| values.extend_from_slice(b)); - // Measure string length via change in values buffer. - // Used to check if decoded value should be truncated (short string) when validate_utf8 is false + // Measure string length via change in values buffer. This way we can + // overwrite short strings in the values buffer as we inline those to + // views. let decoded_len = values.len() - start_offset; if row[0] == null_sentinel(options) { debug_assert_eq!(offset, 1); @@ -361,18 +359,20 @@ fn decode_binary_view_inner( let view = make_view(val, 0, start_offset as u32); views.append(view); - // truncate inline string in values buffer if validate_utf8 is false - if !validate_utf8 && decoded_len <= inline_str_max_len { + if VALIDATE_UTF8 { + view_utf8_validation_buffer.extend_from_slice(val); + } + + if decoded_len <= inline_str_max_len { values.truncate(start_offset); } } *row = &row[offset..]; } - if validate_utf8 { - // the values contains all data, no matter if it is short or long - // we can validate utf8 in one go. - std::str::from_utf8(values.as_slice()).unwrap(); + if VALIDATE_UTF8 { + std::str::from_utf8(&values).unwrap(); + std::str::from_utf8(&view_utf8_validation_buffer).unwrap(); } // SAFETY: @@ -382,7 +382,7 @@ fn decode_binary_view_inner( /// Decodes a binary view array from `rows` with the provided `options` pub fn decode_binary_view(rows: &mut [&[u8]], options: SortOptions) -> BinaryViewArray { - decode_binary_view_inner(rows, options, false) + decode_binary_view_inner::(rows, options) } /// Decodes a string array from `rows` with the provided `options` @@ -421,7 +421,11 @@ pub unsafe fn decode_string_view( options: SortOptions, validate_utf8: bool, ) -> StringViewArray { - let view = decode_binary_view_inner(rows, options, validate_utf8); + let view = if validate_utf8 { + decode_binary_view_inner::(rows, options) + } else { + decode_binary_view_inner::(rows, options) + }; unsafe { view.to_string_view_unchecked() } } From 0214aea449b0df7761826e60b82c4834c3efa3ae Mon Sep 17 00:00:00 2001 From: Jefffrey Date: Thu, 2 Jul 2026 21:24:17 +0900 Subject: [PATCH 2/5] Repurpose previous test for new behaviour --- arrow-row/src/lib.rs | 57 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/arrow-row/src/lib.rs b/arrow-row/src/lib.rs index 73023a633c77..6dbdb4853ea2 100644 --- a/arrow-row/src/lib.rs +++ b/arrow-row/src/lib.rs @@ -4819,6 +4819,63 @@ mod tests { } } + #[test] + fn test_utf8_validation_doesnt_affect_values_buffer_size() { + fn assert_values_buffer_lens(col: ArrayRef) -> usize { + // 1. Convert cols into rows + let converter = RowConverter::new(vec![SortField::new(DataType::Utf8View)]).unwrap(); + + // 2a. Convert rows into cols (validate_utf8 = false) + let rows = converter.convert_columns(&[col]).unwrap(); + let converted = converter.convert_rows(&rows).unwrap(); + let unchecked_values_len = converted[0].as_string_view().data_buffers()[0].len(); + + // 2b. Convert rows into cols (validate_utf8 = true since Row is initialized through RowParser) + let rows = rows.try_into_binary().expect("reasonable size"); + let parser = converter.parser(); + let converted = converter + .convert_rows(rows.iter().map(|b| parser.parse(b.expect("valid bytes")))) + .unwrap(); + let checked_values_len = converted[0].as_string_view().data_buffers()[0].len(); + // Regardless of utf8 validation flag, we should always have minimal data in buffers + assert_eq!(unchecked_values_len, checked_values_len); + checked_values_len + } + + // Case1. StringViewArray with inline strings + let col = Arc::new(StringViewArray::from_iter([ + Some("hello"), // short(5) + None, // null + Some("short"), // short(5) + Some("tiny"), // short(4) + ])) as ArrayRef; + + let values_len = assert_values_buffer_lens(col); + // Since there are no long (>12) strings, len of values buffer is 0 + assert_eq!(values_len, 0); + + // Case2. StringViewArray with long(>12) strings + let col = Arc::new(StringViewArray::from_iter([ + Some("1234567890123"), // 13 + Some("12345678901234"), // 14 + ])) as ArrayRef; + + let values_len = assert_values_buffer_lens(col); + assert_eq!(values_len, 13 + 14); + + // Case3. StringViewArray with both short and long strings + let col = Arc::new(StringViewArray::from_iter([ + Some("tiny"), // 4 (short) + Some("thisisexact13"), // 13 (long) + None, + Some("short"), // 5 (short) + ])) as ArrayRef; + + let values_len = assert_values_buffer_lens(col); + // Since there is single long string, len of values buffer is 13 + assert_eq!(values_len, 13); + } + #[test] fn test_sparse_union() { // create a sparse union with Int32 (type_id = 0) and Utf8 (type_id = 1) From 6f89c2de17ae7bf0f343b0298b049233497d5e6f Mon Sep 17 00:00:00 2001 From: Jefffrey Date: Thu, 2 Jul 2026 23:47:24 +0900 Subject: [PATCH 3/5] only append to view buffer if its a short string --- arrow-row/src/variable.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arrow-row/src/variable.rs b/arrow-row/src/variable.rs index 0adf0cc21f1e..c056393012bd 100644 --- a/arrow-row/src/variable.rs +++ b/arrow-row/src/variable.rs @@ -359,11 +359,11 @@ fn decode_binary_view_inner( let view = make_view(val, 0, start_offset as u32); views.append(view); - if VALIDATE_UTF8 { - view_utf8_validation_buffer.extend_from_slice(val); - } if decoded_len <= inline_str_max_len { + if VALIDATE_UTF8 { + view_utf8_validation_buffer.extend_from_slice(val); + } values.truncate(start_offset); } } From f520cc06bf8c2193f955259bee0ee7173a76f408 Mon Sep 17 00:00:00 2001 From: Jefffrey Date: Thu, 2 Jul 2026 23:48:41 +0900 Subject: [PATCH 4/5] replace bufferbuilder with vec --- arrow-row/src/variable.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/arrow-row/src/variable.rs b/arrow-row/src/variable.rs index c056393012bd..ca9700b90fc1 100644 --- a/arrow-row/src/variable.rs +++ b/arrow-row/src/variable.rs @@ -336,8 +336,8 @@ fn decode_binary_view_inner( Vec::new() }; - let mut views = BufferBuilder::::new(len); - for row in rows { + let mut views = vec![0_u128; len]; + for (i, row) in rows.iter_mut().enumerate() { let start_offset = values.len(); let offset = decode_blocks(row, options, |b| values.extend_from_slice(b)); // Measure string length via change in values buffer. This way we can @@ -347,7 +347,6 @@ fn decode_binary_view_inner( if row[0] == null_sentinel(options) { debug_assert_eq!(offset, 1); debug_assert_eq!(start_offset, values.len()); - views.append(0); } else { // Safety: we just appended the data to the end of the buffer let val = unsafe { values.get_unchecked_mut(start_offset..) }; @@ -356,9 +355,7 @@ fn decode_binary_view_inner( val.iter_mut().for_each(|o| *o = !*o); } - let view = make_view(val, 0, start_offset as u32); - views.append(view); - + views[i] = make_view(val, 0, start_offset as u32); if decoded_len <= inline_str_max_len { if VALIDATE_UTF8 { From ab0c85cf63b836ed4c354406473f602db25e6618 Mon Sep 17 00:00:00 2001 From: Jefffrey Date: Sun, 5 Jul 2026 09:35:34 +0900 Subject: [PATCH 5/5] fix merge issue --- arrow-row/src/lib.rs | 45 -------------------------------------------- 1 file changed, 45 deletions(-) diff --git a/arrow-row/src/lib.rs b/arrow-row/src/lib.rs index 376b89cf8d1c..8ed2debb6a4b 100644 --- a/arrow-row/src/lib.rs +++ b/arrow-row/src/lib.rs @@ -5445,51 +5445,6 @@ mod tests { assert_eq!(rows.row(0).cmp(&rows.row(1)), Ordering::Less); } - #[test] - fn map_should_be_marked_as_unsupported() { - let map_data_type = Field::new_map( - "map", - "entries", - Field::new("key", DataType::Utf8, false), - Field::new("value", DataType::Utf8, true), - false, - true, - ) - .data_type() - .clone(); - - let is_supported = RowConverter::supports_fields(&[SortField::new(map_data_type)]); - - assert!(!is_supported, "Map should not be supported"); - } - - #[test] - fn should_fail_to_create_row_converter_for_unsupported_map_type() { - let map_data_type = Field::new_map( - "map", - "entries", - Field::new("key", DataType::Utf8, false), - Field::new("value", DataType::Utf8, true), - false, - true, - ) - .data_type() - .clone(); - - let converter = RowConverter::new(vec![SortField::new(map_data_type)]); - - match converter { - Err(ArrowError::NotYetImplemented(message)) => { - assert!( - message.contains("Row format support not yet implemented for"), - "Expected NotYetImplemented error for map data type, got: {message}", - ); - } - Err(e) => panic!("Expected NotYetImplemented error, got: {e}"), - Ok(_) => panic!("Expected NotYetImplemented error for map data type"), - } - } - #[test] fn test_utf8_validation_doesnt_affect_values_buffer_size() { fn assert_values_buffer_lens(col: ArrayRef) -> usize {