Add ValuesIter to iterate defined values in ArrayAccessor (#2578) #2579
Conversation
| unsafe { Some(Some(self.array.value_unchecked(old))) } | ||
| } | ||
| } | ||
|
|
||
| fn size_hint(&self) -> (usize, Option<usize>) { | ||
| ( | ||
| self.array.len() - self.current, | ||
| Some(self.array.len() - self.current), | ||
| self.current_end - self.current, |
There was a problem hiding this comment.
Drive by fix, the DoubleEndedIterator size hint previously would be incorrect
| /// the key at a null index is unspecified, and consequently may exceed the bounds of the | ||
| /// values array. | ||
| /// | ||
| const NULLS_DEFINED: bool; |
There was a problem hiding this comment.
I don't especially like that we end up polluting everything just because dictionaries have strange null behaviour...
| @@ -307,9 +307,8 @@ fn cast_primitive_to_decimal<T: ArrayAccessor, F>( | |||
| where | |||
| F: Fn(T::Item) -> i128, | |||
| { | |||
| #[allow(clippy::redundant_closure)] | |||
There was a problem hiding this comment.
I ran into this issue also, and was similarly baffled by it. rust-lang/rust-clippy#1608 provided the solution, so I fixed up the other callsites
| // Dictionary indexes should be valid | ||
| unsafe { self.values.value_unchecked(value_idx) } | ||
| } | ||
| const NULLS_DEFINED: bool = false; | ||
|
|
||
| unsafe fn value_unchecked(&self, index: usize) -> Self::Item { |
There was a problem hiding this comment.
Hmm, I think the problematic one is value_unchecked, no? value always checks is_valid of the key.
But in value_unchecked, the key is retrieved by value_unchecked so it could be unspecified and cause undefined value from values?
There was a problem hiding this comment.
I just moved value to be default implemented, I will revert as the panic messages are better this way
| let comparison = | ||
| ValuesIter::new(left) | ||
| .zip(ValuesIter::new(right)) | ||
| .map(|(a, b)| match (a, b) { | ||
| (Some(a), Some(b)) => op(a, b), | ||
| _ => false, | ||
| }); | ||
| // SAFETY: ValuesIter is a trusted length iterator |
There was a problem hiding this comment.
Not sure how much this impacts performance.
|
This represents a non-trivial performance regression currently 😢 |
|
I'm going to handle this differently |
Draft as I need to double check this doesn't regress benchmarks and I'm not a massive fan of this
Which issue does this PR close?
Closes #2578
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?