martin-g commented on code in PR #21669:
URL: https://github.com/apache/datafusion/pull/21669#discussion_r3099096684
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -9836,6 +10166,14 @@ mod tests {
)]))),
None
);
+ assert_eq!(
+ ScalarValue::max(&DataType::ListView(Arc::new(Field::new(
+ "item",
+ DataType::Int32,
+ true
+ )))),
+ None
+ );
Review Comment:
missing test for LargeListView
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1649,6 +1689,24 @@ impl ScalarValue {
let list = ScalarValue::new_large_list(&[], field.data_type());
Ok(ScalarValue::LargeList(list))
}
+ DataType::ListView(field) => {
+ let empty_arr = new_empty_array(field.data_type());
+ let values = Arc::new(
+ SingleRowListArrayBuilder::new(empty_arr)
+ .with_nullable(field.is_nullable())
Review Comment:
What is the reason the field name to be ignored here ?
I see none of the similar logic above/below uses
```suggestion
.with_field(field)
```
which would preserve both the field's `name` and `is_nullable` properties.
##########
datafusion/common/src/scalar/mod.rs:
##########
Review Comment:
Should this be updated to support ListView/LargeListView too ?
##########
datafusion/common/src/utils/mod.rs:
##########
@@ -480,6 +481,32 @@ impl SingleRowListArrayBuilder {
ScalarValue::FixedSizeList(Arc::new(self.build_fixed_size_list_array(list_size)))
}
+ /// Build a single element [`ListViewArray`]
+ pub fn build_list_view_array(self) -> ListViewArray {
+ let (field, arr) = self.into_field_and_arr();
+ let offsets = ScalarBuffer::from(vec![0]);
+ let sizes = ScalarBuffer::from(vec![arr.len() as i32]);
Review Comment:
let's use a checked API for the cast here because this may silently truncate
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -662,15 +675,26 @@ impl PartialOrd for ScalarValue {
(FixedSizeBinary(_, _), _) => None,
(LargeBinary(v1), LargeBinary(v2)) => v1.partial_cmp(v2),
(LargeBinary(_), _) => None,
- // ScalarValue::List / ScalarValue::FixedSizeList /
ScalarValue::LargeList are ensure to have length 1
+ // ScalarValue::List / ScalarValue::FixedSizeList /
ScalarValue::LargeList / ScalarValue::ListView / ScalarValue::LargeListView
+ // are ensure to have length 1
Review Comment:
```suggestion
// are ensured to have length 1
```
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -4671,6 +4747,14 @@ impl ScalarValue {
let array = copy_array_data(&arr.to_data());
*Arc::make_mut(arr) = LargeListArray::from(array)
}
+ ScalarValue::ListView(arr) => {
+ let array = copy_array_data(&arr.to_data());
+ *Arc::make_mut(arr) = ListViewArray::from(array);
Review Comment:
```suggestion
*Arc::make_mut(arr) = ListViewArray::from(array)
```
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -9760,6 +10082,14 @@ mod tests {
)))),
None
);
+ assert_eq!(
+ ScalarValue::min(&DataType::ListView(Arc::new(Field::new(
+ "item",
+ DataType::Int32,
+ true
+ )))),
+ None
+ );
Review Comment:
missing test for LargeListView
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -9651,6 +9962,17 @@ mod tests {
_ => panic!("Expected List"),
}
+ let list_result =
+
ScalarValue::new_default(&DataType::ListView(Arc::new(list_field.clone())))
+ .unwrap();
+ match list_result {
+ ScalarValue::ListView(arr) => {
+ assert_eq!(arr.len(), 1);
+ assert_eq!(arr.value_size(0), 0); // empty list
+ }
+ _ => panic!("Expected List"),
+ }
Review Comment:
Similar test could be added for LargeListView too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
