-
Notifications
You must be signed in to change notification settings - Fork 786
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
Add DataType::ListView and DataType::LargeListView #5493
Conversation
arrow-data/src/data.rs
Outdated
@@ -119,12 +119,22 @@ pub(crate) fn new_buffers(data_type: &DataType, capacity: usize) -> [MutableBuff | |||
buffer.push(0i32); | |||
[buffer, empty_buffer] | |||
} | |||
DataType::ListView(_) => { | |||
let mut buffer = MutableBuffer::new((1 + capacity) * mem::size_of::<i64>()); | |||
buffer.push(0i32); |
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.
Is this correct? My reading of https://arrow.apache.org/docs/format/Columnar.html#listview-layout is that a ListView of length n
will have an offsets and a sizes buffer both of length n
, instead of a single buffer of length n + 1
like ListArray
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.
You are right, I will fix this later.
(:Originally, I planned to implement the method in the next pull request, so I overlooked this for now.
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.
Is this correct? My reading of https://arrow.apache.org/docs/format/Columnar.html#listview-layout is that a ListView of length
n
will have an offsets and a sizes buffer both of lengthn
, instead of a single buffer of lengthn + 1
likeListArray
Hi @tustvold
Based on the description in the documentation:https://arrow.apache.org/docs/format/Columnar.html#listview-layout, I modified the initialized buffer to be 2 * capacity, which means both the offset buffer and the size buffer, is that correct?
arrow-data/src/data.rs
Outdated
MutableBuffer::new(2 * capacity * mem::size_of::<i32>()), | ||
empty_buffer, |
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.
MutableBuffer::new(2 * capacity * mem::size_of::<i32>()), | |
empty_buffer, | |
MutableBuffer::new(capacity * mem::size_of::<i32>()), | |
MutableBuffer::new(capacity * mem::size_of::<i32>()), |
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.
Thank you @Kikkon -- this looks good to me. I left a small comment suggestion but otherwise 🚀
Co-authored-by: Andrew Lamb <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
Which issue does this PR close?
Closes #5492 .
Rationale for this change
Added DataType LargeListView & ListView, and for unimplemented types, a unimplement will be return.
What changes are included in this PR?
Are there any user-facing changes?