alamb commented on code in PR #7871:
URL: https://github.com/apache/arrow-rs/pull/7871#discussion_r2190751949
##########
parquet-variant/src/variant/list.rs:
##########
@@ -232,52 +232,57 @@ impl<'m, 'v> VariantList<'m, 'v> {
/// [invalid]: Self#Validation
pub fn get(&self, index: usize) -> Option<Variant<'m, 'v>> {
(index < self.num_elements).then(|| {
- self.try_get_impl(index)
- .and_then(Variant::validate)
+ self.try_get_with_shallow_validation(index)
+ .and_then(Variant::with_deep_validation)
.expect("Invalid variant array element")
})
}
/// Fallible version of `get`. Returns element by index, capturing
validation errors
pub fn try_get(&self, index: usize) -> Result<Variant<'m, 'v>, ArrowError>
{
- self.try_get_impl(index)?.validate()
+ self.try_get_with_shallow_validation(index)?
+ .with_deep_validation()
}
- /// Fallible iteration over the elements of this list.
- pub fn iter_try(&self) -> impl Iterator<Item = Result<Variant<'m, 'v>,
ArrowError>> + '_ {
- self.iter_try_impl().map(|result| result?.validate())
+ // Fallible version of `get`, performing only basic (constant-time)
validation.
+ fn try_get_with_shallow_validation(&self, index: usize) ->
Result<Variant<'m, 'v>, ArrowError> {
+ // Fetch the value bytes between the two offsets for this index, from
the value array region
+ // of the byte buffer
+ let byte_range = self.get_offset(index)?..self.get_offset(index + 1)?;
+ let value_bytes =
+ slice_from_slice_at_offset(self.value, self.first_value_byte,
byte_range)?;
+
+ Variant::try_new_with_metadata_and_shallow_validation(self.metadata,
value_bytes)
}
- // Fallible iteration that only performs basic (constant-time) validation.
- fn iter_try_impl(&self) -> impl Iterator<Item = Result<Variant<'m, 'v>,
ArrowError>> + '_ {
- (0..self.len()).map(move |i| self.try_get_impl(i))
+ /// Fallible iteration over the elements of this list.
+ pub fn try_iter(&self) -> impl Iterator<Item = Result<Variant<'m, 'v>,
ArrowError>> + '_ {
+ self.try_iter_with_shallow_validation()
+ .map(|result| result?.with_deep_validation())
}
/// Iterates over the values of this list. When working with [unvalidated]
input, consider
- /// [`Self::iter_try`] to avoid panics due to invalid data.
+ /// [`Self::try_iter`] to avoid panics due to invalid data.
///
/// [unvalidated]: Self#Validation
pub fn iter(&self) -> impl Iterator<Item = Variant<'m, 'v>> + '_ {
- self.iter_try_impl()
+ self.try_iter_with_shallow_validation()
.map(|result| result.expect("Invalid variant list entry"))
}
+ // Fallible iteration that only performs basic (constant-time) validation.
+ fn try_iter_with_shallow_validation(
+ &self,
+ ) -> impl Iterator<Item = Result<Variant<'m, 'v>, ArrowError>> + '_ {
+ (0..self.len()).map(move |i| self.try_get_with_shallow_validation(i))
+ }
+
// Attempts to retrieve the ith offset from the offset array region of the
byte buffer.
fn get_offset(&self, index: usize) -> Result<usize, ArrowError> {
let byte_range =
self.header.first_offset_byte()..self.first_value_byte;
let offset_bytes = slice_from_slice(self.value, byte_range)?;
self.header.offset_size.unpack_usize(offset_bytes, index)
}
-
- // Fallible version of `get`, performing only basic (constant-time)
validation.
- fn try_get_impl(&self, index: usize) -> Result<Variant<'m, 'v>,
ArrowError> {
Review Comment:
I 100% agree with @scovich here
Also, If you have a commit that just moves code around, please feel free to
put it up and tag me aggressively. I'll try and get it merged quicky (as
@scovich says, they are very easy to review)
##########
parquet-variant/src/variant.rs:
##########
@@ -382,21 +385,23 @@ impl<'m, 'v> Variant<'m, 'v> {
VariantBasicType::ShortString => {
Variant::ShortString(decoder::decode_short_string(value_metadata, value_data)?)
}
- VariantBasicType::Object => {
- Variant::Object(VariantObject::try_new_impl(metadata, value)?)
- }
- VariantBasicType::Array =>
Variant::List(VariantList::try_new_impl(metadata, value)?),
+ VariantBasicType::Object => Variant::Object(
+ VariantObject::try_new_with_shallow_validation(metadata,
value)?,
+ ),
+ VariantBasicType::Array =>
Variant::List(VariantList::try_new_with_shallow_validation(
+ metadata, value,
Review Comment:
yeah I don't know -- `cargo fmt` doesn't reformat this line for me locally
(or on CI) 🤷 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
