-
Notifications
You must be signed in to change notification settings - Fork 855
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
Cleanup length and bit_length kernels #4718
Conversation
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 few minor nits, but otherwise this looks like a really nice cleanup @tustvold
Less unsafe and perhaps faster 👍
nulls: Option<&NullBuffer>, | ||
) -> ArrayRef { | ||
let v: Vec<_> = offsets | ||
.windows(2) |
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 is very clever 👍
let list = array.as_binary::<i64>(); | ||
Ok(bit_length_impl::<Int64Type>(list.offsets(), list.nulls())) | ||
} | ||
DataType::FixedSizeBinary(len) => Ok(Arc::new(Int32Array::new( |
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.
the support for FixedSizeBinary seems to be new here but there is no test update -- maybe we can add a test for it?
arrow-string/src/length.rs
Outdated
other => Err(ArrowError::ComputeError(format!( | ||
"bit_length not supported for {other:?}" | ||
"length not supported for {other:?}" |
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.
I think the prior message was correct as this is the bit_length
implementation 🤔
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.
Yeah this was a copypasta
arrow-string/src/length.rs
Outdated
other => Err(ArrowError::ComputeError(format!( | ||
"bit_length not supported for {other:?}" | ||
"length not supported for {other:?}" |
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.
"length not supported for {other:?}" | |
"bit_length not supported for {other:?}" |
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?