Jefffrey commented on code in PR #10436:
URL: https://github.com/apache/arrow-rs/pull/10436#discussion_r3789570998
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -34,23 +34,126 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
(_, Dictionary(to_index_type, to_value_type)) => {
dictionary_to_dictionary_cast(array, to_index_type, to_value_type,
cast_options)
}
- // `unpack_dictionary` can handle Utf8View/BinaryView types, but
incurs unnecessary data
- // copy of the value buffer. Fast path which avoids copying underlying
values buffer.
- // TODO: handle LargeUtf8/LargeBinary -> View (need to check offsets
can fit)
- // TODO: handle cross types (String -> BinaryView, Binary ->
StringView)
- // (need to validate utf8?)
- (Utf8, Utf8View) => view_from_dict_values::<K, Utf8Type,
StringViewType>(
- array.keys(),
- array.values().as_string::<i32>(),
- ),
- (Binary, BinaryView) => view_from_dict_values::<K, BinaryType,
BinaryViewType>(
+ // There are two ways to produce a view array from a dictionary, and
neither of them
+ // copies the values buffer:
+ //
+ // (a) `unpack_dictionary` builds one view per *dictionary value* and
then uses `take`,
+ // which gathers 16-byte views and passes the data buffers
through untouched.
+ // (b) `view_from_dict_values` builds one view per *row* directly
against the values
+ // buffer, skipping the intermediate array entirely.
+ //
+ // (a) is memory-bandwidth-bound -- the gather in `take` benchmarks at
or below the cost
+ // of a hand-written `u128` gather -- so it wins once rows reach
roughly 0.6x the
+ // dictionary size. (b) reads the payload bytes of every row out of
the values buffer to
+ // build each view, which is far more expensive per row, and only pays
off when the
+ // dictionary is substantially larger than the array is long: there
(a) spends most of
+ // its time building views that no row ever references.
+ //
+ // So take (b) only when it actually wins. Everything else falls
through to (a) below.
Review Comment:
```suggestion
// `unpack_dictionary` operates per dictionary value before using
take kernel to form
// final view. `view_from_dict_values` builds output view directly
per row (key index).
// Based on benchmarking, `view_from_dict_values` is more efficient
for sparse dictionaries
// (more dictionary values than there are rows/keys), whilst
`unpack_dictionary` is
// more efficient for dense dictionaries, where a sparse dictionary
is when rows reach
// roughly 0.6x the dictionary size.
//
// Therefore delegate to the faster method based on the density of
the input dictionary.
```
My attempt at cutting down the verbosity a bit, feel free to edit or make
suggestions if it doesn't seem as clear or is inaccurate at bits
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -126,6 +229,73 @@ fn dictionary_to_dictionary_cast<K:
ArrowDictionaryKeyType>(
Ok(new_array)
}
+/// Cast `Dict<K, Binary>` or `Dict<K, LargeBinary>` to `Utf8View`, validating
UTF-8 for each
+/// dictionary value.
+///
+/// Fast path when all values are valid UTF-8: reuses the values buffer
without copying.
+/// When some values are invalid and `cast_options.safe` is true, rows
pointing to those
+/// values become null. When `cast_options.safe` is false, returns an error
immediately.
+fn binary_dict_to_string_view<K: ArrowDictionaryKeyType, O: OffsetSizeTrait>(
+ keys: &PrimitiveArray<K>,
+ values: &GenericByteArray<GenericBinaryType<O>>,
+ cast_options: &CastOptions,
+) -> Result<ArrayRef, ArrowError> {
+ match GenericStringArray::<O>::try_from_binary(values.clone()) {
+ Ok(_) => {
+ // All dictionary values are valid UTF-8: reuse the buffer
zero-copy.
+ view_from_dict_values::<K, GenericBinaryType<O>,
StringViewType>(keys, values)
+ }
+ Err(e) => {
+ if !cast_options.safe {
+ return Err(e);
Review Comment:
do we need to consider if potentially invalid utf8 is actually masked out by
the null buffer of the byte array? this could be overly restrictive for such
niche inputs
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -34,23 +34,126 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
(_, Dictionary(to_index_type, to_value_type)) => {
dictionary_to_dictionary_cast(array, to_index_type, to_value_type,
cast_options)
}
- // `unpack_dictionary` can handle Utf8View/BinaryView types, but
incurs unnecessary data
- // copy of the value buffer. Fast path which avoids copying underlying
values buffer.
- // TODO: handle LargeUtf8/LargeBinary -> View (need to check offsets
can fit)
- // TODO: handle cross types (String -> BinaryView, Binary ->
StringView)
- // (need to validate utf8?)
- (Utf8, Utf8View) => view_from_dict_values::<K, Utf8Type,
StringViewType>(
- array.keys(),
- array.values().as_string::<i32>(),
- ),
- (Binary, BinaryView) => view_from_dict_values::<K, BinaryType,
BinaryViewType>(
+ // There are two ways to produce a view array from a dictionary, and
neither of them
+ // copies the values buffer:
+ //
+ // (a) `unpack_dictionary` builds one view per *dictionary value* and
then uses `take`,
+ // which gathers 16-byte views and passes the data buffers
through untouched.
+ // (b) `view_from_dict_values` builds one view per *row* directly
against the values
+ // buffer, skipping the intermediate array entirely.
+ //
+ // (a) is memory-bandwidth-bound -- the gather in `take` benchmarks at
or below the cost
+ // of a hand-written `u128` gather -- so it wins once rows reach
roughly 0.6x the
+ // dictionary size. (b) reads the payload bytes of every row out of
the values buffer to
+ // build each view, which is far more expensive per row, and only pays
off when the
+ // dictionary is substantially larger than the array is long: there
(a) spends most of
+ // its time building views that no row ever references.
+ //
+ // So take (b) only when it actually wins. Everything else falls
through to (a) below.
+ (Utf8, Utf8View) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, Utf8Type, StringViewType>(
+ array.keys(),
+ array.values().as_string::<i32>(),
+ )
+ }
+ (Binary, BinaryView) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, BinaryType, BinaryViewType>(
+ array.keys(),
+ array.values().as_binary::<i32>(),
+ )
+ }
+ // LargeUtf8/LargeBinary additionally require a values buffer small
enough to be addressed
+ // by the u32 offset of a view.
+ (LargeUtf8, Utf8View)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_string::<i64>()) =>
+ {
+ view_from_dict_values::<K, LargeUtf8Type, StringViewType>(
+ array.keys(),
+ array.values().as_string::<i64>(),
+ )
+ }
+ (LargeBinary, BinaryView)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_binary::<i64>()) =>
+ {
+ view_from_dict_values::<K, LargeBinaryType, BinaryViewType>(
+ array.keys(),
+ array.values().as_binary::<i64>(),
+ )
+ }
+ // Cross casts to a binary view need no validation: valid UTF-8 is
valid binary.
+ (Utf8, BinaryView) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, Utf8Type, BinaryViewType>(
+ array.keys(),
+ array.values().as_string::<i32>(),
+ )
+ }
+ (LargeUtf8, BinaryView)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_string::<i64>()) =>
+ {
+ view_from_dict_values::<K, LargeUtf8Type, BinaryViewType>(
+ array.keys(),
+ array.values().as_string::<i64>(),
+ )
+ }
+ // Cross casts to a string view require UTF-8 validation of the
dictionary values.
+ (Binary, Utf8View) if prefer_direct_views(array) =>
binary_dict_to_string_view::<K, i32>(
array.keys(),
array.values().as_binary::<i32>(),
+ cast_options,
),
+ (LargeBinary, Utf8View)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_binary::<i64>()) =>
+ {
+ binary_dict_to_string_view::<K, i64>(
+ array.keys(),
+ array.values().as_binary::<i64>(),
+ cast_options,
+ )
+ }
_ => unpack_dictionary(array, to_type, cast_options),
}
}
+/// Whether building views per row beats `unpack_dictionary` for this array.
+///
+/// `unpack_dictionary` costs `O(values)` to build the intermediate view array
plus `O(keys)` for
+/// a bandwidth-bound gather; building views directly costs `O(keys)` but with
a much larger
+/// constant, since every row has to read its payload out of the values
buffer. The direct path
+/// therefore only wins when the dictionary is substantially larger than the
array is long.
+///
+/// The measured crossover sits near `keys ~= 0.6 * values`, so this
deliberately switches short
+/// of it rather than at the crossover itself: at `keys * 2 == values` the
direct path is still
+/// ahead by 15-30%, which leaves margin for the exact crossover moving with
cache size or
+/// microarchitecture. Being wrong in this direction merely forgoes a win;
being wrong the other
+/// way is a large regression on the common case.
+#[inline]
+fn prefer_direct_views<K: ArrowDictionaryKeyType>(array: &DictionaryArray<K>)
-> bool {
Review Comment:
```suggestion
#[inline]
fn is_sparse<K: ArrowDictionaryKeyType>(array: &DictionaryArray<K>) -> bool {
```
I feel the doc comment is too verbose and repeats what is said above; maybe
we can move this method as an inner method to `dictionary_cast` to try colocate
the comments to avoid redundancy
also a name like `is_sparse` to me reads better than `prefer_direct_views`
because we dont exactly know what `direct_views` means here
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -7639,6 +7639,316 @@ mod tests {
assert_eq!(casted_binary_array.as_ref(), &binary_view_array);
}
+ /// Casting a dictionary to a view type has two implementations: building
one view per row
+ /// directly against the values buffer, and `unpack_dictionary`. Which one
runs depends on
+ /// how the row count compares to the dictionary size, so these helpers
pin both branches of
+ /// that choice for each arm.
+ ///
+ /// `values` must have 6 entries; the returned key sets sit either side of
the threshold.
+ fn keys_taking_direct_path() -> Int32Array {
+ // 2 keys < 6/2 values -> views are built directly per row
+ Int32Array::from_iter([Some(0), Some(3)])
+ }
Review Comment:
it might be better to move this test code into cast/dictionary.rs file, so
it lives closer together
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -34,23 +34,126 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
(_, Dictionary(to_index_type, to_value_type)) => {
dictionary_to_dictionary_cast(array, to_index_type, to_value_type,
cast_options)
}
- // `unpack_dictionary` can handle Utf8View/BinaryView types, but
incurs unnecessary data
- // copy of the value buffer. Fast path which avoids copying underlying
values buffer.
- // TODO: handle LargeUtf8/LargeBinary -> View (need to check offsets
can fit)
- // TODO: handle cross types (String -> BinaryView, Binary ->
StringView)
- // (need to validate utf8?)
- (Utf8, Utf8View) => view_from_dict_values::<K, Utf8Type,
StringViewType>(
- array.keys(),
- array.values().as_string::<i32>(),
- ),
- (Binary, BinaryView) => view_from_dict_values::<K, BinaryType,
BinaryViewType>(
+ // There are two ways to produce a view array from a dictionary, and
neither of them
+ // copies the values buffer:
+ //
+ // (a) `unpack_dictionary` builds one view per *dictionary value* and
then uses `take`,
+ // which gathers 16-byte views and passes the data buffers
through untouched.
+ // (b) `view_from_dict_values` builds one view per *row* directly
against the values
+ // buffer, skipping the intermediate array entirely.
+ //
+ // (a) is memory-bandwidth-bound -- the gather in `take` benchmarks at
or below the cost
+ // of a hand-written `u128` gather -- so it wins once rows reach
roughly 0.6x the
+ // dictionary size. (b) reads the payload bytes of every row out of
the values buffer to
+ // build each view, which is far more expensive per row, and only pays
off when the
+ // dictionary is substantially larger than the array is long: there
(a) spends most of
+ // its time building views that no row ever references.
+ //
+ // So take (b) only when it actually wins. Everything else falls
through to (a) below.
+ (Utf8, Utf8View) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, Utf8Type, StringViewType>(
+ array.keys(),
+ array.values().as_string::<i32>(),
+ )
+ }
+ (Binary, BinaryView) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, BinaryType, BinaryViewType>(
+ array.keys(),
+ array.values().as_binary::<i32>(),
+ )
+ }
+ // LargeUtf8/LargeBinary additionally require a values buffer small
enough to be addressed
+ // by the u32 offset of a view.
+ (LargeUtf8, Utf8View)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_string::<i64>()) =>
+ {
+ view_from_dict_values::<K, LargeUtf8Type, StringViewType>(
+ array.keys(),
+ array.values().as_string::<i64>(),
+ )
+ }
+ (LargeBinary, BinaryView)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_binary::<i64>()) =>
+ {
+ view_from_dict_values::<K, LargeBinaryType, BinaryViewType>(
+ array.keys(),
+ array.values().as_binary::<i64>(),
+ )
+ }
+ // Cross casts to a binary view need no validation: valid UTF-8 is
valid binary.
+ (Utf8, BinaryView) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, Utf8Type, BinaryViewType>(
+ array.keys(),
+ array.values().as_string::<i32>(),
+ )
+ }
+ (LargeUtf8, BinaryView)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_string::<i64>()) =>
+ {
+ view_from_dict_values::<K, LargeUtf8Type, BinaryViewType>(
+ array.keys(),
+ array.values().as_string::<i64>(),
+ )
+ }
+ // Cross casts to a string view require UTF-8 validation of the
dictionary values.
+ (Binary, Utf8View) if prefer_direct_views(array) =>
binary_dict_to_string_view::<K, i32>(
array.keys(),
array.values().as_binary::<i32>(),
+ cast_options,
),
+ (LargeBinary, Utf8View)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_binary::<i64>()) =>
+ {
+ binary_dict_to_string_view::<K, i64>(
+ array.keys(),
+ array.values().as_binary::<i64>(),
+ cast_options,
+ )
+ }
_ => unpack_dictionary(array, to_type, cast_options),
}
}
+/// Whether building views per row beats `unpack_dictionary` for this array.
+///
+/// `unpack_dictionary` costs `O(values)` to build the intermediate view array
plus `O(keys)` for
+/// a bandwidth-bound gather; building views directly costs `O(keys)` but with
a much larger
+/// constant, since every row has to read its payload out of the values
buffer. The direct path
+/// therefore only wins when the dictionary is substantially larger than the
array is long.
+///
+/// The measured crossover sits near `keys ~= 0.6 * values`, so this
deliberately switches short
+/// of it rather than at the crossover itself: at `keys * 2 == values` the
direct path is still
+/// ahead by 15-30%, which leaves margin for the exact crossover moving with
cache size or
+/// microarchitecture. Being wrong in this direction merely forgoes a win;
being wrong the other
+/// way is a large regression on the common case.
+#[inline]
+fn prefer_direct_views<K: ArrowDictionaryKeyType>(array: &DictionaryArray<K>)
-> bool {
+ array.keys().len() < array.values().len() / 2
+}
+
+/// Whether the values buffer can back a view array, i.e. it is smaller than
4GiB.
+///
+/// Required because views address their data with a `u32` offset, and
+/// [`GenericByteViewBuilder::append_block`] asserts the block it is handed is
smaller than
+/// `u32::MAX`. Failing this check is therefore a panic in the direct path,
not an error.
+///
+/// `unpack_dictionary` has no such limit: it reaches
+/// `impl From<&GenericByteArray> for GenericByteViewArray`, which makes the
same test and falls
+/// back to copying via `from_iter` when the buffer is too large. So this
guards the direct path
+/// only, and the fallback is strictly more capable rather than an equivalent
failure.
+///
+/// This deliberately measures the whole buffer rather than the largest live
offset: slicing a
+/// byte array slices its offsets but keeps `value_data` intact, so a slice
can have small offsets
+/// and still carry an oversized buffer -- and it is the buffer that
`append_block` rejects.
+#[inline]
+fn values_buffer_fits_in_view<T: ByteArrayType>(values: &GenericByteArray<T>)
-> bool {
+ values.values().len() < u32::MAX as usize
Review Comment:
```suggestion
values.values().len() < i32::MAX as usize
```
- https://github.com/apache/arrow-rs/issues/6172
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -34,23 +34,126 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
(_, Dictionary(to_index_type, to_value_type)) => {
dictionary_to_dictionary_cast(array, to_index_type, to_value_type,
cast_options)
}
- // `unpack_dictionary` can handle Utf8View/BinaryView types, but
incurs unnecessary data
- // copy of the value buffer. Fast path which avoids copying underlying
values buffer.
- // TODO: handle LargeUtf8/LargeBinary -> View (need to check offsets
can fit)
- // TODO: handle cross types (String -> BinaryView, Binary ->
StringView)
- // (need to validate utf8?)
- (Utf8, Utf8View) => view_from_dict_values::<K, Utf8Type,
StringViewType>(
- array.keys(),
- array.values().as_string::<i32>(),
- ),
- (Binary, BinaryView) => view_from_dict_values::<K, BinaryType,
BinaryViewType>(
+ // There are two ways to produce a view array from a dictionary, and
neither of them
+ // copies the values buffer:
+ //
+ // (a) `unpack_dictionary` builds one view per *dictionary value* and
then uses `take`,
+ // which gathers 16-byte views and passes the data buffers
through untouched.
+ // (b) `view_from_dict_values` builds one view per *row* directly
against the values
+ // buffer, skipping the intermediate array entirely.
+ //
+ // (a) is memory-bandwidth-bound -- the gather in `take` benchmarks at
or below the cost
+ // of a hand-written `u128` gather -- so it wins once rows reach
roughly 0.6x the
+ // dictionary size. (b) reads the payload bytes of every row out of
the values buffer to
+ // build each view, which is far more expensive per row, and only pays
off when the
+ // dictionary is substantially larger than the array is long: there
(a) spends most of
+ // its time building views that no row ever references.
+ //
+ // So take (b) only when it actually wins. Everything else falls
through to (a) below.
+ (Utf8, Utf8View) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, Utf8Type, StringViewType>(
+ array.keys(),
+ array.values().as_string::<i32>(),
+ )
+ }
+ (Binary, BinaryView) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, BinaryType, BinaryViewType>(
+ array.keys(),
+ array.values().as_binary::<i32>(),
+ )
+ }
+ // LargeUtf8/LargeBinary additionally require a values buffer small
enough to be addressed
+ // by the u32 offset of a view.
Review Comment:
```suggestion
// `view_from_dict_values` directly appends the values buffer as a
block using
// `GenericByteViewBuilder::append_block` which asserts length of
the buffer; we must
// ensure this assertion holds to use it for large variants which
may exceed the max allowable length.
// If we exceed the length we can simply fallback to
`unpack_dictionary` which still builds it
// correctly.
```
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -34,23 +34,126 @@ pub(crate) fn dictionary_cast<K: ArrowDictionaryKeyType>(
(_, Dictionary(to_index_type, to_value_type)) => {
dictionary_to_dictionary_cast(array, to_index_type, to_value_type,
cast_options)
}
- // `unpack_dictionary` can handle Utf8View/BinaryView types, but
incurs unnecessary data
- // copy of the value buffer. Fast path which avoids copying underlying
values buffer.
- // TODO: handle LargeUtf8/LargeBinary -> View (need to check offsets
can fit)
- // TODO: handle cross types (String -> BinaryView, Binary ->
StringView)
- // (need to validate utf8?)
- (Utf8, Utf8View) => view_from_dict_values::<K, Utf8Type,
StringViewType>(
- array.keys(),
- array.values().as_string::<i32>(),
- ),
- (Binary, BinaryView) => view_from_dict_values::<K, BinaryType,
BinaryViewType>(
+ // There are two ways to produce a view array from a dictionary, and
neither of them
+ // copies the values buffer:
+ //
+ // (a) `unpack_dictionary` builds one view per *dictionary value* and
then uses `take`,
+ // which gathers 16-byte views and passes the data buffers
through untouched.
+ // (b) `view_from_dict_values` builds one view per *row* directly
against the values
+ // buffer, skipping the intermediate array entirely.
+ //
+ // (a) is memory-bandwidth-bound -- the gather in `take` benchmarks at
or below the cost
+ // of a hand-written `u128` gather -- so it wins once rows reach
roughly 0.6x the
+ // dictionary size. (b) reads the payload bytes of every row out of
the values buffer to
+ // build each view, which is far more expensive per row, and only pays
off when the
+ // dictionary is substantially larger than the array is long: there
(a) spends most of
+ // its time building views that no row ever references.
+ //
+ // So take (b) only when it actually wins. Everything else falls
through to (a) below.
+ (Utf8, Utf8View) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, Utf8Type, StringViewType>(
+ array.keys(),
+ array.values().as_string::<i32>(),
+ )
+ }
+ (Binary, BinaryView) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, BinaryType, BinaryViewType>(
+ array.keys(),
+ array.values().as_binary::<i32>(),
+ )
+ }
+ // LargeUtf8/LargeBinary additionally require a values buffer small
enough to be addressed
+ // by the u32 offset of a view.
+ (LargeUtf8, Utf8View)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_string::<i64>()) =>
+ {
+ view_from_dict_values::<K, LargeUtf8Type, StringViewType>(
+ array.keys(),
+ array.values().as_string::<i64>(),
+ )
+ }
+ (LargeBinary, BinaryView)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_binary::<i64>()) =>
+ {
+ view_from_dict_values::<K, LargeBinaryType, BinaryViewType>(
+ array.keys(),
+ array.values().as_binary::<i64>(),
+ )
+ }
+ // Cross casts to a binary view need no validation: valid UTF-8 is
valid binary.
+ (Utf8, BinaryView) if prefer_direct_views(array) => {
+ view_from_dict_values::<K, Utf8Type, BinaryViewType>(
+ array.keys(),
+ array.values().as_string::<i32>(),
+ )
+ }
+ (LargeUtf8, BinaryView)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_string::<i64>()) =>
+ {
+ view_from_dict_values::<K, LargeUtf8Type, BinaryViewType>(
+ array.keys(),
+ array.values().as_string::<i64>(),
+ )
+ }
+ // Cross casts to a string view require UTF-8 validation of the
dictionary values.
+ (Binary, Utf8View) if prefer_direct_views(array) =>
binary_dict_to_string_view::<K, i32>(
array.keys(),
array.values().as_binary::<i32>(),
+ cast_options,
),
+ (LargeBinary, Utf8View)
+ if prefer_direct_views(array)
+ &&
values_buffer_fits_in_view(array.values().as_binary::<i64>()) =>
+ {
+ binary_dict_to_string_view::<K, i64>(
+ array.keys(),
+ array.values().as_binary::<i64>(),
+ cast_options,
+ )
+ }
_ => unpack_dictionary(array, to_type, cast_options),
}
}
+/// Whether building views per row beats `unpack_dictionary` for this array.
+///
+/// `unpack_dictionary` costs `O(values)` to build the intermediate view array
plus `O(keys)` for
+/// a bandwidth-bound gather; building views directly costs `O(keys)` but with
a much larger
+/// constant, since every row has to read its payload out of the values
buffer. The direct path
+/// therefore only wins when the dictionary is substantially larger than the
array is long.
+///
+/// The measured crossover sits near `keys ~= 0.6 * values`, so this
deliberately switches short
+/// of it rather than at the crossover itself: at `keys * 2 == values` the
direct path is still
+/// ahead by 15-30%, which leaves margin for the exact crossover moving with
cache size or
+/// microarchitecture. Being wrong in this direction merely forgoes a win;
being wrong the other
+/// way is a large regression on the common case.
+#[inline]
+fn prefer_direct_views<K: ArrowDictionaryKeyType>(array: &DictionaryArray<K>)
-> bool {
+ array.keys().len() < array.values().len() / 2
+}
+
+/// Whether the values buffer can back a view array, i.e. it is smaller than
4GiB.
+///
+/// Required because views address their data with a `u32` offset, and
+/// [`GenericByteViewBuilder::append_block`] asserts the block it is handed is
smaller than
+/// `u32::MAX`. Failing this check is therefore a panic in the direct path,
not an error.
+///
+/// `unpack_dictionary` has no such limit: it reaches
+/// `impl From<&GenericByteArray> for GenericByteViewArray`, which makes the
same test and falls
+/// back to copying via `from_iter` when the buffer is too large. So this
guards the direct path
+/// only, and the fallback is strictly more capable rather than an equivalent
failure.
+///
+/// This deliberately measures the whole buffer rather than the largest live
offset: slicing a
+/// byte array slices its offsets but keeps `value_data` intact, so a slice
can have small offsets
+/// and still carry an oversized buffer -- and it is the buffer that
`append_block` rejects.
+#[inline]
Review Comment:
```suggestion
```
same here, dont need to repeat here, easier to just keep it within the
actual match arms above per my previous 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
