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

Change builder append methods to be infallible where possible #2103

Merged
merged 5 commits into from
Jul 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions arrow/benches/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ fn bench_primitive(c: &mut Criterion) {
b.iter(|| {
let mut builder = Int64Builder::new(64);
for _ in 0..NUM_BATCHES {
let _ = black_box(builder.append_slice(&data[..]));
builder.append_slice(&data[..]);
}
black_box(builder.finish());
})
Expand All @@ -57,7 +57,7 @@ fn bench_primitive_nulls(c: &mut Criterion) {
b.iter(|| {
let mut builder = UInt8Builder::new(64);
for _ in 0..NUM_BATCHES * BATCH_SIZE {
let _ = black_box(builder.append_null());
builder.append_null();
}
black_box(builder.finish());
})
Expand All @@ -80,7 +80,7 @@ fn bench_bool(c: &mut Criterion) {
b.iter(|| {
let mut builder = BooleanBuilder::new(64);
for _ in 0..NUM_BATCHES {
let _ = black_box(builder.append_slice(&data[..]));
builder.append_slice(&data[..]);
}
black_box(builder.finish());
})
Expand All @@ -98,7 +98,7 @@ fn bench_string(c: &mut Criterion) {
b.iter(|| {
let mut builder = StringBuilder::new(64);
for _ in 0..NUM_BATCHES * BATCH_SIZE {
let _ = black_box(builder.append_value(SAMPLE_STRING));
builder.append_value(SAMPLE_STRING);
}
black_box(builder.finish());
})
Expand Down
8 changes: 4 additions & 4 deletions arrow/benches/cast_kernels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@ fn build_utf8_date_array(size: usize, with_nulls: bool) -> ArrayRef {

for _ in 0..size {
if with_nulls && rng.gen::<f32>() > 0.8 {
builder.append_null().unwrap();
builder.append_null();
} else {
let string = NaiveDate::from_num_days_from_ce(rng.sample(range))
.format("%Y-%m-%d")
.to_string();
builder.append_value(&string).unwrap();
builder.append_value(&string);
}
}
Arc::new(builder.finish())
Expand All @@ -70,12 +70,12 @@ fn build_utf8_date_time_array(size: usize, with_nulls: bool) -> ArrayRef {

for _ in 0..size {
if with_nulls && rng.gen::<f32>() > 0.8 {
builder.append_null().unwrap();
builder.append_null();
} else {
let string = NaiveDateTime::from_timestamp(rng.sample(range), 0)
.format("%Y-%m-%dT%H:%M:%S")
.to_string();
builder.append_value(&string).unwrap();
builder.append_value(&string);
}
}
Arc::new(builder.finish())
Expand Down
4 changes: 2 additions & 2 deletions arrow/benches/take_kernels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ fn create_random_index(size: usize, null_density: f32) -> UInt32Array {
let mut builder = UInt32Builder::new(size);
for _ in 0..size {
if rng.gen::<f32>() < null_density {
builder.append_null().unwrap()
builder.append_null();
} else {
let value = rng.gen_range::<u32, _>(0u32..size as u32);
builder.append_value(value).unwrap();
builder.append_value(value);
}
}
builder.finish()
Expand Down
12 changes: 5 additions & 7 deletions arrow/examples/builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,17 @@ fn main() {
let mut primitive_array_builder = Int32Builder::new(100);

// Append an individual primitive value
primitive_array_builder.append_value(55).unwrap();
primitive_array_builder.append_value(55);

// Append a null value
primitive_array_builder.append_null().unwrap();
primitive_array_builder.append_null();

// Append a slice of primitive values
primitive_array_builder.append_slice(&[39, 89, 12]).unwrap();
primitive_array_builder.append_slice(&[39, 89, 12]);

// Append lots of values
primitive_array_builder.append_null().unwrap();
primitive_array_builder
.append_slice(&(25..50).collect::<Vec<i32>>())
.unwrap();
primitive_array_builder.append_null();
primitive_array_builder.append_slice(&(25..50).collect::<Vec<i32>>());

// Build the `PrimitiveArray`
let primitive_array = primitive_array_builder.finish();
Expand Down
6 changes: 3 additions & 3 deletions arrow/src/array/array_boolean.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,9 @@ mod tests {
#[test]
fn test_boolean_with_null_fmt_debug() {
let mut builder = BooleanArray::builder(3);
builder.append_value(true).unwrap();
builder.append_null().unwrap();
builder.append_value(false).unwrap();
builder.append_value(true);
builder.append_null();
builder.append_value(false);
let arr = builder.finish();
assert_eq!(
"BooleanArray\n[\n true,\n null,\n false,\n]",
Expand Down
6 changes: 2 additions & 4 deletions arrow/src/array/array_dictionary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,7 @@ impl<'a, T: ArrowPrimitiveType + ArrowDictionaryKeyType> FromIterator<Option<&'a
.append(i)
.expect("Unable to append a value to a dictionary array.");
} else {
builder
.append_null()
.expect("Unable to append a null value to a dictionary array.");
builder.append_null();
}
});

Expand Down Expand Up @@ -463,7 +461,7 @@ mod tests {
let value_builder = PrimitiveBuilder::<UInt32Type>::new(2);
let mut builder = PrimitiveDictionaryBuilder::new(key_builder, value_builder);
builder.append(12345678).unwrap();
builder.append_null().unwrap();
builder.append_null();
builder.append(22345678).unwrap();
let array = builder.finish();
assert_eq!(
Expand Down
6 changes: 3 additions & 3 deletions arrow/src/array/array_primitive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -937,9 +937,9 @@ mod tests {
#[test]
fn test_int32_with_null_fmt_debug() {
let mut builder = Int32Array::builder(3);
builder.append_slice(&[0, 1]).unwrap();
builder.append_null().unwrap();
builder.append_slice(&[3, 4]).unwrap();
builder.append_slice(&[0, 1]);
builder.append_null();
builder.append_slice(&[3, 4]);
let arr = builder.finish();
assert_eq!(
"PrimitiveArray<Int32>\n[\n 0,\n 1,\n null,\n 3,\n 4,\n]",
Expand Down
13 changes: 5 additions & 8 deletions arrow/src/array/array_string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -446,15 +446,12 @@ mod tests {
let string_builder = StringBuilder::new(3);
let mut list_of_string_builder = ListBuilder::new(string_builder);

list_of_string_builder.values().append_value("foo").unwrap();
list_of_string_builder.values().append_value("bar").unwrap();
list_of_string_builder.append(true).unwrap();
list_of_string_builder.values().append_value("foo");
list_of_string_builder.values().append_value("bar");
list_of_string_builder.append(true);

list_of_string_builder
.values()
.append_value("foobar")
.unwrap();
list_of_string_builder.append(true).unwrap();
list_of_string_builder.values().append_value("foobar");
list_of_string_builder.append(true);
let list_of_strings = list_of_string_builder.finish();

assert_eq!(list_of_strings.len(), 2);
Expand Down
44 changes: 22 additions & 22 deletions arrow/src/array/builder/boolean_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ use crate::array::ArrayData;
use crate::array::ArrayRef;
use crate::array::BooleanArray;
use crate::datatypes::DataType;
use crate::error::{ArrowError, Result};

use crate::error::ArrowError;
use crate::error::Result;

use super::BooleanBufferBuilder;

Expand Down Expand Up @@ -81,44 +83,42 @@ impl BooleanBuilder {

/// Appends a value of type `T` into the builder
#[inline]
pub fn append_value(&mut self, v: bool) -> Result<()> {
pub fn append_value(&mut self, v: bool) {
self.values_builder.append(v);
if let Some(b) = self.bitmap_builder.as_mut() {
b.append(true)
}
Ok(())
}

/// Appends a null slot into the builder
#[inline]
pub fn append_null(&mut self) -> Result<()> {
pub fn append_null(&mut self) {
self.materialize_bitmap_builder();
self.bitmap_builder.as_mut().unwrap().append(false);
self.values_builder.advance(1);
Ok(())
}

/// Appends an `Option<T>` into the builder
#[inline]
pub fn append_option(&mut self, v: Option<bool>) -> Result<()> {
pub fn append_option(&mut self, v: Option<bool>) {
match v {
None => self.append_null()?,
Some(v) => self.append_value(v)?,
None => self.append_null(),
Some(v) => self.append_value(v),
};
Ok(())
}

/// Appends a slice of type `T` into the builder
#[inline]
pub fn append_slice(&mut self, v: &[bool]) -> Result<()> {
pub fn append_slice(&mut self, v: &[bool]) {
if let Some(b) = self.bitmap_builder.as_mut() {
b.append_n(v.len(), true)
}
self.values_builder.append_slice(v);
Ok(())
}

/// Appends values from a slice of type `T` and a validity boolean slice
/// Appends values from a slice of type `T` and a validity boolean slice.
///
/// Returns an error if the slices are of different lengths
#[inline]
pub fn append_values(&mut self, values: &[bool], is_valid: &[bool]) -> Result<()> {
if values.len() != is_valid.len() {
Expand Down Expand Up @@ -205,9 +205,9 @@ mod tests {
let mut builder = BooleanArray::builder(10);
for i in 0..10 {
if i == 3 || i == 6 || i == 9 {
builder.append_value(true).unwrap();
builder.append_value(true);
} else {
builder.append_value(false).unwrap();
builder.append_value(false);
}
}

Expand All @@ -229,10 +229,10 @@ mod tests {
BooleanArray::from(vec![Some(true), Some(false), None, None, Some(false)]);

let mut builder = BooleanArray::builder(0);
builder.append_slice(&[true, false]).unwrap();
builder.append_null().unwrap();
builder.append_null().unwrap();
builder.append_value(false).unwrap();
builder.append_slice(&[true, false]);
builder.append_null();
builder.append_null();
builder.append_value(false);
let arr2 = builder.finish();

assert_eq!(arr1, arr2);
Expand All @@ -243,7 +243,7 @@ mod tests {
let arr1 = BooleanArray::from(vec![true; 513]);

let mut builder = BooleanArray::builder(512);
builder.append_slice(&[true; 513]).unwrap();
builder.append_slice(&[true; 513]);
let arr2 = builder.finish();

assert_eq!(arr1, arr2);
Expand All @@ -252,9 +252,9 @@ mod tests {
#[test]
fn test_boolean_array_builder_no_null() {
let mut builder = BooleanArray::builder(0);
builder.append_option(Some(true)).unwrap();
builder.append_value(false).unwrap();
builder.append_slice(&[true, false, true]).unwrap();
builder.append_option(Some(true));
builder.append_value(false);
builder.append_slice(&[true, false, true]);
builder
.append_values(&[false, false, true], &[true, true, true])
.unwrap();
Expand Down
13 changes: 5 additions & 8 deletions arrow/src/array/builder/buffer_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,6 @@ mod tests {
use crate::array::Int32BufferBuilder;
use crate::array::Int8Builder;
use crate::array::UInt8BufferBuilder;
use crate::error::Result;

#[test]
fn test_builder_i32_empty() {
Expand Down Expand Up @@ -457,17 +456,17 @@ mod tests {
}

#[test]
fn test_append_values() -> Result<()> {
fn test_append_values() {
let mut a = Int8Builder::new(0);
a.append_value(1)?;
a.append_null()?;
a.append_value(-2)?;
a.append_value(1);
a.append_null();
a.append_value(-2);
assert_eq!(a.len(), 3);

// append values
let values = &[1, 2, 3, 4];
let is_valid = &[true, true, false, true];
a.append_values(values, is_valid)?;
a.append_values(values, is_valid);

assert_eq!(a.len(), 7);
let array = a.finish();
Expand All @@ -478,7 +477,5 @@ mod tests {
assert_eq!(array.value(4), 2);
assert!(array.is_null(5));
assert_eq!(array.value(6), 4);

Ok(())
}
}
12 changes: 7 additions & 5 deletions arrow/src/array/builder/decimal_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ impl DecimalBuilder {

/// Append a null value to the array.
#[inline]
pub fn append_null(&mut self) -> Result<()> {
pub fn append_null(&mut self) {
self.builder.append_null()
}

Expand Down Expand Up @@ -167,6 +167,8 @@ impl Decimal256Builder {
}

/// Appends a [`Decimal256`] number into the builder.
///
/// Returns an error if `value` has different precision, scale or length in bytes than this builder
#[inline]
pub fn append_value(&mut self, value: &Decimal256) -> Result<()> {
if self.precision != value.precision() || self.scale != value.scale() {
Expand All @@ -187,7 +189,7 @@ impl Decimal256Builder {

/// Append a null value to the array.
#[inline]
pub fn append_null(&mut self) -> Result<()> {
pub fn append_null(&mut self) {
self.builder.append_null()
}

Expand Down Expand Up @@ -215,7 +217,7 @@ mod tests {
let mut builder = DecimalBuilder::new(30, 38, 6);

builder.append_value(8_887_000_000_i128).unwrap();
builder.append_null().unwrap();
builder.append_null();
builder.append_value(-8_887_000_000_i128).unwrap();
let decimal_array: DecimalArray = builder.finish();

Expand All @@ -233,7 +235,7 @@ mod tests {
builder
.append_value(Decimal128::new_from_i128(30, 38, 8_887_000_000_i128))
.unwrap();
builder.append_null().unwrap();
builder.append_null();
builder
.append_value(Decimal128::new_from_i128(30, 38, -8_887_000_000_i128))
.unwrap();
Expand All @@ -255,7 +257,7 @@ mod tests {
let value = Decimal256::try_new_from_bytes(40, 6, bytes.as_slice()).unwrap();
builder.append_value(&value).unwrap();

builder.append_null().unwrap();
builder.append_null();

bytes = vec![255; 32];
let value = Decimal256::try_new_from_bytes(40, 6, bytes.as_slice()).unwrap();
Expand Down
Loading