-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
ARROW-10448: [Rust] Remove PrimitiveArray::new that can cause UB #8560
Conversation
I think we have a similar problem with building arrays from So something like the below gets created, but ends up as a 0-len array // try build array data without specifying length
ArrayData::builder(DataType::_).buffers(vec![buffer1, buffer2]).build() |
That is a very good point. I think that we have a trade-off here: do we check that the This could be a good topic for the mailing list, as it is a general question, specially for IPC and the c data interface. |
Is it the case that "UB" means "Undefined Behavior"? |
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.
Looks like a good improvement to me. 👍 It also makes it clearer to people how to construct arrays (aka use the Builders
) rather than also potentially using PrimitiveArray::new
directly
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.
LGTM
This PR removes `PrimitiveArray::new`. `PrimitiveArray::new` is `pub`, but it is dangerous because: * when the buffer's content is not aligned with T, many of its methods cause UB * when used with `null_count > 0`, many calls panic as the null bitmap is `None`, but the `null_count != 0` * when used with `null_count > 0`, it creates an array out of spec (as the buffer for the null bitmap is `None` but the null count is not zero) Since: * a change in this method's signature (to either add the bitmap or remove `null_count`) requires a backward incompatible change * it is only used in tests * we have good offers to create primitive arrays: * from an ArrayData, * from a vector or vector of optionals * from an iterator This PR removes it. Closes apache#8560 from jorgecarleitao/remove_new Authored-by: Jorge C. Leitao <jorgecarleitao@gmail.com> Signed-off-by: Neville Dipale <nevilledips@gmail.com>
This PR removes
PrimitiveArray::new
.PrimitiveArray::new
ispub
, but it is dangerous because:null_count > 0
, many calls panic as the null bitmap isNone
, but thenull_count != 0
null_count > 0
, it creates an array out of spec (as the buffer for the null bitmap isNone
but the null count is not zero)Since:
null_count
) requires a backward incompatible changeThis PR removes it.