mbrobbel commented on code in PR #8564:
URL: https://github.com/apache/arrow-rs/pull/8564#discussion_r2431551387
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -1458,14 +1458,21 @@ impl<T: ArrowPrimitiveType, Ptr:
Into<NativeAdapter<T>>> FromIterator<Ptr> for P
impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
/// Creates a [`PrimitiveArray`] from an iterator of trusted length.
+ ///
/// # Safety
+ ///
/// The iterator must be
[`TrustedLen`](https://doc.rust-lang.org/std/iter/trait.TrustedLen.html).
- /// I.e. that `size_hint().1` correctly reports its length.
+ /// I.e. that `size_hint().1` correctly reports its length. Note that this
is a stronger
+ /// guarantee than `ExactSizeIterator` provides, which could still report
a wrong length.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the iterator does not report an upper bound on `size_hint()`.
#[inline]
pub unsafe fn from_trusted_len_iter<I, P>(iter: I) -> Self
where
P: std::borrow::Borrow<Option<<T as ArrowPrimitiveType>::Native>>,
- I: IntoIterator<Item = P>,
+ I: ExactSizeIterator<Item = P>,
Review Comment:
We should use
[`TrustedLen`](https://doc.rust-lang.org/stable/std/iter/trait.TrustedLen.html),
but we can't because it's nightly-only. So the next best things is
[`ExactSizeIterator`](https://doc.rust-lang.org/stable/std/iter/trait.ExactSizeIterator.html)
for iterators that know their exact length. However there is a note in the
docs:
> Note that this trait is a safe trait and as such does not and cannot
guarantee that the returned length is correct. This means that unsafe code must
not rely on the correctness of
[Iterator::size_hint](https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.size_hint).
The unstable and unsafe
[TrustedLen](https://doc.rust-lang.org/stable/std/iter/trait.TrustedLen.html)
trait gives this additional guarantee.
Since this method is marked `unsafe` we require users to make sure their
iterators are `TrustedLen`. This means:
> The iterator reports a size hint where it is either exact (lower bound is
equal to upper bound), or the upper bound is
[None](https://doc.rust-lang.org/stable/std/option/enum.Option.html#variant.None).
The upper bound must only be
[None](https://doc.rust-lang.org/stable/std/option/enum.Option.html#variant.None)
if the actual iterator length is larger than
[usize::MAX](https://doc.rust-lang.org/stable/std/primitive.usize.html#associatedconstant.MAX).
In that case, the lower bound must be
[usize::MAX](https://doc.rust-lang.org/stable/std/primitive.usize.html#associatedconstant.MAX),
resulting in an
[Iterator::size_hint()](https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.size_hint)
of (usize::MAX, None).
The motivation to use `ExactSizeIterator` here (I think) is to use
`ExactSizeIterator::len` instead of `Iterator::size_hint`. This has the same
"can't be `None`" requirement on the upper bound reported by the size hint.
For the bound we can use `I: IntoIterator<Item = P, IntoIter:
ExactSizeIterator>`.
--
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]
