feat: support ListView and LargeListView in ScalarValue#21669
feat: support ListView and LargeListView in ScalarValue#21669Jefffrey wants to merge 10 commits intoapache:mainfrom
ListView and LargeListView in ScalarValue#21669Conversation
github-actions
bot
added
sql
| 3, | ||
| ), | ||
| )), | ||
| ScalarValue::ListView(Arc::new(ListViewArray::from_iter_primitive::< |
There was a problem hiding this comment.
Only change in this file is adding cases for listview & largelistview; other changes is indent change. Would recommend reviewing with whitespace off
| } | ||
| ScalarValue::ListView(arr) => { | ||
| let array = copy_array_data(&arr.to_data()); | ||
| *Arc::make_mut(arr) = ListViewArray::from(array); |
There was a problem hiding this comment.
| *Arc::make_mut(arr) = ListViewArray::from(array); | |
| *Arc::make_mut(arr) = ListViewArray::from(array) |
There was a problem hiding this comment.
I don't see a value add here; it's valid syntax
| 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]); |
There was a problem hiding this comment.
let's use a checked API for the cast here because this may silently truncate
There was a problem hiding this comment.
Fixed to use checked API; it'll panic if overflow but I don't think that'll be a common usage
| let empty_arr = new_empty_array(field.data_type()); | ||
| let values = Arc::new( | ||
| SingleRowListArrayBuilder::new(empty_arr) | ||
| .with_nullable(field.is_nullable()) |
There was a problem hiding this comment.
What is the reason the field name to be ignored here ?
I see none of the similar logic above/below uses
| .with_nullable(field.is_nullable()) | |
| .with_field(field) |
which would preserve both the field's name and is_nullable properties.
There was a problem hiding this comment.
Fixed to use with_field, thanks
4 participants
Which issue does this PR close?
ListView,LargeListViewinScalarValue#18886ListViewandLargeListViewinScalarValue#18884Rationale for this change
More support for listview types in the codebase
What changes are included in this PR?
Added
ListViewandLargeListViewtoScalarValuewith all accompanying changesSupport
ListViewandLargeListViewin proto, both for the arrow datatype & the newly introduced scalarvalue variants.Are these changes tested?
Yes, added tests
Are there any user-facing changes?
No