-
Notifications
You must be signed in to change notification settings - Fork 847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use BooleanBuffer in BooleanArray (#3879) #3895
Conversation
|
||
let array_data = ArrayData::builder(DataType::Boolean) | ||
.len(filter.len()) | ||
.add_buffer(new_mask); | ||
.len(mask.len()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A follow up PR will add a BooleanArray::new
method to encapsulate this logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a very nice improvement to me.
cc @jhorstmann and @viirya
I think we should give this one some time to marinate / gather feedback
buffer_unary_not(left.values(), left.offset(), left.len()) | ||
let values = match right { | ||
true => left.values().clone(), | ||
false => !left.values(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is fancy 👍
@@ -96,19 +96,19 @@ impl BooleanArray { | |||
BooleanBuilder::with_capacity(capacity) | |||
} | |||
|
|||
/// Returns a `Buffer` holding all the values of this array. | |||
/// Returns the underlying [`BooleanBuffer`] holding all the values of this array. | |||
/// | |||
/// Note this doesn't take the offset of this array into account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this doesn't take the offset of this array into account.
This is still true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot, whilst technically still true, it is rather misleading will remove
* Use BooleanBuffer in BooleanArray (apache#3879) * Review feedback
Which issue does this PR close?
Part of #3879
Rationale for this change
Continues the process of making arrays wrappers around strongly typed buffer abstractions.
What changes are included in this PR?
Are there any user-facing changes?
Yes, it changes the return type of
BooleanArray::values
. I opted to just break this API instead of deprecating, as I wanted to keep the name the same, and the method is rarely used in downstreams, DataFusion doesn't call it at all, likely because it is hard to use correctly (something this PR improves).