You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "XiangpengHao (via GitHub)" <gi...@apache.org> on 2024/06/11 14:29:14 UTC
[PR] zero copy cast dict array to view type arrays [arrow-rs]
XiangpengHao opened a new pull request, #5871:
URL: https://github.com/apache/arrow-rs/pull/5871
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Part of #5861 .
# Rationale for this change
Casting from `DictionaryArray` to `String/BinaryView` was previously handled by `unpack_dictionary`: https://github.com/apache/arrow-rs/blob/master/arrow-cast/src/cast/dictionary.rs#L93-L105
which incurs unnecessary copy to the value buffer. This pr handles Utf8View and BinaryView so that it will reuse the value buffer instead of creating a new one.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1635357225
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,97 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_values = dict_array.values();
+ let value_offsets = string_values.value_offsets();
+ let value_buffer = string_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+
+ // Safety:
+ // the buffer is from StringArray which is utf8.
+ let string_view = unsafe {
+ StringViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
Review Comment:
added now!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#issuecomment-2165201332
My plan is to merge this branch up to main, run the benchmarks added in https://github.com/apache/arrow-rs/pull/5874 on it and then assuming they look good merge it in
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#issuecomment-2165422279
The bechmark I ran suggests this kernel is almost 2x as fast as the existing approach (which makes sense as it avoids a copy).
Nice work @XiangpengHao
```
++ critcmp master view-to-dict
group master view-to-dict
----- ------ ------------
cast dict to string view 1.79 123.2±1.68µs ? ?/sec 1.00 68.8±2.76µs ? ?/sec
cast string view to dict 1.00 249.9±0.68µs ? ?/sec 1.07 268.3±0.53µs ? ?/sec
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#issuecomment-2161524589
BTW, with this pull request, casting a dictionary array to string view with 10_000 items improved performance by 25%
```
dict to view time: [37.979 µs 37.990 µs 38.001 µs]
change: [-25.271% -25.253% -25.237%] (p = 0.00 < 0.05)
Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
2 (2.00%) low mild
5 (5.00%) high mild
4 (4.00%) high severe
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1636611276
##########
arrow-array/src/builder/generic_bytes_view_builder.rs:
##########
@@ -139,22 +169,9 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> {
));
}
- if len <= 12 {
- let mut view_buffer = [0; 16];
- view_buffer[0..4].copy_from_slice(&len.to_le_bytes());
- view_buffer[4..4 + b.len()].copy_from_slice(b);
- self.views_builder.append(u128::from_le_bytes(view_buffer));
- } else {
- let view = ByteView {
- length: len,
- prefix: u32::from_le_bytes(b[0..4].try_into().unwrap()),
- buffer_index: block,
- offset,
- };
- self.views_builder.append(view.into());
+ unsafe {
+ self.append_view_unchecked(block, offset, len);
Review Comment:
💯
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1635424761
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,65 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_view = view_from_dict_values::<K, StringViewType, GenericStringType<i32>>(
+ dict_array.values(),
+ dict_array.keys(),
+ );
+ Ok(Arc::new(string_view))
+ }
+ BinaryView => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<BinaryArray>()
+ .unwrap();
+
+ let binary_view = view_from_dict_values::<K, BinaryViewType, BinaryType>(
+ dict_array.values(),
+ dict_array.keys(),
+ );
+ Ok(Arc::new(binary_view))
+ }
_ => unpack_dictionary::<K>(array, to_type, cast_options),
}
}
+fn view_from_dict_values<K: ArrowDictionaryKeyType, T: ByteViewType, V: ByteArrayType>(
+ array: &GenericByteArray<V>,
+ keys: &PrimitiveArray<K>,
+) -> GenericByteViewArray<T> {
+ let value_buffer = array.values();
+ let value_offsets = array.value_offsets();
+ let mut builder = GenericByteViewBuilder::<T>::with_capacity(keys.len());
+ builder.append_block(value_buffer.clone());
+ for i in keys.iter() {
Review Comment:
Another potential optimization is a separate loop if there are no nulls in keys (so we can avoid the branch)
Another potental idea is to use `value_offsets.windows(2)` as an iterator to avoid the bounds checks in `value_offsets`
However, I think we should merge this basic PR in as is, and then add a bencmark and optimize this kenrnel as a follow on PR (if we care). I can file a ticket if @tustvold agrees
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -5257,10 +5249,48 @@ mod tests {
let binary_view_array = cast(&binary_array, &DataType::BinaryView).unwrap();
assert_eq!(binary_view_array.data_type(), &DataType::BinaryView);
- let expect_binary_view_array = BinaryViewArray::from(data);
+ let expect_binary_view_array = BinaryViewArray::from_iter(VIEW_TEST_DATA);
assert_eq!(binary_view_array.as_ref(), &expect_binary_view_array);
}
+ #[test]
+ fn test_dict_to_view() {
+ let values = StringArray::from_iter(VIEW_TEST_DATA);
+ let keys = Int8Array::from_iter([Some(1), Some(0), None, Some(3), None, Some(1), Some(4)]);
+ let string_dict_array =
+ DictionaryArray::<Int8Type>::try_new(keys, Arc::new(values)).unwrap();
+ let typed_dict = string_dict_array.downcast_dict::<StringArray>().unwrap();
+
+ let string_view_array = {
+ let mut builder = StringViewBuilder::new().with_block_size(8); // multiple buffers.
Review Comment:
👍
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve performance of casting `DictionaryArray` to `StringViewArray` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1635419387
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,97 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_values = dict_array.values();
+ let value_offsets = string_values.value_offsets();
+ let value_buffer = string_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+
+ // Safety:
+ // the buffer is from StringArray which is utf8.
+ let string_view = unsafe {
+ StringViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
+ )
+ };
+ Ok(Arc::new(string_view))
+ }
+ BinaryView => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<BinaryArray>()
+ .unwrap();
+
+ let binary_values = dict_array.values();
+ let value_offsets = binary_values.value_offsets();
+ let value_buffer = binary_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+ let binary_view = unsafe {
+ BinaryViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
+ )
+ };
+ Ok(Arc::new(binary_view))
+ }
_ => unpack_dictionary::<K>(array, to_type, cast_options),
}
}
+fn view_from_dict_values<K: ArrowDictionaryKeyType>(
+ value_offsets: &[i32],
+ value_buffer: &arrow_buffer::Buffer,
+ keys: &PrimitiveArray<K>,
+) -> ScalarBuffer<u128> {
+ let mut view_builder = BufferBuilder::<u128>::new(keys.len());
+ for i in keys.iter() {
+ match i {
+ Some(v) => {
+ let idx = v.to_usize().unwrap();
+ let offset = value_offsets[idx];
+ let end = value_offsets[idx + 1];
+ let length = end - offset;
+ let value_buf = &value_buffer[offset as usize..end as usize];
+
+ if length <= 12 {
Review Comment:
Unfortunately, adding `append_view_unchecked` improved the performance by 50%
```
dict to view time: [38.116 µs 38.122 µs 38.127 µs]
change: [-50.618% -50.455% -50.290%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) low severe
4 (4.00%) low mild
1 (1.00%) high mild
2 (2.00%) high severe
```
I think it justifies adding `append_view_unchecked`, what do you think?
Also, should I add the benchmark to the repo? It's a bit tricky to setup two versions of the cast implementation..
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1636610336
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,97 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_values = dict_array.values();
+ let value_offsets = string_values.value_offsets();
+ let value_buffer = string_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+
+ // Safety:
+ // the buffer is from StringArray which is utf8.
+ let string_view = unsafe {
+ StringViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
+ )
+ };
+ Ok(Arc::new(string_view))
+ }
+ BinaryView => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<BinaryArray>()
+ .unwrap();
+
+ let binary_values = dict_array.values();
+ let value_offsets = binary_values.value_offsets();
+ let value_buffer = binary_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+ let binary_view = unsafe {
+ BinaryViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
+ )
+ };
+ Ok(Arc::new(binary_view))
+ }
_ => unpack_dictionary::<K>(array, to_type, cast_options),
}
}
+fn view_from_dict_values<K: ArrowDictionaryKeyType>(
+ value_offsets: &[i32],
+ value_buffer: &arrow_buffer::Buffer,
+ keys: &PrimitiveArray<K>,
+) -> ScalarBuffer<u128> {
+ let mut view_builder = BufferBuilder::<u128>::new(keys.len());
+ for i in keys.iter() {
+ match i {
+ Some(v) => {
+ let idx = v.to_usize().unwrap();
+ let offset = value_offsets[idx];
+ let end = value_offsets[idx + 1];
+ let length = end - offset;
+ let value_buf = &value_buffer[offset as usize..end as usize];
+
+ if length <= 12 {
Review Comment:
> I think it justifies adding append_view_unchecked, what do you think?
Makes sense to me
> Also, should I add the benchmark to the repo? It's a bit tricky to setup two versions of the cast implementation..
I do think we should add the benchmark to the repo (so we can use it for future optimizations)
In terms of justifying `append_view_unsafe` I think running the benchmark on a local checkout that calls `append_view` and then revert and run the same benchmark is fine (which is presumably what you did)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1635269725
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,97 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_values = dict_array.values();
+ let value_offsets = string_values.value_offsets();
+ let value_buffer = string_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+
+ // Safety:
+ // the buffer is from StringArray which is utf8.
+ let string_view = unsafe {
+ StringViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
Review Comment:
Nice catch! added a new test to cover this
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1635433312
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,65 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_view = view_from_dict_values::<K, StringViewType, GenericStringType<i32>>(
+ dict_array.values(),
+ dict_array.keys(),
+ );
+ Ok(Arc::new(string_view))
+ }
+ BinaryView => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<BinaryArray>()
+ .unwrap();
+
+ let binary_view = view_from_dict_values::<K, BinaryViewType, BinaryType>(
+ dict_array.values(),
+ dict_array.keys(),
+ );
+ Ok(Arc::new(binary_view))
+ }
_ => unpack_dictionary::<K>(array, to_type, cast_options),
}
}
+fn view_from_dict_values<K: ArrowDictionaryKeyType, T: ByteViewType, V: ByteArrayType>(
+ array: &GenericByteArray<V>,
+ keys: &PrimitiveArray<K>,
+) -> GenericByteViewArray<T> {
+ let value_buffer = array.values();
+ let value_offsets = array.value_offsets();
+ let mut builder = GenericByteViewBuilder::<T>::with_capacity(keys.len());
+ builder.append_block(value_buffer.clone());
+ for i in keys.iter() {
Review Comment:
ops, I checked in a `append_view_unchecked`.
But if we do `try_append_view`, it is even slower than the `unpack_dictionary` approach
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1635435370
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,65 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_view = view_from_dict_values::<K, StringViewType, GenericStringType<i32>>(
+ dict_array.values(),
+ dict_array.keys(),
+ );
+ Ok(Arc::new(string_view))
+ }
+ BinaryView => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<BinaryArray>()
+ .unwrap();
+
+ let binary_view = view_from_dict_values::<K, BinaryViewType, BinaryType>(
+ dict_array.values(),
+ dict_array.keys(),
+ );
+ Ok(Arc::new(binary_view))
+ }
_ => unpack_dictionary::<K>(array, to_type, cast_options),
}
}
+fn view_from_dict_values<K: ArrowDictionaryKeyType, T: ByteViewType, V: ByteArrayType>(
+ array: &GenericByteArray<V>,
+ keys: &PrimitiveArray<K>,
+) -> GenericByteViewArray<T> {
+ let value_buffer = array.values();
+ let value_offsets = array.value_offsets();
+ let mut builder = GenericByteViewBuilder::<T>::with_capacity(keys.len());
+ builder.append_block(value_buffer.clone());
+ for i in keys.iter() {
Review Comment:
I can also move `append_view_unchecked` as a follow-up PR and potentially clean up other use cases where ByteViews are manually constructed.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1635097217
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,97 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
Review Comment:
💯 for comments
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,97 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_values = dict_array.values();
+ let value_offsets = string_values.value_offsets();
+ let value_buffer = string_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+
+ // Safety:
+ // the buffer is from StringArray which is utf8.
+ let string_view = unsafe {
+ StringViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
Review Comment:
I think calling `nulls()` doesn't handle the case where the dictionary value itself (rather than the key) was null
https://github.com/apache/arrow-rs/blob/cf59b6cd826412635dc391d4cf0f9d8310f5a226/arrow-array/src/array/dictionary_array.rs#L727
I think this should call `logical_nulls()` instead: https://github.com/apache/arrow-rs/blob/cf59b6cd826412635dc391d4cf0f9d8310f5a226/arrow-array/src/array/dictionary_array.rs#L731
Also, it would be good to create a test case that covers this too
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -5257,10 +5249,30 @@ mod tests {
let binary_view_array = cast(&binary_array, &DataType::BinaryView).unwrap();
assert_eq!(binary_view_array.data_type(), &DataType::BinaryView);
- let expect_binary_view_array = BinaryViewArray::from(data);
+ let expect_binary_view_array = BinaryViewArray::from_iter(VIEW_TEST_DATA);
assert_eq!(binary_view_array.as_ref(), &expect_binary_view_array);
}
+ #[test]
+ fn test_dict_to_view() {
+ let string_view_array = StringViewArray::from_iter(VIEW_TEST_DATA);
+ let string_dict_array: DictionaryArray<Int8Type> = VIEW_TEST_DATA.into_iter().collect();
Review Comment:
Can you please update this test to have a dictionary array that has:
1. Repeated use of dictionary values
2. keys that are not all increasing
3. Nulls in the values (as well as the keys)
Perhaps what you can do is create a `StringArray` from `VIEW_TEST_DATA` and then make create the indexes manually
Like this example https://docs.rs/arrow/latest/arrow/array/struct.DictionaryArray.html#example-from-existing-arrays
Does that make sense?
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,97 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_values = dict_array.values();
+ let value_offsets = string_values.value_offsets();
+ let value_buffer = string_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+
+ // Safety:
+ // the buffer is from StringArray which is utf8.
+ let string_view = unsafe {
+ StringViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
+ )
+ };
+ Ok(Arc::new(string_view))
+ }
+ BinaryView => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<BinaryArray>()
+ .unwrap();
+
+ let binary_values = dict_array.values();
+ let value_offsets = binary_values.value_offsets();
+ let value_buffer = binary_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+ let binary_view = unsafe {
+ BinaryViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
+ )
+ };
+ Ok(Arc::new(binary_view))
+ }
_ => unpack_dictionary::<K>(array, to_type, cast_options),
}
}
+fn view_from_dict_values<K: ArrowDictionaryKeyType>(
+ value_offsets: &[i32],
+ value_buffer: &arrow_buffer::Buffer,
+ keys: &PrimitiveArray<K>,
+) -> ScalarBuffer<u128> {
+ let mut view_builder = BufferBuilder::<u128>::new(keys.len());
+ for i in keys.iter() {
+ match i {
+ Some(v) => {
+ let idx = v.to_usize().unwrap();
+ let offset = value_offsets[idx];
+ let end = value_offsets[idx + 1];
+ let length = end - offset;
+ let value_buf = &value_buffer[offset as usize..end as usize];
+
+ if length <= 12 {
Review Comment:
Instead of creating the views directly, what do you think about using `try_append_view` and `try_append_block` added in this PR: https://github.com/apache/arrow-rs/pull/5796
Maybe as bonus points you could add a benchmark for this casting operation, and see if adding `append_view_unchecked` would make any difference
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] zero copy cast dict array to view type arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5871:
URL: https://github.com/apache/arrow-rs/pull/5871#discussion_r1635319342
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -85,10 +85,97 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
Ok(new_array)
}
+ Utf8View => {
+ // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer.
+ // we handle it here to avoid the copy.
+ let dict_array = array
+ .as_dictionary::<K>()
+ .downcast_dict::<StringArray>()
+ .unwrap();
+
+ let string_values = dict_array.values();
+ let value_offsets = string_values.value_offsets();
+ let value_buffer = string_values.values().clone();
+
+ let view_buffer =
+ view_from_dict_values(value_offsets, &value_buffer, dict_array.keys());
+
+ // Safety:
+ // the buffer is from StringArray which is utf8.
+ let string_view = unsafe {
+ StringViewArray::new_unchecked(
+ view_buffer,
+ vec![value_buffer],
+ dict_array.nulls().cloned(),
Review Comment:
(fyi there seem to be no new commits to this PR)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org