-
Notifications
You must be signed in to change notification settings - Fork 819
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
Fix Decimal and List ArrayData Validation (#1813) (#1814) #1816
Conversation
&'a self, | ||
buffer: &'a Buffer, | ||
) -> Result<&'a [T]> { | ||
fn typed_offsets<T: ArrowNativeType + num::Num>(&self) -> Result<&[T]> { |
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 found the way this accepted a buffer, but then used information from self to interpret that data, deeply confusing. I therefore changed this
c201841
to
76bc699
Compare
Fix offset validation for sliced children of list arrays (apache#1814)
76bc699
to
bdaba34
Compare
@@ -819,12 +826,12 @@ impl ArrayData { | |||
match &self.data_type { | |||
DataType::List(field) | DataType::Map(field, _) => { | |||
let values_data = self.get_single_valid_child_data(field.data_type())?; | |||
self.validate_offsets::<i32>(&self.buffers[0], values_data.len)?; | |||
self.validate_offsets::<i32>(values_data.len)?; |
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 is the fix for #1814
}; | ||
let value = i128::from_le_bytes(raw_val.try_into().unwrap()); | ||
validate_decimal_precision(value, *p)?; | ||
let values_buffer: &[i128] = self.typed_buffer(0, self.len)?; |
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 is the fix for #1813
Codecov Report
@@ Coverage Diff @@
## master #1816 +/- ##
==========================================
+ Coverage 83.42% 83.44% +0.01%
==========================================
Files 199 199
Lines 56632 56639 +7
==========================================
+ Hits 47244 47260 +16
+ Misses 9388 9379 -9
Continue to review full report at Codecov.
|
|
||
if (buffer.len() / std::mem::size_of::<T>()) < required_offsets { | ||
if buffer.len() < required_len { |
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.
Hmm, shouldn't it be
let required_len = len * std::mem::size_of::<T>();
if buffer.len() < required_len {
...
}
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.
No? You need to take the offset into account, otherwise you will potentially panic a few lines down?
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.
Oh, looks correct. ArrayData.offset is offset into buffer, different than buffer's offset.
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.
Yeah, it's pretty confusing. FWIW #1799 may eventually allow us to get rid of this source of bugs
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.
Thanks @tustvold. The fix looks good to me. The refactoring also makes the code more clear.
Co-authored-by: Liang-Chi Hsieh <[email protected]>
Which issue does this PR close?
Closes #1813
Closes #1814
Rationale for this change
See tickets
What changes are included in this PR?
See tickets
Are there any user-facing changes?
No