Add support for tinystr, rust_decimal, and glam types

[Swoorup]

• Implement Arrow serialization/deserialization for TinyAsciiStr, Decimal, and glam vector/matrix types
• Add new optional dependencies and features

[gooroo-dev]

Please double check the following review of the pull request:

Issues counts

🐞Mistake	🤪Typo	🚨Security	🚀Performance	💪Best Practices	📖Readability	❓Others
1	0	0	0	1	1	0

Changes in the diff

• ➕ Added support for tinystr, rust_decimal, and glam types.
• ➕ Added feature flags for tinystr, rust_decimal, and glam in Cargo.toml.
• ➕ Implemented ArrowField, ArrowSerialize, and ArrowDeserialize for tinystr, rust_decimal, and glam types.
• ➕ Added tests for tinystr, rust_decimal, and glam types.
• 📖 Improved code organization by creating a features module.

Identified Issues

ID	Type	Details	Severity	Confidence
1	🐞Mistake	Missing newline at the end of Cargo.toml file.	🟡Low	🔴High
2	💪Best Practices	Use of unwrap() in tests can cause panics.	🟠Medium	🔴High
3	📖Readability	Lack of comments explaining the purpose of macros in glam.rs.	🟡Low	🟠Medium

Issue 1: Missing newline at the end of Cargo.toml file

Details: The Cargo.toml file is missing a newline at the end of the file. This is a common best practice to ensure compatibility with various tools and editors.

File Path: Cargo.toml

Lines of Code:

+glam = "0.29"
\ No newline at end of file

Fix:

+glam = "0.29"
+

Explanation: Adding a newline at the end of the file ensures compatibility with various tools and editors.

Issue 2: Use of unwrap() in tests can cause panics

Details: The use of unwrap() in tests can cause the tests to panic if an error occurs. It's better to use expect() with a descriptive error message.

File Path: arrow_convert/tests/test_glam.rs, arrow_convert/tests/test_rust_decimal.rs, arrow_convert/tests/test_tinystr.rs

Lines of Code:

let arrow_array: ArrayRef = original.try_into_arrow().unwrap();

Fix:

let arrow_array: ArrayRef = original.try_into_arrow().expect("Failed to convert to Arrow array");

Explanation: Using expect() with a descriptive error message helps to identify the cause of the failure more easily.

Issue 3: Lack of comments explaining the purpose of macros in glam.rs

Details: The macros in glam.rs are not documented, which can make it difficult for other developers to understand their purpose and usage.

File Path: arrow_convert/src/features/glam.rs

Lines of Code:

macro_rules! impl_glam_ty {
    // ...
}

macro_rules! impl_glam_vec_bool {
    // ...
}

macro_rules! impl_glam_vec_f32 {
    // ...
}

macro_rules! impl_glam_mat_f32 {
    // ...
}

macro_rules! impl_glam_vec_f64 {
    // ...
}

macro_rules! impl_glam_mat_f64 {
    // ...
}

Fix:

/// Macro to implement ArrowField, ArrowSerialize, and ArrowDeserialize for a given glam type.
macro_rules! impl_glam_ty {
    // ...
}

/// Macro to implement ArrowField, ArrowSerialize, and ArrowDeserialize for glam boolean vectors.
macro_rules! impl_glam_vec_bool {
    // ...
}

/// Macro to implement ArrowField, ArrowSerialize, and ArrowDeserialize for glam float32 vectors.
macro_rules! impl_glam_vec_f32 {
    // ...
}

/// Macro to implement ArrowField, ArrowSerialize, and ArrowDeserialize for glam float32 matrices.
macro_rules! impl_glam_mat_f32 {
    // ...
}

/// Macro to implement ArrowField, ArrowSerialize, and ArrowDeserialize for glam float64 vectors.
macro_rules! impl_glam_vec_f64 {
    // ...
}

/// Macro to implement ArrowField, ArrowSerialize, and ArrowDeserialize for glam float64 matrices.
macro_rules! impl_glam_mat_f64 {
    // ...
}

Explanation: Adding comments to the macros helps other developers understand their purpose and usage.

Missing Tests

The current tests cover the basic roundtrip serialization and deserialization for tinystr, rust_decimal, and glam types. However, additional edge case tests can be added to ensure robustness.

Additional Tests:

1. Test for tinystr with maximum length:

#[cfg(feature = "tinystr")]
#[test]
fn test_tinyasciistr_max_length() {
    use arrow::array::{Array, ArrayRef, FixedSizeBinaryArray};
    use arrow_convert::deserialize::TryIntoCollection;
    use arrow_convert::serialize::TryIntoArrow;
    use tinystr::TinyAsciiStr;

    let original: Vec<TinyAsciiStr<10>> = vec![
        TinyAsciiStr::from_str("ABCDEFGHIJ").unwrap(),
    ];

    let arrow_array: ArrayRef = original.try_into_arrow().expect("Failed to convert to Arrow array");
    assert!(arrow_array.as_any().is::<FixedSizeBinaryArray>());
    let fixed_size_array = arrow_array.as_any().downcast_ref::<FixedSizeBinaryArray>().unwrap();
    assert_eq!(fixed_size_array.value_length(), 10);

    let roundtrip: Vec<TinyAsciiStr<10>> = arrow_array.try_into_collection().expect("Failed to convert from Arrow array");
    assert_eq!(original, roundtrip);
}

2. Test for rust_decimal with edge values:

#[cfg(feature = "rust_decimal")]
#[test]
fn test_decimal_edge_values() {
    use arrow::array::{Array, ArrayRef};
    use arrow::datatypes::DECIMAL_DEFAULT_SCALE;
    use arrow::{array::Decimal128Array, datatypes::DECIMAL128_MAX_PRECISION};
    use arrow_convert::deserialize::TryIntoCollection;
    use arrow_convert::serialize::*;
    use rust_decimal::Decimal;

    let original: Vec<Decimal> = vec![
        Decimal::new(i64::MAX, DECIMAL_DEFAULT_SCALE),
        Decimal::new(i64::MIN, DECIMAL_DEFAULT_SCALE),
        Decimal::new(0, DECIMAL_DEFAULT_SCALE),
    ];

    let arrow_array: ArrayRef = original.try_into_arrow().expect("Failed to convert to Arrow array");
    assert!(arrow_array.as_any().is::<Decimal128Array>());

    let decimal_array = arrow_array.as_any().downcast_ref::<Decimal128Array>().unwrap();
    assert_eq!(decimal_array.precision(), DECIMAL128_MAX_PRECISION);
    assert_eq!(decimal_array.scale(), DECIMAL_DEFAULT_SCALE);

    let roundtrip: Vec<Decimal> = arrow_array.try_into_collection().expect("Failed to convert from Arrow array");
    assert_eq!(original, roundtrip);
}

3. Test for glam with edge values:

#[cfg(feature = "glam")]
#[test]
fn test_glam_edge_values() {
    use glam::*;
    use arrow::array::{Array, ArrayRef};
    use arrow_convert::deserialize::TryIntoCollection;
    use arrow_convert::serialize::TryIntoArrow;
    use arrow_convert::{ArrowDeserialize, ArrowField, ArrowSerialize};
    use pretty_assertions::assert_eq;

    #[derive(Debug, Clone, PartialEq, ArrowField, ArrowSerialize, ArrowDeserialize)]
    pub struct GlamObj {
        a1: Vec2,
        a2: Vec3,
        a3: Vec4,
        b1: DVec2,
        b2: DVec3,
        b3: DVec4,
        c1: BVec2,
        c2: BVec3,
        c3: BVec4,
        m1: Mat2,
        m2: Mat3,
        m3: Mat4,
        dm1: DMat2,
        dm2: DMat3,
        dm3: DMat4,
    }

    let original: Vec<GlamObj> = vec![
        GlamObj {
            a1: Vec2::new(f32::MAX, f32::MIN),
            a2: Vec3::new(f32::MAX, f32::MIN, 0.0),
            a3: Vec4::new(f32::MAX, f32::MIN, 0.0, 1.0),
            b1: DVec2::new(f64::MAX, f64::MIN),
            b2: DVec3::new(f64::MAX, f64::MIN, 0.0),
            b3: DVec4::new(f64::MAX, f64::MIN, 0.0, 1.0),
            c1: BVec2::new(true, false),
            c2: BVec3::new(true, false, true),
            c3: BVec4::new(true, false, true, false),
            m1: Mat2::from_cols_array(&[f32::MAX, f32::MIN, 0.0, 1.0]),
            m2: Mat3::from_cols_array(&[f32::MAX, f32::MIN, 0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0]),
            m3: Mat4::from_cols_array(&[
                f32::MAX, f32::MIN, 0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0,
            ]),
            dm1: DMat2::from_cols_array(&[f64::MAX, f64::MIN, 0.0, 1.0]),
            dm2: DMat3::from_cols_array(&[f64::MAX, f64::MIN, 0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0]),
            dm3: DMat4::from_cols_array(&[
                f64::MAX, f64::MIN, 0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0,
            ]),
        },
    ];

    let arrow_array: ArrayRef = original.try_into_arrow().expect("Failed to convert to Arrow array");
    assert!(arrow_array.as_any().is::<arrow::array::StructArray>());
    let struct_array = arrow_array
        .as_any()
        .downcast_ref::<arrow::array::StructArray>()
        .unwrap();

    assert_eq!(struct_array.num_columns(), 15);

    let roundtrip: Vec<GlamObj> = arrow_array.try_into_collection().expect("Failed to convert from Arrow array");
    assert_eq!(original, roundtrip);
}

These additional tests ensure that the new features handle edge cases and maximum values correctly.

Summon me to re-review when updated! Yours, Gooroo.dev
React or reply to let me know what you think!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.69%. Comparing base (6e31682) to head (ce01e61).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #12   +/-   ##
=======================================
  Coverage   92.69%   92.69%           
=======================================
  Files           9        9           
  Lines        1669     1669           
=======================================
  Hits         1547     1547           
  Misses        122      122           

Flag	Coverage Δ
	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Swoorup]

Good job gooroo.dev