adamreeve commented on code in PR #9910:
URL: https://github.com/apache/arrow-rs/pull/9910#discussion_r3198233163
##########
arrow-array/src/array/fixed_size_binary_array.rs:
##########
@@ -88,11 +88,19 @@ use std::sync::Arc;
///
#[derive(Clone)]
pub struct FixedSizeBinaryArray {
- data_type: DataType, // Must be DataType::FixedSizeBinary(value_length)
+ /// Must be DataType::FixedSizeBinary(value_length)
Review Comment:
```suggestion
/// Must be DataType::FixedSizeBinary(value_size)
```
##########
arrow-array/src/array/fixed_size_binary_array.rs:
##########
@@ -311,14 +299,16 @@ impl FixedSizeBinaryArray {
offset.saturating_add(len) <= self.len,
"the length + offset of the sliced FixedSizeBinaryArray cannot
exceed the existing length"
);
-
- let size = self.value_length as usize;
+ let offset_bytes = offset
Review Comment:
I think this was already safe due to the assertion above that offset must be
<= `self.len`. We compute `self.len` from the `value_data` buffer length on
array construction so we know `self.len * self.value_size` doesn't overflow.
But defense in depth is good and this check is pretty cheap :+1:.
##########
arrow-array/src/array/fixed_size_binary_array.rs:
##########
@@ -88,11 +88,19 @@ use std::sync::Arc;
///
#[derive(Clone)]
pub struct FixedSizeBinaryArray {
- data_type: DataType, // Must be DataType::FixedSizeBinary(value_length)
+ /// Must be DataType::FixedSizeBinary(value_length)
+ data_type: DataType,
+ /// `len` values, each `value_size` bytes
value_data: Buffer,
+ /// Optional Null Buffer
nulls: Option<NullBuffer>,
+ /// Number of elements in the array
len: usize,
- value_length: i32,
+ /// size of each element, validated to fit in a positive i32
+ ///
+ /// note: Arrow stores `value_len` using i32. This implementation stores it
Review Comment:
I think this is referring to the Arrow format specification and IPC
metadata, which uses the name `byteWidth`
(https://github.com/apache/arrow/blob/2a89d03bbefd620b42126b8e00f8ae57e99cd638/format/Schema.fbs#L211)
```suggestion
/// note: Arrow stores `byteWidth` using i32. This implementation stores
it
```
##########
arrow-string/src/substring.rs:
##########
@@ -124,15 +124,18 @@ pub fn substring(
start as i32,
length.map(|e| e as i32),
),
- DataType::FixedSizeBinary(old_len) => fixed_size_binary_substring(
- array
- .as_any()
- .downcast_ref::<FixedSizeBinaryArray>()
- .expect("a fixed size binary is expected"),
- *old_len,
- start as i32,
- length.map(|e| e as i32),
- ),
+ DataType::FixedSizeBinary(old_len) => {
+ let old_len: usize = (*old_len).try_into().expect("old_len
overflow");
Review Comment:
```suggestion
let old_len: usize = (*old_len).try_into().expect("negative
FixedSizeBinary value length");
```
--
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]
