Skip to content

Commit eb208c3

Browse files
authored
remove arrow from Canonical::empty (#2105)
Arrow Lists are nullable but elements of those lists are not. In `Identity::return_dtype`, previous behaviour returned `list(i64?)?` when given `list(i64)?`
1 parent 8d02407 commit eb208c3

File tree

5 files changed

+24
-11
lines changed

5 files changed

+24
-11
lines changed

vortex-array/src/canonical.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,14 @@ use arrow_schema::{DataType, Field, FieldRef, Fields};
1616
use itertools::Itertools;
1717
use vortex_datetime_dtype::{is_temporal_ext_type, TemporalMetadata, TimeUnit};
1818
use vortex_dtype::{DType, NativePType, PType};
19-
use vortex_error::{vortex_bail, VortexError, VortexResult};
19+
use vortex_error::{vortex_bail, VortexError, VortexExpect, VortexResult};
2020

2121
use crate::array::{
2222
varbinview_as_arrow, BoolArray, ExtensionArray, ListArray, NullArray, PrimitiveArray,
2323
StructArray, TemporalArray, VarBinViewArray,
2424
};
2525
use crate::arrow::{infer_data_type, FromArrowArray};
26+
use crate::builders::builder_with_capacity;
2627
use crate::compute::try_cast;
2728
use crate::encoding::Encoding;
2829
use crate::stats::ArrayStatistics;
@@ -100,13 +101,12 @@ impl Canonical {
100101

101102
impl Canonical {
102103
// Create an empty canonical array of the given dtype.
103-
pub fn empty(dtype: &DType) -> VortexResult<Canonical> {
104-
let arrow_dtype = infer_data_type(dtype)?;
105-
ArrayData::from_arrow(
106-
arrow_array::new_empty_array(&arrow_dtype),
107-
dtype.is_nullable(),
108-
)
109-
.into_canonical()
104+
pub fn empty(dtype: &DType) -> Canonical {
105+
Self::try_empty(dtype).vortex_expect("Cannot build an empty array")
106+
}
107+
108+
pub fn try_empty(dtype: &DType) -> VortexResult<Canonical> {
109+
builder_with_capacity(dtype, 0).finish()?.into_canonical()
110110
}
111111
}
112112

vortex-array/src/compute/compare.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ pub fn compare(
124124
DType::Bool((left.dtype().is_nullable() || right.dtype().is_nullable()).into());
125125

126126
if left.is_empty() {
127-
return Ok(Canonical::empty(&result_dtype)?.into_array());
127+
return Ok(Canonical::empty(&result_dtype).into_array());
128128
}
129129

130130
// Always try to put constants on the right-hand side so encodings can optimise themselves.

vortex-array/src/compute/filter.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ pub fn filter(array: &ArrayData, mask: &Mask) -> VortexResult<ArrayData> {
5454

5555
// Fast-path for empty mask.
5656
if true_count == 0 {
57-
return Ok(Canonical::empty(array.dtype())?.into());
57+
return Ok(Canonical::empty(array.dtype()).into());
5858
}
5959

6060
// Fast-path for full mask

vortex-expr/src/identity.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,10 @@ pub fn ident() -> ExprRef {
5050

5151
#[cfg(test)]
5252
mod tests {
53+
use std::sync::Arc;
54+
55+
use vortex_dtype::{DType, Nullability, PType};
56+
5357
use crate::{ident, test_harness};
5458

5559
#[test]
@@ -58,4 +62,13 @@ mod tests {
5862
assert_eq!(ident().return_dtype(&dtype).unwrap(), dtype);
5963
assert_eq!(ident().return_dtype(&dtype).unwrap(), dtype);
6064
}
65+
66+
#[test]
67+
fn list_dtype() {
68+
let in_dtype = DType::List(
69+
Arc::new(DType::Primitive(PType::I64, Nullability::NonNullable)),
70+
Nullability::Nullable,
71+
);
72+
assert_eq!(ident().return_dtype(&in_dtype).unwrap(), in_dtype);
73+
}
6174
}

vortex-expr/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ pub trait VortexExpr: Debug + Send + Sync + DynEq + DynHash + Display {
6969

7070
/// Compute the type of the array returned by [VortexExpr::evaluate].
7171
fn return_dtype(&self, scope_dtype: &DType) -> VortexResult<DType> {
72-
let empty = Canonical::empty(scope_dtype)?.into_array();
72+
let empty = Canonical::empty(scope_dtype).into_array();
7373
self.unchecked_evaluate(&empty)
7474
.map(|array| array.dtype().clone())
7575
}

0 commit comments

Comments
 (0)