Jefffrey commented on code in PR #10672:
URL: https://github.com/apache/arrow-rs/pull/10672#discussion_r3771844234
##########
arrow-string/src/substring.rs:
##########
@@ -247,6 +250,68 @@ fn utf8_bounds(val: &str, start: i64, length:
Option<usize>) -> (usize, usize) {
(start_offset, end_offset)
}
+/// Byte range of one element, following the same rules as [`byte_substring`].
+fn view_substring_range(len: usize, start: i64, length: Option<u64>) ->
(usize, usize) {
Review Comment:
its confusing to have both `len` and `length`, perhaps rename them to
something like `original_length` and `substring_length`
##########
arrow-string/src/substring.rs:
##########
@@ -1035,4 +1100,90 @@ mod tests {
let expected = BinaryArray::from(vec![Some(expected_bytes)]);
assert_eq!(expected, *actual);
}
+
+ /// The view result has to match the [`DataType::Utf8`] result for the
same input.
+ #[test]
+ fn string_view_matches_utf8() {
+ let values = vec![
+ Some("hello world"),
+ Some(""),
+ None,
+ Some("a"),
+ Some("this one is definitely longer than twelve bytes"),
+ ];
+ let utf8 = StringArray::from(values.clone());
+ let view = StringViewArray::from(values);
+
+ for (start, length) in [
+ (0, None),
+ (0, Some(0)),
+ (0, Some(5)),
+ (0, Some(1000)),
+ (1, Some(3)),
+ (5, None),
+ (100, Some(2)),
+ (100, None),
+ (-3, None),
+ (-3, Some(2)),
+ (-100, Some(4)),
+ (-100, None),
+ ] {
+ let expected = substring(&utf8, start, length).unwrap();
+ let expected = expected.as_string::<i32>();
+ let actual = substring(&view, start, length).unwrap();
+ let actual = actual.as_string_view();
+ assert_eq!(
+ expected.iter().collect::<Vec<_>>(),
+ actual.iter().collect::<Vec<_>>(),
+ "start={start} length={length:?}"
+ );
+ }
+ }
+
+ #[test]
+ fn binary_view_matches_binary() {
+ let values: Vec<Option<&[u8]>> = vec![Some(b"hello world"), Some(b""),
None, Some(b"abc")];
+ let binary = BinaryArray::from(values.clone());
+ let view = BinaryViewArray::from(values);
+
+ for (start, length) in [
+ (0, None),
+ (0, Some(5)),
+ (2, Some(3)),
+ (-3, None),
+ (100, Some(2)),
+ ] {
+ let expected = substring(&binary, start, length).unwrap();
+ let expected = expected.as_binary::<i32>();
+ let actual = substring(&view, start, length).unwrap();
+ let actual = actual.as_binary_view();
+ assert_eq!(
+ expected.iter().collect::<Vec<_>>(),
+ actual.iter().collect::<Vec<_>>(),
+ "start={start} length={length:?}"
+ );
+ }
+ }
+
+ #[test]
+ fn string_view_rejects_an_invalid_char_boundary() {
Review Comment:
do we have a positive case, for slicing on a valid boundary that isnt only
ascii?
##########
arrow-string/src/substring.rs:
##########
@@ -1035,4 +1100,90 @@ mod tests {
let expected = BinaryArray::from(vec![Some(expected_bytes)]);
assert_eq!(expected, *actual);
}
+
+ /// The view result has to match the [`DataType::Utf8`] result for the
same input.
Review Comment:
```suggestion
```
goes without saying
##########
arrow-string/src/substring.rs:
##########
@@ -1035,4 +1100,90 @@ mod tests {
let expected = BinaryArray::from(vec![Some(expected_bytes)]);
assert_eq!(expected, *actual);
}
+
+ /// The view result has to match the [`DataType::Utf8`] result for the
same input.
+ #[test]
+ fn string_view_matches_utf8() {
+ let values = vec![
+ Some("hello world"),
+ Some(""),
+ None,
+ Some("a"),
+ Some("this one is definitely longer than twelve bytes"),
+ ];
+ let utf8 = StringArray::from(values.clone());
+ let view = StringViewArray::from(values);
+
+ for (start, length) in [
+ (0, None),
+ (0, Some(0)),
+ (0, Some(5)),
+ (0, Some(1000)),
+ (1, Some(3)),
+ (5, None),
+ (100, Some(2)),
+ (100, None),
+ (-3, None),
+ (-3, Some(2)),
+ (-100, Some(4)),
+ (-100, None),
+ ] {
+ let expected = substring(&utf8, start, length).unwrap();
+ let expected = expected.as_string::<i32>();
+ let actual = substring(&view, start, length).unwrap();
+ let actual = actual.as_string_view();
+ assert_eq!(
+ expected.iter().collect::<Vec<_>>(),
+ actual.iter().collect::<Vec<_>>(),
+ "start={start} length={length:?}"
+ );
+ }
+ }
+
+ #[test]
+ fn binary_view_matches_binary() {
+ let values: Vec<Option<&[u8]>> = vec![Some(b"hello world"), Some(b""),
None, Some(b"abc")];
Review Comment:
should add a long one here too
--
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]
