Skip to content

[Variant] Follow up nits and uncomment test cases #7846

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.

Sign up for GitHub

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

Merged
alamb merged 1 commit into from
Jul 3, 2025
Merged

[Variant] Follow up nits and uncomment test cases #7846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions parquet-variant/benches/variant_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ fn bench_object_list_unknown_schema(c: &mut Criterion) {
});
}

// Creates objects with a homogenous schema (same field names)
// Creates objects with a partially homogenous schema (same field names)
/*
{
"id": &[u8], // Following are common across all objects
Expand All @@ -272,7 +272,7 @@ fn bench_object_list_unknown_schema(c: &mut Criterion) {
"ended": u32,
"span_name": String,

"attributees": {
"attributes": {
// following fields are randomized
}
}
Expand Down
2 changes: 1 addition & 1 deletion parquet-variant/src/decoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ impl TryFrom<u8> for VariantPrimitiveType {
/// Used to unpack offset array entries such as metadata dictionary offsets or object/array value
/// offsets. Also used to unpack object field ids. These are always derived from a two-bit
/// `XXX_size_minus_one` field in the corresponding header byte.
#[derive(Clone, Debug, Copy, PartialEq)]
#[derive(Debug, Clone, Copy, PartialEq)]
pub(crate) enum OffsetSizeBytes {
One = 1,
Two = 2,
Expand Down
2 changes: 1 addition & 1 deletion parquet-variant/src/variant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ impl Deref for ShortString<'_> {
/// [metadata]: VariantMetadata#Validation
/// [object]: VariantObject#Validation
/// [array]: VariantList#Validation
#[derive(Clone, Debug, PartialEq)]
#[derive(Debug, Clone, PartialEq)]
pub enum Variant<'m, 'v> {
/// Primitive type: Null
Null,
Expand Down
4 changes: 2 additions & 2 deletions parquet-variant/src/variant/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use arrow_schema::ArrowError;
const NUM_HEADER_BYTES: usize = 1;

/// A parsed version of the variant array value header byte.
#[derive(Clone, Debug, PartialEq)]
#[derive(Debug, Clone, PartialEq)]
pub(crate) struct VariantListHeader {
num_elements_size: OffsetSizeBytes,
offset_size: OffsetSizeBytes,
Expand Down Expand Up @@ -118,7 +118,7 @@ impl VariantListHeader {
///
/// [valid]: VariantMetadata#Validation
/// [Variant spec]: https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#value-data-for-array-basic_type3
#[derive(Clone, Debug, PartialEq)]
#[derive(Debug, Clone, PartialEq)]
pub struct VariantList<'m, 'v> {
pub metadata: VariantMetadata<'m>,
pub value: &'v [u8],
Expand Down
4 changes: 2 additions & 2 deletions parquet-variant/src/variant/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::utils::{
use arrow_schema::ArrowError;

/// Header structure for [`VariantMetadata`]
#[derive(Clone, Debug, Copy, PartialEq)]
#[derive(Debug, Clone, Copy, PartialEq)]
pub(crate) struct VariantMetadataHeader {
version: u8,
is_sorted: bool,
Expand Down Expand Up @@ -128,7 +128,7 @@ impl VariantMetadataHeader {
///
/// [`Variant`]: crate::Variant
/// [Variant Spec]: https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#metadata-encoding
#[derive(Clone, Copy, Debug, PartialEq)]
#[derive(Debug, Clone, Copy, PartialEq)]
pub struct VariantMetadata<'m> {
bytes: &'m [u8],
header: VariantMetadataHeader,
Expand Down
25 changes: 11 additions & 14 deletions parquet-variant/src/variant/object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use arrow_schema::ArrowError;
const NUM_HEADER_BYTES: usize = 1;

/// Header structure for [`VariantObject`]
#[derive(Clone, Debug, PartialEq)]
#[derive(Debug, Clone, PartialEq)]
pub(crate) struct VariantObjectHeader {
num_elements_size: OffsetSizeBytes,
field_id_size: OffsetSizeBytes,
Expand Down Expand Up @@ -115,7 +115,7 @@ impl VariantObjectHeader {
///
/// [valid]: VariantMetadata#Validation
/// [Variant spec]: https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#value-data-for-object-basic_type2
#[derive(Clone, Debug, PartialEq)]
#[derive(Debug, Clone, PartialEq)]
pub struct VariantObject<'m, 'v> {
pub metadata: VariantMetadata<'m>,
pub value: &'v [u8],
Expand Down Expand Up @@ -397,20 +397,17 @@ mod tests {
let missing_field = variant_obj.get("missing");
assert!(missing_field.is_none());

// https://github.com/apache/arrow-rs/issues/7784
// Fixme: The following assertion will panic! That is not good
Comment on lines -400 to -401
Copy link
Copy Markdown
Contributor

@scovich scovich Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean the underlying bug has indeed been fixed now?

Copy link
Copy Markdown
Contributor Author

@friendlymatthew friendlymatthew Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

// let missing_field_name = variant_obj.field_name(3);
// assert!(missing_field_name.is_none());
//
// Fixme: The `.field_name()` will panic! This is not good
// let missing_field_name = variant_obj.field_name(300);
// assert!(missing_field_name.is_none());
let missing_field_name = variant_obj.field_name(3);
assert!(missing_field_name.is_none());

let missing_field_name = variant_obj.field_name(300);
assert!(missing_field_name.is_none());

// let missing_field_value = variant_obj.field(3);
// assert!(missing_field_value.is_none());
let missing_field_value = variant_obj.field(3);
assert!(missing_field_value.is_none());

// let missing_field_value = variant_obj.field(300);
// assert!(missing_field_value.is_none());
let missing_field_value = variant_obj.field(300);
assert!(missing_field_value.is_none());

// Test fields iterator
let fields: Vec<_> = variant_obj.iter().collect();
Expand Down
Loading