Simplify Validation/Alignment APIs of ArrayDataBuilder: validate and align
#6966
Conversation
There was a problem hiding this comment.
Just some minor nits, the use of an unsafe flag does have the unfortunate property of disconnecting the point of unsafe ""entry" from its "usage", but in this case it is probably fine, and this is really an issue with unsafe in general.
Maybe one day we'll get unsafe fields and that'll make this easier to track.
Yes I agree this is unforutnate. I will add some comments to make this as explicit as I can (namely that setting the flag to true incorrectly results in undefined behavior) |
arrow-data/src/data.rs
Outdated
| @@ -1786,6 +1790,10 @@ pub struct ArrayDataBuilder { | |||
| offset: usize, | |||
| buffers: Vec<Buffer>, | |||
| child_data: Vec<ArrayData>, | |||
| /// Should buffers be realigned (copying if necessary)? Defaults to false. | |||
There was a problem hiding this comment.
This is the point of this PR -- to add these flags to the builder directly and then check them during calls to build()
arrow-data/src/data.rs
Outdated
| Ok(data) | ||
| } | ||
|
|
||
| /// Creates an array data, validating all inputs, and aligning any buffers | ||
| #[deprecated(since = "54.1.0", note = "Use ArrayData::with_align_buffers instead")] |
There was a problem hiding this comment.
deprecated one API in favor of setting a flag
Co-authored-by: Raphael Taylor-Davies <[email protected]>
…d align (apache#6966) * Simplify ArrayDataBuilder API for validation and alignment * Update arrow-data/src/data.rs Co-authored-by: Raphael Taylor-Davies <[email protected]> * Rename functions for consistency * Improve documentation --------- Co-authored-by: Raphael Taylor-Davies <[email protected]>
Which issue does this PR close?
Rationale for this change
ArrayDataBuilder has two options today:
There are three methods today:
build()that does validates but does not ensure alignmentbuild_unchecked()that neither validates nor ensures alignmentbuild_aligned()that validates and ensures alignment@totoroyyb proposed to add the other combination (no validation but aligned) in #6938 as a fourth function
build_aligned_uncheckedInstead of a fourth function I think we can use builder flags for a better API
What changes are included in this PR?
validateandalignfields to ArrayBuilderbuild_alignedin favor of those flagsNote I chose not to deprecate
build_uncheckedas that is used in more places in the code so deprecating it would be more API churnAre there any user-facing changes?
New APIs and deprecated APIs in ArrayDataBuilder