Skip to content

Commit

Permalink
fix(common): ListArray::from_protobuf expects wrong cardinality of …
Browse files Browse the repository at this point in the history
…inner array (#8091)

The internal of `ListArray` stores its data flattened. For example, `values (array[1]), (array[]::int[]), (null), (array[2, 3]);` stores an inner `I32Array` with `[1, 2, 3]`, along with offset array `[0, 1, 1, 1, 3]` and null bitmap `TTFT`.

The cardinality of this inner array is not the length of outer array, but the last element of offset array.

Fixes #8082

Approved-By: kwannoel

Co-Authored-By: Xiangjin <xiangjin@singularity-data.com>
Co-Authored-By: xiangjinwu <17769960+xiangjinwu@users.noreply.github.com>
  • Loading branch information
xiangjinwu and xiangjinwu authored Feb 22, 2023
1 parent 8bc69d4 commit b429e9c
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 7 deletions.
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.
#[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

0 comments on commit b429e9c

Please sign in to comment.