Skip to content

Commit

Permalink
fix failing test and bad reference desing in LargeList builder
Browse files Browse the repository at this point in the history
  • Loading branch information
@ritchie46
ritchie46 committed Sep 6, 2020
1 parent 27d46ac commit e23cc3a
Show file tree
Hide file tree
Showing 8 changed files with 88 additions and 75 deletions.
39 changes: 26 additions & 13 deletions polars/src/chunked_array/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,8 @@ where
}

pub trait LargListBuilderTrait {
fn append_opt_series(&mut self, opt_s: Option<&Series>);
fn append_opt_series(&mut self, opt_s: &Option<Series>);
fn append_series(&mut self, s: &Series);
fn finish(&mut self) -> LargeListChunked;
}

Expand Down Expand Up @@ -434,6 +435,18 @@ macro_rules! append_opt_series {
}};
}

macro_rules! append_series {
($self:ident, $s: ident) => {{
let data = $s.array_data();
$self
.builder
.values()
.append_data(&data)
.expect("should not fail");
$self.builder.append(true).expect("should not fail");
}};
}

macro_rules! finish_largelist_builder {
($self:ident) => {{
let arr = Arc::new($self.builder.finish());
Expand Down Expand Up @@ -502,10 +515,14 @@ impl<T> LargListBuilderTrait for LargeListPrimitiveChunkedBuilder<T>
where
T: ArrowPrimitiveType,
{
fn append_opt_series(&mut self, opt_s: Option<&Series>) {
fn append_opt_series(&mut self, opt_s: &Option<Series>) {
append_opt_series!(self, opt_s)
}

fn append_series(&mut self, s: &Series) {
append_series!(self, s);
}

fn finish(&mut self) -> LargeListChunked {
finish_largelist_builder!(self)
}
Expand All @@ -527,21 +544,17 @@ impl LargeListUtf8ChunkedBuilder {

LargeListUtf8ChunkedBuilder { builder, field }
}

pub fn append_opt_series(&mut self, opt_s: Option<&Series>) {
append_opt_series!(self, opt_s)
}

pub fn finish(mut self) -> LargeListChunked {
finish_largelist_builder!(self)
}
}

impl LargListBuilderTrait for LargeListUtf8ChunkedBuilder {
fn append_opt_series(&mut self, opt_s: Option<&Series>) {
fn append_opt_series(&mut self, opt_s: &Option<Series>) {
append_opt_series!(self, opt_s)
}

fn append_series(&mut self, s: &Series) {
append_series!(self, s);
}

fn finish(&mut self) -> LargeListChunked {
finish_largelist_builder!(self)
}
Expand Down Expand Up @@ -613,8 +626,8 @@ mod test {
let s2 = Int32Chunked::new_from_slice("b", &[4, 5, 6]).into_series();
s1.append(&s2).unwrap();

builder.append_opt_series(Some(&s1));
builder.append_opt_series(Some(&s2));
builder.append_series(&s1);
builder.append_series(&s2);
let ls = builder.finish();
if let AnyType::LargeList(s) = ls.get_any(0) {
// many chunks are aggregated to one in the ListArray
Expand Down
2 changes: 1 addition & 1 deletion polars/src/chunked_array/chunkops.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ impl ChunkOps for LargeListChunked {
let mut builder =
get_large_list_builder(self.dtype(), chunk_length, self.name());
while let Some(v) = iter.next() {
builder.append_opt_series(v.as_ref())
builder.append_opt_series(&v)
}
let list = builder.finish();
// cheap clone of Arc
Expand Down
30 changes: 16 additions & 14 deletions polars/src/chunked_array/ops.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ impl ChunkFilter<LargeListType> for LargeListChunked {
true => opt_series,
false => None,
};
builder.append_opt_series(opt_val.as_ref())
builder.append_opt_series(&opt_val)
});
Ok(builder.finish())
}
Expand All @@ -688,7 +688,7 @@ impl ChunkFilter<LargeListType> for LargeListChunked {
pub trait ChunkShift<T, V> {
/// Shift the values by a given period and fill the parts that will be empty due to this operation
/// with `fill_value`.
fn shift(&self, periods: i32, fill_value: Option<V>) -> Result<ChunkedArray<T>>;
fn shift(&self, periods: i32, fill_value: &Option<V>) -> Result<ChunkedArray<T>>;
}

fn chunk_shift_helper<T>(
Expand Down Expand Up @@ -719,7 +719,7 @@ where
T: PolarsNumericType,
T::Native: Copy,
{
fn shift(&self, periods: i32, fill_value: Option<T::Native>) -> Result<ChunkedArray<T>> {
fn shift(&self, periods: i32, fill_value: &Option<T::Native>) -> Result<ChunkedArray<T>> {
if periods.abs() >= self.len() as i32 {
return Err(PolarsError::OutOfBounds);
}
Expand All @@ -729,14 +729,14 @@ where
// Fill the front of the array
if periods > 0 {
for _ in 0..periods {
builder.append_option(fill_value)
builder.append_option(*fill_value)
}
chunk_shift_helper(self, &mut builder, amount);
// Fill the back of the array
} else {
chunk_shift_helper(self, &mut builder, amount);
for _ in 0..periods.abs() {
builder.append_option(fill_value)
builder.append_option(*fill_value)
}
}
Ok(builder.finish())
Expand Down Expand Up @@ -774,7 +774,7 @@ macro_rules! impl_shift {
}

impl ChunkShift<BooleanType, bool> for BooleanChunked {
fn shift(&self, periods: i32, fill_value: Option<bool>) -> Result<BooleanChunked> {
fn shift(&self, periods: i32, fill_value: &Option<bool>) -> Result<BooleanChunked> {
if periods.abs() >= self.len() as i32 {
return Err(PolarsError::OutOfBounds);
}
Expand All @@ -783,34 +783,36 @@ impl ChunkShift<BooleanType, bool> for BooleanChunked {
fn append_fn(builder: &mut PrimitiveChunkedBuilder<BooleanType>, v: Option<bool>) {
builder.append_option(v);
}
let fill_value = *fill_value;

impl_shift!(self, builder, periods, fill_value, append_option, append_fn)
}
}

impl ChunkShift<Utf8Type, &str> for Utf8Chunked {
fn shift(&self, periods: i32, fill_value: Option<&str>) -> Result<Utf8Chunked> {
fn shift(&self, periods: i32, fill_value: &Option<&str>) -> Result<Utf8Chunked> {
if periods.abs() >= self.len() as i32 {
return Err(PolarsError::OutOfBounds);
}
let mut builder = Utf8ChunkedBuilder::new(self.name(), self.len());
fn append_fn(builder: &mut Utf8ChunkedBuilder, v: Option<&str>) {
builder.append_option(v);
}
let fill_value = *fill_value;

impl_shift!(self, builder, periods, fill_value, append_option, append_fn)
}
}

impl ChunkShift<LargeListType, &Series> for LargeListChunked {
fn shift(&self, periods: i32, fill_value: Option<&Series>) -> Result<LargeListChunked> {
impl ChunkShift<LargeListType, Series> for LargeListChunked {
fn shift(&self, periods: i32, fill_value: &Option<Series>) -> Result<LargeListChunked> {
if periods.abs() >= self.len() as i32 {
return Err(PolarsError::OutOfBounds);
}
let dt = self.get_inner_dtype();
let mut builder = get_large_list_builder(dt, self.len(), self.name());
fn append_fn(builder: &mut Box<dyn LargListBuilderTrait>, v: Option<Series>) {
builder.append_opt_series(v.as_ref());
builder.append_opt_series(&v);
}

impl_shift!(
Expand All @@ -831,13 +833,13 @@ mod test {
#[test]
fn test_shift() {
let ca = Int32Chunked::new_from_slice("", &[1, 2, 3]);
let shifted = ca.shift(1, Some(0)).unwrap();
let shifted = ca.shift(1, &Some(0)).unwrap();
assert_eq!(shifted.cont_slice().unwrap(), &[0, 1, 2]);
let shifted = ca.shift(1, None).unwrap();
let shifted = ca.shift(1, &None).unwrap();
assert_eq!(Vec::from(&shifted), &[None, Some(1), Some(2)]);
let shifted = ca.shift(-1, None).unwrap();
let shifted = ca.shift(-1, &None).unwrap();
assert_eq!(Vec::from(&shifted), &[Some(1), Some(2), None]);
assert!(ca.shift(3, None).is_err());
assert!(ca.shift(3, &None).is_err());
}

#[test]
Expand Down
12 changes: 6 additions & 6 deletions polars/src/chunked_array/take.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ impl ChunkTake for LargeListChunked {
let taker = self.take_rand();

for idx in indices {
builder.append_opt_series(taker.get(idx).as_ref());
builder.append_opt_series(&taker.get(idx));
}
Ok(builder.finish())
}
Expand All @@ -279,7 +279,7 @@ impl ChunkTake for LargeListChunked {
let taker = self.take_rand();
for idx in indices {
let v = taker.get_unchecked(idx);
builder.append_opt_series(Some(&v));
builder.append_opt_series(&Some(v));
}
builder.finish()
}
Expand All @@ -304,9 +304,9 @@ impl ChunkTake for LargeListChunked {
match opt_idx {
Some(idx) => {
let opt_s = taker.get(idx);
builder.append_opt_series(opt_s.as_ref())
builder.append_opt_series(&opt_s)
}
None => builder.append_opt_series(None),
None => builder.append_opt_series(&None),
};
}
Ok(builder.finish())
Expand All @@ -331,9 +331,9 @@ impl ChunkTake for LargeListChunked {
match opt_idx {
Some(idx) => {
let s = taker.get_unchecked(idx);
builder.append_opt_series(Some(&s))
builder.append_opt_series(&Some(s))
}
None => builder.append_opt_series(None),
None => builder.append_opt_series(&None),
};
}
builder.finish()
Expand Down
37 changes: 25 additions & 12 deletions polars/src/chunked_array/upstream_traits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,9 @@ impl FromIterator<Series> for LargeListChunked {
let v = it.next().unwrap();
let mut builder = get_large_list_builder(v.dtype(), capacity, "collected");

builder.append_opt_series(Some(&v));
builder.append_opt_series(&Some(v));
while let Some(s) = it.next() {
builder.append_opt_series(Some(&s));
builder.append_opt_series(&Some(s));
}
builder.finish()
}
Expand All @@ -156,22 +156,22 @@ impl<'a> FromIterator<&'a Series> for LargeListChunked {
let v = it.next().unwrap();
let mut builder = get_large_list_builder(v.dtype(), capacity, "collected");

builder.append_opt_series(Some(v));
builder.append_series(v);
while let Some(s) = it.next() {
builder.append_opt_series(Some(s));
builder.append_series(s);
}

builder.finish()
}
}

impl FromIterator<Option<Series>> for LargeListChunked {
fn from_iter<I: IntoIterator<Item = Option<Series>>>(iter: I) -> Self {
macro_rules! impl_from_iter_opt_series {
($iter:ident) => {{
// we don't know the type of the series until we get Some(Series) from the iterator.
// until that happens we count the number of None's so that we can first fill the None's until
// we know the type

let mut it = iter.into_iter();
let mut it = $iter.into_iter();

let v;
let mut cnt = 0;
Expand Down Expand Up @@ -199,19 +199,32 @@ impl FromIterator<Option<Series>> for LargeListChunked {

// first fill all None's we encountered
while cnt > 0 {
builder.append_opt_series(None);
builder.append_opt_series(&None);
cnt -= 1;
}

// now the first non None
builder.append_opt_series(Some(v).as_ref());
builder.append_series(&v);

// now we have added all Nones, we can consume the rest of the iterator.
while let Some(opt_s) = it.next() {
builder.append_opt_series(opt_s.as_ref());
builder.append_opt_series(&opt_s);
}

builder.finish()

}}
}

impl FromIterator<Option<Series>> for LargeListChunked {
fn from_iter<I: IntoIterator<Item = Option<Series>>>(iter: I) -> Self {
impl_from_iter_opt_series!(iter)
}
}

impl<'a> FromIterator<&'a Option<Series>> for LargeListChunked {
fn from_iter<I: IntoIterator<Item = &'a Option<Series>>>(iter: I) -> Self {
impl_from_iter_opt_series!(iter)
}
}

Expand Down Expand Up @@ -414,10 +427,10 @@ mod test {
let s1 = Series::new("", &[true, false, true]);
let s2 = Series::new("", &[true, false, true]);

let ll: LargeListChunked = [Some(&s1), Some(&s2)].iter().copied().collect();
let ll: LargeListChunked = [&s1, &s2].iter().map(|&s| s).collect();
assert_eq!(ll.len(), 2);
assert_eq!(ll.null_count(), 0);
let ll: LargeListChunked = [None, Some(&s2)].iter().copied().collect();
let ll: LargeListChunked = [None, Some(s2)].iter().collect();
assert_eq!(ll.len(), 2);
assert_eq!(ll.null_count(), 1);
}
Expand Down
37 changes: 11 additions & 26 deletions polars/src/fmt.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,8 @@ impl Debug for Series {
write![f, "Series: '{}' [{}]\n[\n", $name, $dtype]?;

for i in 0..limit {
let opt_v = $a.get(i);
match opt_v {
Some(v) => write!(f, "\t{}\n", v)?,
None => write!(f, "\tnull\n")?,
}
let v = $a.get_any(i);
write!(f, "\t{}\n", v)?;
}

write![f, "]"]
Expand Down Expand Up @@ -139,26 +136,6 @@ impl Debug for Series {
}

write![f, "]"]
// let limit = 3;
// write![f, "Series: list \n[\n"]?;
// a.into_iter().take(limit).for_each(|opt_s| match opt_s {
// None => {
// write!(f, "\tnull\n").ok();
// }
// Some(s) => {
// // Inner series are indented one level
// let series_formatted = format!("{:?}", s);
//
// // now prepend a tab before every line.
// let mut with_tab = String::with_capacity(series_formatted.len() + 10);
// for line in series_formatted.split("\n") {
// with_tab.push_str(&format!("\t{}\n", line))
// }
//
// write!(f, "\t{}\n", with_tab).ok();
// }
// });
// write![f, "]"]
}
}
}
Expand Down Expand Up @@ -366,7 +343,15 @@ mod test {
builder.append_slice(None);
let list = builder.finish().into_series();

println!("{:?}", list)
println!("{:?}", list);
assert_eq!(
r#"Series: 'a' [list]
[
[1, 2, 3]
null
]"#,
format!("{:?}", list)
);
}

#[test]
Expand Down

0 comments on commit e23cc3a

Please sign in to comment.