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

fix(common): ListArray::from_protobuf expects wrong cardinality of inner array #8091

Merged
merged 5 commits into from
Feb 22, 2023
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
14 changes: 14 additions & 0 deletions e2e_test/batch/types/array_ty.slt.part
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,20 @@ select max(ARRAY[1, v1*2]) from t;
----
{1,6}

query T
select CAST(NULL as bool[]) from t;
----
NULL
NULL
NULL

query T
select array[false, false] from t;
----
{f,f}
{f,f}
{f,f}

statement ok
drop table t;

Expand Down
8 changes: 7 additions & 1 deletion src/common/src/array/column_proto_readers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ pub fn read_numeric_array<T: PrimitiveArrayItemType, R: PrimitiveValueReader<T>>
}
}
let arr = builder.finish();
ensure_eq!(arr.len(), cardinality);

Ok(arr.into())
}

Expand All @@ -68,7 +70,7 @@ pub fn read_bool_array(array: &ProstArray, cardinality: usize) -> ArrayResult<Ar
let bitmap: Bitmap = array.get_null_bitmap()?.into();

let arr = BoolArray::new(data, bitmap);
assert_eq!(arr.len(), cardinality);
ensure_eq!(arr.len(), cardinality);

Ok(arr.into())
}
Expand Down Expand Up @@ -133,6 +135,8 @@ macro_rules! read_one_value_array {
}
}
let arr = builder.finish();
ensure_eq!(arr.len(), cardinality);

Ok(arr.into())
}
)*
Expand Down Expand Up @@ -195,5 +199,7 @@ pub fn read_string_array<B: ArrayBuilder, R: VarSizedValueReader<B>>(
}
}
let arr = builder.finish();
ensure_eq!(arr.len(), cardinality);

Ok(arr.into())
}
2 changes: 1 addition & 1 deletion src/common/src/array/decimal_array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ mod tests {

assert_eq!(prost_array.values.len(), 1);

let decoded_array = ArrayImpl::from_protobuf(&prost_array, 4)
let decoded_array = ArrayImpl::from_protobuf(&prost_array, 8)
.unwrap()
.into_decimal();

Expand Down
19 changes: 14 additions & 5 deletions src/common/src/array/list_array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ use crate::types::{
DatumRef, Scalar, ScalarRefImpl, ToDatumRef,
};

/// This is a naive implementation of list array.
/// We will eventually move to a more efficient flatten implementation.
Comment on lines -35 to -36
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[derive(Debug)]
pub struct ListArrayBuilder {
bitmap: BitmapBuilder,
Expand Down Expand Up @@ -146,8 +144,15 @@ impl ListArrayBuilder {
}
}

/// This is a naive implementation of list array.
/// We will eventually move to a more efficient flatten implementation.
/// Each item of this `ListArray` is a `List<T>`, or called `T[]` (T array).
///
/// * As other arrays, there is a null bitmap, with `1` meaning nonnull and `0` meaning null.
/// * As [`BytesArray`], there is an offsets `Vec` and a value `Array`. The value `Array` has all
/// items concatenated, and the offsets `Vec` stores start and end indices into it for slicing.
/// Effectively, the inner array is the flattened form, and `offsets.len() == n + 1`.
///
/// For example, `values (array[1]), (array[]::int[]), (null), (array[2, 3]);` stores an inner
/// `I32Array` with `[1, 2, 3]`, along with offsets `[0, 1, 1, 1, 3]` and null bitmap `TTFT`.
#[derive(Debug, Clone, PartialEq)]
pub struct ListArray {
bitmap: Bitmap,
Expand Down Expand Up @@ -221,7 +226,11 @@ impl ListArray {
);
let bitmap: Bitmap = array.get_null_bitmap()?.into();
let array_data = array.get_list_array_data()?.to_owned();
let value = ArrayImpl::from_protobuf(array_data.value.as_ref().unwrap(), bitmap.len())?;
let flatten_len = match array_data.offsets.last() {
Some(&n) => n as usize,
None => bail!("Must have at least one element in offsets"),
};
let value = ArrayImpl::from_protobuf(array_data.value.as_ref().unwrap(), flatten_len)?;
let arr = ListArray {
bitmap,
offsets: array_data.offsets,
Expand Down