Skip to content

Commit 4bf9db8

Browse files
AdamGSAdamGS
committed
CR comment - option instead of panic
1 parent da11f13 commit 4bf9db8

File tree

3 files changed

+45
-43
lines changed

3 files changed

+45
-43
lines changed

‎parquet-variant-compute/src/shred_variant.rs‎

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -876,7 +876,7 @@ mod tests {
876876
Some(expected_variant) => {
877877
assert!(fallback_value.is_valid(idx));
878878
let metadata_bytes =
879-
binary_array_value(fallback_metadata.as_ref(), idx);
879+
binary_array_value(fallback_metadata.as_ref(), idx).unwrap();
880880
let metadata_bytes =
881881
if fallback_metadata.is_valid(idx) && !metadata_bytes.is_empty() {
882882
metadata_bytes
@@ -886,7 +886,7 @@ mod tests {
886886
assert_eq!(
887887
Variant::new(
888888
metadata_bytes,
889-
binary_array_value(fallback_value.as_ref(), idx)
889+
binary_array_value(fallback_value.as_ref(), idx).unwrap()
890890
),
891891
expected_variant.clone()
892892
);
@@ -952,7 +952,7 @@ mod tests {
952952
assert_eq!(
953953
Variant::new(
954954
EMPTY_VARIANT_METADATA_BYTES,
955-
binary_array_value(element_fallbacks.as_ref(), idx)
955+
binary_array_value(element_fallbacks.as_ref(), idx).unwrap()
956956
),
957957
expected_variant.clone()
958958
);
@@ -1099,8 +1099,8 @@ mod tests {
10991099
assert!(typed_value_field.is_null(1)); // typed_value should be null
11001100
assert_eq!(
11011101
Variant::new(
1102-
binary_array_value(metadata_field.as_ref(), 1),
1103-
binary_array_value(value_field.as_ref(), 1)
1102+
binary_array_value(metadata_field.as_ref(), 1).unwrap(),
1103+
binary_array_value(value_field.as_ref(), 1).unwrap()
11041104
),
11051105
Variant::from("hello")
11061106
);
@@ -1118,8 +1118,8 @@ mod tests {
11181118
assert!(!value_field.is_null(4)); // should contain Variant::Null
11191119
assert_eq!(
11201120
Variant::new(
1121-
binary_array_value(metadata_field.as_ref(), 4),
1122-
binary_array_value(value_field.as_ref(), 4)
1121+
binary_array_value(metadata_field.as_ref(), 4).unwrap(),
1122+
binary_array_value(value_field.as_ref(), 4).unwrap()
11231123
),
11241124
Variant::Null
11251125
);
@@ -1198,8 +1198,8 @@ mod tests {
11981198
assert!(typed_value.is_null(1));
11991199
assert_eq!(
12001200
Variant::new(
1201-
binary_array_value(metadata.as_ref(), 1),
1202-
binary_array_value(value.as_ref(), 1)
1201+
binary_array_value(metadata.as_ref(), 1).unwrap(),
1202+
binary_array_value(value.as_ref(), 1).unwrap()
12031203
),
12041204
Variant::from(42i64)
12051205
);
@@ -1215,8 +1215,8 @@ mod tests {
12151215
assert!(typed_value.is_null(3));
12161216
assert_eq!(
12171217
Variant::new(
1218-
binary_array_value(metadata.as_ref(), 3),
1219-
binary_array_value(value.as_ref(), 3)
1218+
binary_array_value(metadata.as_ref(), 3).unwrap(),
1219+
binary_array_value(value.as_ref(), 3).unwrap()
12201220
),
12211221
Variant::Null
12221222
);
@@ -1260,8 +1260,8 @@ mod tests {
12601260
assert!(typed_value.is_null(1));
12611261
assert_eq!(
12621262
Variant::new(
1263-
binary_array_value(metadata.as_ref(), 1),
1264-
binary_array_value(value.as_ref(), 1)
1263+
binary_array_value(metadata.as_ref(), 1).unwrap(),
1264+
binary_array_value(value.as_ref(), 1).unwrap()
12651265
),
12661266
Variant::from("not_binary")
12671267
);
@@ -1277,8 +1277,8 @@ mod tests {
12771277
assert!(typed_value.is_null(3));
12781278
assert_eq!(
12791279
Variant::new(
1280-
binary_array_value(metadata.as_ref(), 3),
1281-
binary_array_value(value.as_ref(), 3)
1280+
binary_array_value(metadata.as_ref(), 3).unwrap(),
1281+
binary_array_value(value.as_ref(), 3).unwrap()
12821282
),
12831283
Variant::Null
12841284
);
@@ -1684,7 +1684,7 @@ mod tests {
16841684
assert_eq!(
16851685
Variant::new(
16861686
EMPTY_VARIANT_METADATA_BYTES,
1687-
binary_array_value(id_values.as_ref(), 1)
1687+
binary_array_value(id_values.as_ref(), 1).unwrap()
16881688
),
16891689
Variant::Null
16901690
);
@@ -1804,8 +1804,8 @@ mod tests {
18041804
value: &'v dyn Array,
18051805
) -> Variant<'m, 'v> {
18061806
Variant::new(
1807-
binary_array_value(metadata, i),
1808-
binary_array_value(value, i),
1807+
binary_array_value(metadata, i).unwrap(),
1808+
binary_array_value(value, i).unwrap(),
18091809
)
18101810
}
18111811
let expect = |i, expected_result: Option<ShreddedValue<ShreddedStruct>>| {
@@ -2002,7 +2002,7 @@ mod tests {
20022002
// Helper to correctly create a variant object using a row's existing metadata
20032003
let object_with_foo_field = |i| {
20042004
use parquet_variant::{ParentState, ValueBuilder, VariantMetadata};
2005-
let metadata = VariantMetadata::new(binary_array_value(metadata.as_ref(), i));
2005+
let metadata = VariantMetadata::new(binary_array_value(metadata.as_ref(), i).unwrap());
20062006
let mut metadata_builder = ReadOnlyMetadataBuilder::new(&metadata);
20072007
let mut value_builder = ValueBuilder::new();
20082008
let state = ParentState::variant(&mut value_builder, &mut metadata_builder);
@@ -2102,8 +2102,8 @@ mod tests {
21022102
assert!(value_field.is_valid(3));
21032103
assert_eq!(
21042104
Variant::new(
2105-
binary_array_value(result.metadata_field().as_ref(), 3),
2106-
binary_array_value(value_field.as_ref(), 3)
2105+
binary_array_value(result.metadata_field().as_ref(), 3).unwrap(),
2106+
binary_array_value(value_field.as_ref(), 3).unwrap()
21072107
),
21082108
Variant::from("not an object")
21092109
);
@@ -2292,8 +2292,8 @@ mod tests {
22922292

22932293
// Verify the value field contains the name field
22942294
let row_1_variant = Variant::new(
2295-
binary_array_value(metadata.as_ref(), 1),
2296-
binary_array_value(value.as_ref(), 1),
2295+
binary_array_value(metadata.as_ref(), 1).unwrap(),
2296+
binary_array_value(value.as_ref(), 1).unwrap(),
22972297
);
22982298
let Variant::Object(obj) = row_1_variant else {
22992299
panic!("Expected object");
@@ -2327,8 +2327,8 @@ mod tests {
23272327
assert!(session_id_value.is_valid(3)); // type mismatch, stored in value
23282328
assert!(session_id_typed_value.is_null(3));
23292329
let session_id_variant = Variant::new(
2330-
binary_array_value(metadata.as_ref(), 3),
2331-
binary_array_value(session_id_value.as_ref(), 3),
2330+
binary_array_value(metadata.as_ref(), 3).unwrap(),
2331+
binary_array_value(session_id_value.as_ref(), 3).unwrap(),
23322332
);
23332333
assert_eq!(session_id_variant, Variant::from("not-a-uuid"));
23342334

@@ -2341,8 +2341,8 @@ mod tests {
23412341
assert!(id_value.is_valid(4)); // type mismatch, stored in value
23422342
assert!(id_typed_value.is_null(4));
23432343
let id_variant = Variant::new(
2344-
binary_array_value(metadata.as_ref(), 4),
2345-
binary_array_value(id_value.as_ref(), 4),
2344+
binary_array_value(metadata.as_ref(), 4).unwrap(),
2345+
binary_array_value(id_value.as_ref(), 4).unwrap(),
23462346
);
23472347
assert_eq!(id_variant, Variant::from(12345i64));
23482348

‎parquet-variant-compute/src/unshred_variant.rs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ pub fn unshred_variant(array: &VariantArray) -> Result<VariantArray> {
7676
if array.is_null(i) {
7777
value_builder.append_null();
7878
} else {
79-
let metadata_bytes = binary_array_value(metadata.as_ref(), i);
79+
let metadata_bytes = binary_array_value(metadata.as_ref(), i)
80+
.expect("metadata field must be a binary-like array");
8081
let metadata = VariantMetadata::new(metadata_bytes);
8182
let mut value_builder = value_builder.builder_ext(&metadata);
8283
row_builder.append_row(&mut value_builder, &metadata, i)?;
@@ -329,7 +330,8 @@ impl<'a> ValueOnlyUnshredVariantBuilder<'a> {
329330
if self.value.is_null(index) {
330331
builder.append_null();
331332
} else {
332-
let value_bytes = binary_array_value(self.value.as_ref(), index);
333+
let value_bytes = binary_array_value(self.value.as_ref(), index)
334+
.expect("value field must be a binary-like array");
333335
let variant = Variant::new_with_metadata(metadata.clone(), value_bytes);
334336
builder.append_value(variant);
335337
}
@@ -353,7 +355,8 @@ macro_rules! handle_unshredded_case {
353355
($self:expr, $builder:expr, $metadata:expr, $index:expr, $partial_shredding:expr) => {{
354356
let value = $self.value.as_ref().filter(|v| v.is_valid($index));
355357
let value = value.map(|v| {
356-
let bytes = binary_array_value(v.as_ref(), $index);
358+
let bytes = binary_array_value(v.as_ref(), $index)
359+
.expect("value field must be a binary-like array");
357360
Variant::new_with_metadata($metadata.clone(), bytes)
358361
});
359362

‎parquet-variant-compute/src/variant_array.rs‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,17 +41,13 @@ use parquet_variant::{
4141
use std::borrow::Cow;
4242
use std::sync::Arc;
4343

44-
/// Returns the raw bytes at the given index from a binary-like array.
45-
///
46-
/// # Panics
47-
/// Panics if the array is not `Binary`, `LargeBinary`, or `BinaryView`,
48-
/// or if `index` is out of bounds.
49-
pub(crate) fn binary_array_value(array: &dyn Array, index: usize) -> &[u8] {
44+
/// Returns the raw bytes at the given index from a binary-like array, return `None` if the array isn't binary-like.
45+
pub(crate) fn binary_array_value(array: &dyn Array, index: usize) -> Option<&[u8]> {
5046
match array.data_type() {
51-
DataType::Binary => array.as_binary::<i32>().value(index),
52-
DataType::LargeBinary => array.as_binary::<i64>().value(index),
53-
DataType::BinaryView => array.as_binary_view().value(index),
54-
other => panic!("Expected Binary, LargeBinary, or BinaryView array, got {other}"),
47+
DataType::Binary => Some(array.as_binary::<i32>().value(index)),
48+
DataType::LargeBinary => Some(array.as_binary::<i64>().value(index)),
49+
DataType::BinaryView => Some(array.as_binary_view().value(index)),
50+
_ => None,
5551
}
5652
}
5753

@@ -401,8 +397,10 @@ impl VariantArray {
401397
}
402398
// Otherwise fall back to value, if available
403399
(_, Some(value)) if value.is_valid(index) => {
404-
let metadata = binary_array_value(self.metadata.as_ref(), index);
405-
let value = binary_array_value(value.as_ref(), index);
400+
let metadata = binary_array_value(self.metadata.as_ref(), index)
401+
.expect("metadata field must be a binary-like array");
402+
let value = binary_array_value(value.as_ref(), index)
403+
.expect("value field must be a binary-like array");
406404
Ok(Variant::new(metadata, value))
407405
}
408406
// It is technically invalid for neither value nor typed_value fields to be available,
@@ -986,7 +984,8 @@ fn typed_value_to_variant<'a>(
986984
Ok(Uuid::from_slice(value).unwrap().into()) // unwrap is safe: slice is always 16 bytes
987985
}
988986
DataType::Binary | DataType::LargeBinary | DataType::BinaryView => {
989-
let value = binary_array_value(typed_value.as_ref(), index);
987+
let value = binary_array_value(typed_value.as_ref(), index)
988+
.expect("match arm guarantees the array is binary-like");
990989
Ok(Variant::from(value))
991990
}
992991
DataType::Utf8 => {

0 commit comments

Comments
 (0)