Add NullBuffer and BooleanBuffer From conversions #4380
Conversation
| /// // [1, 2, 3, 4] | ||
| /// let array = Int32Array::new(vec![1, 2, 3, 4].into(), None); | ||
| /// // [1, null, 3, 4] | ||
| /// let nulls = NullBuffer::from(vec![true, false, true, true]); |
There was a problem hiding this comment.
It is unfortunate that arrow went with the name null buffer, and then made true mean not null. But hey ho
There was a problem hiding this comment.
I agree -- it should be called the ValidBuffer or else have defined 1 to mean null. As you say 🤷
There was a problem hiding this comment.
BTW could this be
| /// let nulls = NullBuffer::from(vec![true, false, true, true]); | |
| /// let nulls = NullBuffer::from(&[true, false, true, true]); |
And potentially avoid the allocation? I see you added that From impl 🤔
There was a problem hiding this comment.
For From<&[bool]>, looks like it still does allocation.
There was a problem hiding this comment.
It would eliminate the allocation for the Vec, although the compiler might be smart enough to do that anyway, will update.
Edit this actually causes issues because it infers the type as &[bool; 4], will leave as vec
| } | ||
| } | ||
|
|
||
| impl From<&[bool]> for NullBuffer { |
There was a problem hiding this comment.
I debated adding custom conversion that would count the nulls, but I wasn't entirely convinced it would be faster - as the bit counting is very fast, so I left it for now. We can easily optimise later
| } | ||
| } | ||
|
|
||
| impl From<&[bool]> for NullBuffer { |
There was a problem hiding this comment.
I debated adding custom conversion that would count the nulls, but I wasn't entirely convinced it would be faster - as the bit counting is very fast, so I left it for now. We can easily optimise later
| /// // [1, 2, 3, 4] | ||
| /// let array = Int32Array::new(vec![1, 2, 3, 4].into(), None); | ||
| /// // [1, null, 3, 4] | ||
| /// let nulls = NullBuffer::from(vec![true, false, true, true]); |
There was a problem hiding this comment.
I agree -- it should be called the ValidBuffer or else have defined 1 to mean null. As you say 🤷
| /// | ||
| /// ``` |
There was a problem hiding this comment.
Giving a little context about when to use this constructor I think might help
| /// | |
| /// ``` | |
| /// | |
| /// # Example | |
| /// | |
| /// Creating a [`PrimitiveArray`] directly from a [`ScalarBuffer`] and [`NullBuffer`] ls generally | |
| /// the most performant approach and avoids any additional allocations. | |
| /// | |
| /// ``` |
| /// // [1, 2, 3, 4] | ||
| /// let array = Int32Array::new(vec![1, 2, 3, 4].into(), None); | ||
| /// // [1, null, 3, 4] | ||
| /// let nulls = NullBuffer::from(vec![true, false, true, true]); |
There was a problem hiding this comment.
BTW could this be
| /// let nulls = NullBuffer::from(vec![true, false, true, true]); | |
| /// let nulls = NullBuffer::from(&[true, false, true, true]); |
And potentially avoid the allocation? I see you added that From impl 🤔
Which issue does this PR close?
Closes #.
Rationale for this change
Follow up to #4379 and #4338
What changes are included in this PR?
Are there any user-facing changes?