- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1k
 
Add cast support for (Large)ListView <-> (Large)List #8735
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.
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
base: main
Are you sure you want to change the base?
Conversation
3065079    to
    36065f0      
    Compare
  
    36065f0    to
    5701fce      
    Compare
  
    | 
           cc @connortsui20 in case you want to take a look at this casting code in datafusion, since you've done a bunch of related things in vortex.  | 
    
5701fce    to
    71e0c6c      
    Compare
  
    | OffsetSize: OffsetSizeTrait, | ||
| { | ||
| let list = array.as_list::<OffsetSize>(); | ||
| let list_view: GenericListViewArray<OffsetSize> = list.clone().into(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses a new From impl, because I saw that was the pattern for other types into ListViewArray, but we don't do that for ListView to List. Just pointing out the inconsistency, but think it's fine.
| (ListView(list_from), List(list_to)) => { | ||
| can_cast_types(list_from.data_type(), list_to.data_type()) | ||
| } | ||
| (LargeListView(list_from), LargeList(list_to)) => { | ||
| can_cast_types(list_from.data_type(), list_to.data_type()) | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to simplify this branch, will do
Which issue does this PR close?
Related to tracking issue #5375 for
ListView.Rationale for this change
We need cast support for
ListViewandLargeListView.Are these changes tested?
Yes