Skip to content
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-2398: [Rust] Create Builder<T> for building buffers directly in aligned memory #1838

Closed
wants to merge 9 commits into from

Conversation

@andygrove
Copy link
Member

andygrove commented Apr 5, 2018

Also adds our first example

@andygrove andygrove changed the title Create Builder<T> for building buffers with zero-copy on build ARROW-2398: [Rust] Create Builder<T> for building buffers directly in aligned memory Apr 5, 2018
@andygrove

This comment has been minimized.

Copy link
Member Author

andygrove commented Apr 5, 2018

rustfmt has stopped working on my system, so CI will probably fail until I fix that.

andygrove added 3 commits Apr 5, 2018
}

/// Build a Buffer from the existing memory
pub fn build(&mut self) -> Buffer<T> {

This comment has been minimized.

Copy link
@xhochy

xhochy Apr 5, 2018

Member

Is there a reason that this is called build? In C++ we call this Finish and it would be nice to have them consistent. If build is a typical name used in Rust, it's probably better to keep it as is.

@andygrove

This comment has been minimized.

Copy link
Member Author

andygrove commented Apr 5, 2018

@xhochy I renamed it to finish(). It's best to keep it consistent within the project.

@andygrove

This comment has been minimized.

Copy link
Member Author

andygrove commented Apr 5, 2018

Moved builder into separate file for consistency with C++. Also implemented Drop for builder (in case builder was created but finish was not called).

@pitrou

This comment has been minimized.

Copy link
Contributor

pitrou commented Apr 5, 2018

@andygrove I think you forgot to add buffer.rs.

@andygrove

This comment has been minimized.

Copy link
Member Author

andygrove commented Apr 5, 2018

Oops, added. Also ran rustfmt again.

@max-sixty

This comment has been minimized.

Copy link
Contributor

max-sixty commented Apr 5, 2018

Out of interest, why wouldn't a Vec work rather than a Builder. It's also a pointer-to-data, len, & capacity. Is this (by-and-large) the simplest way of having a no-copy constructor?

@andygrove

This comment has been minimized.

Copy link
Member Author

andygrove commented Apr 5, 2018

@maxim-lian Vec is almost perfect, yes. The only problem is that the memory isn't aligned to an 8 or 64 byte boundary as required by the Arrow specification.

@xhochy
xhochy approved these changes Apr 6, 2018
Copy link
Member

xhochy left a comment

+1, LGTM

@xhochy xhochy closed this in 29c376d Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.