Add ListView & LargeListView basic construction and validation
#5664
Conversation
Kikkon
changed the title
ListView & LargeListView basic construction and validation
arrow-data/src/data.rs
Outdated
| if offset > values_length { | ||
| return Err(ArrowError::InvalidArgumentError(format!( | ||
| "Size {} at index {} is offset {} is out of bounds for {}", | ||
| size, i, offset, self.data_type | ||
| ))); | ||
| } | ||
| if size > values_length - offset { | ||
| return Err(ArrowError::InvalidArgumentError(format!( | ||
| "Size {} at index {} is larger than the remaining values for {}", | ||
| size, i, self.data_type | ||
| ))); | ||
| } |
There was a problem hiding this comment.
This logic does not appear to be correct
From https://arrow.apache.org/docs/format/Columnar.html#listview-layout the variants this should be validating are
0 <= offsets[i] <= length of the child array
0 <= offsets[i] + size[i] <= length of the child array
There was a problem hiding this comment.
Since the offset is already greater than 0, there is no boundary check for 0 here. I have adjusted the order for easier understanding.
arrow-data/src/data.rs
Outdated
| size, i, offset, self.data_type | ||
| ))); | ||
| } | ||
| if size + offset > values_length { |
There was a problem hiding this comment.
I think this needs to be checked addition to handle if size and offset were both i64::MAX, perhaps we could get a test of this?
There was a problem hiding this comment.
Why do we need to check if both the offset and size are i64::max here? Are we concerned about exceeding the maximum limit of usize? I don't think either of them should exceed the maximum value of usize, right?
There was a problem hiding this comment.
The addition could overflow
arrow-data/src/data.rs
Outdated
| if offset > values_length { | ||
| return Err(ArrowError::InvalidArgumentError(format!( | ||
| "Size {} at index {} is offset {} is out of bounds for {}", | ||
| size, i, offset, self.data_type | ||
| ))); | ||
| } |
There was a problem hiding this comment.
I think we can probably drop this check in favor of the one below
|
Thank you |
Which issue does this PR close?
A part of #5501
Rationale for this change
What changes are included in this PR?
Add ListView & LargeListView basic ArrayData construction and validation
Are there any user-facing changes?