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.

Sign up for GitHub

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

Preserve list element name #2840

Closed
wants to merge 5 commits into from
Closed

Preserve list element name #2840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15a4a26
scalar list data type
comphead Jun 23, 2022
83e0d1b
Merge branch 'scalar_list_elname' into scalar_list_elname_merged
comphead Jun 29, 2022
72889ad
list element name
comphead Jul 6, 2022
29243c4
Merge branch 'master' into scalar_list_elname_merged
comphead Jul 6, 2022
e9d4fe8
remove unused code
comphead Jul 13, 2022
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
29 changes: 17 additions & 12 deletions datafusion/common/src/scalar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ pub enum ScalarValue {
/// large binary
LargeBinary(Option<Vec<u8>>),
/// list of nested ScalarValue
List(Option<Vec<ScalarValue>>, Box<DataType>),
List(Option<Vec<ScalarValue>>, Box<Field>),
/// Date stored as a signed 32bit int
Date32(Option<i32>),
/// Date stored as a signed 64bit int
Expand Down Expand Up @@ -583,9 +583,9 @@ impl ScalarValue {
ScalarValue::LargeUtf8(_) => DataType::LargeUtf8,
ScalarValue::Binary(_) => DataType::Binary,
ScalarValue::LargeBinary(_) => DataType::LargeBinary,
ScalarValue::List(_, data_type) => DataType::List(Box::new(Field::new(
ScalarValue::List(_, field) => DataType::List(Box::new(Field::new(
"item",
data_type.as_ref().clone(),
field.data_type().clone(),
true,
))),
ScalarValue::Date32(_) => DataType::Date32,
Expand Down Expand Up @@ -1181,7 +1181,7 @@ impl ScalarValue {
.collect::<LargeBinaryArray>(),
),
},
ScalarValue::List(values, data_type) => Arc::new(match data_type.as_ref() {
ScalarValue::List(values, field) => Arc::new(match field.data_type() {
DataType::Boolean => build_list!(BooleanBuilder, Boolean, values, size),
DataType::Int8 => build_list!(Int8Builder, Int8, values, size),
DataType::Int16 => build_list!(Int16Builder, Int16, values, size),
Expand All @@ -1204,7 +1204,7 @@ impl ScalarValue {
repeat(self.clone()).take(size),
&DataType::List(Box::new(Field::new(
"item",
data_type.as_ref().clone(),
field.data_type().clone(),
true,
))),
)
Expand Down Expand Up @@ -1330,8 +1330,10 @@ impl ScalarValue {
Some(scalar_vec)
}
};
let data_type = nested_type.data_type().clone();
ScalarValue::List(value, Box::new(data_type))
ScalarValue::List(
value,
Box::new(Field::new("item", nested_type.data_type().clone(), true)),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is effectively going to lose the original field name (which isn't attached to the array) in this particular codepath, which is a similar problem to what I observed in #2874

However, this PR certainly seems better than the current state of master which loses the field name through all paths

)
}
DataType::Date32 => {
typed_cast!(array, index, Date32Array, Date32)
Expand Down Expand Up @@ -1430,8 +1432,10 @@ impl ScalarValue {
Some(scalar_vec)
}
};
let data_type = nested_type.data_type().clone();
ScalarValue::List(value, Box::new(data_type))
ScalarValue::List(
value,
Box::new(Field::new("item", nested_type.data_type().clone(), true)),
)
}
other => {
return Err(DataFusionError::NotImplemented(format!(
Expand Down Expand Up @@ -1775,9 +1779,10 @@ impl TryFrom<&DataType> for ScalarValue {
DataType::Dictionary(_index_type, value_type) => {
value_type.as_ref().try_into()?
}
DataType::List(ref nested_type) => {
ScalarValue::List(None, Box::new(nested_type.data_type().clone()))
}
DataType::List(ref nested_type) => ScalarValue::List(
None,
Box::new(Field::new("item", nested_type.data_type().clone(), true)),
),
DataType::Struct(fields) => {
ScalarValue::Struct(None, Box::new(fields.clone()))
}
Expand Down
67 changes: 37 additions & 30 deletions datafusion/core/src/scalar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,8 @@ mod tests {
#[test]
fn scalar_list_null_to_array() {
let list_array_ref =
ScalarValue::List(None, Box::new(DataType::UInt64)).to_array();
ScalarValue::List(None, Box::new(Field::new("item", DataType::UInt64, true)))
.to_array();
let list_array = list_array_ref.as_any().downcast_ref::<ListArray>().unwrap();

assert!(list_array.is_null(0));
Expand All @@ -175,7 +176,7 @@ mod tests {
ScalarValue::UInt64(None),
ScalarValue::UInt64(Some(101)),
]),
Box::new(DataType::UInt64),
Box::new(Field::new("item", DataType::UInt64, true)),
)
.to_array();

Expand Down Expand Up @@ -612,35 +613,35 @@ mod tests {
assert_eq!(
List(
Some(vec![Int32(Some(1)), Int32(Some(5))]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
)
.partial_cmp(&List(
Some(vec![Int32(Some(1)), Int32(Some(5))]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
)),
Some(Ordering::Equal)
);

assert_eq!(
List(
Some(vec![Int32(Some(10)), Int32(Some(5))]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
)
.partial_cmp(&List(
Some(vec![Int32(Some(1)), Int32(Some(5))]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
)),
Some(Ordering::Greater)
);

assert_eq!(
List(
Some(vec![Int32(Some(1)), Int32(Some(5))]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
)
.partial_cmp(&List(
Some(vec![Int32(Some(10)), Int32(Some(5))]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
)),
Some(Ordering::Less)
);
Expand All @@ -649,11 +650,11 @@ mod tests {
assert_eq!(
List(
Some(vec![Int64(Some(1)), Int64(Some(5))]),
Box::new(DataType::Int64),
Box::new(Field::new("item", DataType::Int32, true)),
)
.partial_cmp(&List(
Some(vec![Int32(Some(1)), Int32(Some(5))]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
)),
None
);
Expand Down Expand Up @@ -876,17 +877,17 @@ mod tests {
ScalarValue::from(2i32),
ScalarValue::from(3i32),
]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
);

let l1 = ScalarValue::List(
Some(vec![ScalarValue::from(4i32), ScalarValue::from(5i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
);

let l2 = ScalarValue::List(
Some(vec![ScalarValue::from(6i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
);

// Define struct scalars
Expand Down Expand Up @@ -931,12 +932,18 @@ mod tests {
// Define list-of-structs scalars
let nl0 = ScalarValue::List(
Some(vec![s0.clone(), s1.clone()]),
Box::new(s0.get_datatype()),
Box::new(Field::new("item", s0.get_datatype(), true)),
);

let nl1 = ScalarValue::List(Some(vec![s2]), Box::new(s0.get_datatype()));
let nl1 = ScalarValue::List(
Some(vec![s2]),
Box::new(Field::new("item", s0.get_datatype(), true)),
);

let nl2 = ScalarValue::List(Some(vec![s1]), Box::new(s0.get_datatype()));
let nl2 = ScalarValue::List(
Some(vec![s1]),
Box::new(Field::new("item", s0.get_datatype(), true)),
);

// iter_to_array for list-of-struct
let array = ScalarValue::iter_to_array(vec![nl0, nl1, nl2]).unwrap();
Expand Down Expand Up @@ -1087,48 +1094,48 @@ mod tests {
ScalarValue::from(2i32),
ScalarValue::from(3i32),
]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
),
ScalarValue::List(
Some(vec![ScalarValue::from(4i32), ScalarValue::from(5i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
),
]),
Box::new(DataType::List(Box::new(Field::new(
Box::new(Field::new(
"item",
DataType::Int32,
DataType::List(Box::new(Field::new("item", DataType::Int32, true))),
true,
)))),
)),
);

let l2 = ScalarValue::List(
Some(vec![
ScalarValue::List(
Some(vec![ScalarValue::from(6i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
),
ScalarValue::List(
Some(vec![ScalarValue::from(7i32), ScalarValue::from(8i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
),
]),
Box::new(DataType::List(Box::new(Field::new(
Box::new(Field::new(
"item",
DataType::Int32,
DataType::List(Box::new(Field::new("item", DataType::Int32, true))),
true,
)))),
)),
);

let l3 = ScalarValue::List(
Some(vec![ScalarValue::List(
Some(vec![ScalarValue::from(9i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
)]),
Box::new(DataType::List(Box::new(Field::new(
Box::new(Field::new(
"item",
DataType::Int32,
DataType::List(Box::new(Field::new("item", DataType::Int32, true))),
true,
)))),
)),
);

let array = ScalarValue::iter_to_array(vec![l1, l2, l3]).unwrap();
Expand Down
38 changes: 19 additions & 19 deletions datafusion/physical-expr/src/aggregate/array_agg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ impl Accumulator for ArrayAggAccumulator {
fn evaluate(&self) -> Result<ScalarValue> {
Ok(ScalarValue::List(
Some(self.values.clone()),
Box::new(self.datatype.clone()),
Box::new(Field::new("item", self.datatype.clone(), true)),
))
}
}
Expand Down Expand Up @@ -179,7 +179,7 @@ mod tests {
ScalarValue::Int32(Some(4)),
ScalarValue::Int32(Some(5)),
]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
);

generic_test_op!(a, DataType::Int32, ArrayAgg, list, DataType::Int32)
Expand All @@ -195,57 +195,57 @@ mod tests {
ScalarValue::from(2i32),
ScalarValue::from(3i32),
]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
),
ScalarValue::List(
Some(vec![ScalarValue::from(4i32), ScalarValue::from(5i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
),
]),
Box::new(DataType::List(Box::new(Field::new(
Box::new(Field::new(
"item",
DataType::Int32,
DataType::List(Box::new(Field::new("item", DataType::Int32, true))),
true,
)))),
)),
);

let l2 = ScalarValue::List(
Some(vec![
ScalarValue::List(
Some(vec![ScalarValue::from(6i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
),
ScalarValue::List(
Some(vec![ScalarValue::from(7i32), ScalarValue::from(8i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
),
]),
Box::new(DataType::List(Box::new(Field::new(
Box::new(Field::new(
"item",
DataType::Int32,
DataType::List(Box::new(Field::new("item", DataType::Int32, true))),
true,
)))),
)),
);

let l3 = ScalarValue::List(
Some(vec![ScalarValue::List(
Some(vec![ScalarValue::from(9i32)]),
Box::new(DataType::Int32),
Box::new(Field::new("item", DataType::Int32, true)),
)]),
Box::new(DataType::List(Box::new(Field::new(
Box::new(Field::new(
"item",
DataType::Int32,
DataType::List(Box::new(Field::new("item", DataType::Int32, true))),
true,
)))),
)),
);

let list = ScalarValue::List(
Some(vec![l1.clone(), l2.clone(), l3.clone()]),
Box::new(DataType::List(Box::new(Field::new(
Box::new(Field::new(
"item",
DataType::Int32,
DataType::List(Box::new(Field::new("item", DataType::Int32, true))),
true,
)))),
)),
);

let array = ScalarValue::iter_to_array(vec![l1, l2, l3]).unwrap();
Expand Down
Loading