Question about type::Value

[koenichiwa]

Disclaimer:

I'm still getting acquainted with the codebase, and the issue history. This is not an issue per se, but is mainly intended as a question or discussion starter. Please let me know if there is a better place for that, and I'll happily migrate there.

---

I noticed a couple of issues that might share a similar underlying cause:

• Partial impl<'a> From<&'a Value> for ValueRef<'a> causes panics #533
• Implementing the list, struct, map types fully #394

While looking into this, I noticed that in the implementations of the Value and ValueRef enums there are similar todo! and unimplemented! macros. It also seems like the current dynamic type design might allow some unintended behavior.

For example, the following compiles:

#[cfg(test)]
mod test {
    use crate::types::Value;

    #[test]
    fn should_this_compile() {
        let list = Value::List(
            vec![
                false.into(),
                1u8.into(),
                2u32.into(),
                3f64.into(),
            ]
        );
        // It compiles !
    }
}

This allows constructing a Value::List containing heterogeneous element types. I was wondering whether this is expected, or if this is something we’d prefer to prevent at the type level.

---

Has there been any prior exploration into introducing something like a ValueKind trait to model value categories more strictly?

Very roughly, I was thinking along the lines of:

use crate::types::Type;

pub trait ValueKind {
    type Inner;
    fn data_type() -> Type;
    fn inner(&self) -> &Self::Inner;
    fn inner_mut(&mut self) -> &mut Self::Inner;
    fn into_inner(self) -> Self::Inner;
}

With concrete wrappers such as:

pub struct Boolean(bool);

impl ValueKind for Boolean {
    type Inner = bool;

    fn data_type() -> Type {
        Type::Boolean
    }

    fn inner(&self) -> &Self::Inner {
        self.0;
    }

    fn inner_mut(&mut self) -> &mut Self::Inner {
        &mut self.0
    }

    fn into_inner(self) -> Self::Inner {
        self.0
    }
}

pub struct List<T>(Vec<T>);

impl<T> ValueKind for List<T>
where
    T: ValueKind,
{
    type Inner = Vec<T>;

    fn data_type() -> Type {
        Type::List(Box::new(T::data_type()))
    }
    //...
}

This would enforce homogeneous lists at the type level. The same would happen for the Map and Array types (Struct and Enum might be a bit harder to tackle). It would also allow data_type() to recursively construct the full Type

I’m very curious whether:

• This flexibility in Value::List is intentional
• There were prior discussions a similar implementation like this

I’d be happy to explore this further if there’s interest.

[mlafeldt]

Thanks for the well-researched write-up!

The heterogeneity in Value::List(Vec<Value>) is a consequence of Value not carrying child type metadata. For row values decoded from DuckDB/Arrow, list payloads are always typed homogeneously, but this does not prevent user-constructed lists from mixing variants.

A full trait-based approach would be a large breaking change. The smallest incremental step is to add element-type metadata to collection variants, e.g. `List(Type, Vec, and similar for other nested variants, and validate construction/binding paths against that metadata.

Contributions toward fixing the todo!/unimplemented! in #394 and #533 would be very welcome, of course!

[koenichiwa]

Thanks for the feedback! I have some follow up questions if that's ok

The heterogeneity in Value::List(Vec) is a consequence of Value not carrying child type metadata.

I guess you mean crate::type::Value here? Or do you mean that it is because the arrow or duckdb-cpp libraries don't expose a child type?

The smallest incremental step is to add element-type metadata to collection variants, e.g. `List(Type, Vec, and similar for other nested variants, and validate construction/binding paths against that metadata.

That is also a good solution. My personal first instinct would be to leverage Rusts type system here, so illegal states are unpresentable. Let me know if you're interested in any exploration, just to check its feasibility.

I get that large breaking changes are not always an option, so I tried out your solution on the List type, and this part works:

impl Value {
    #[inline]
    pub fn data_type(&self) -> Type {
        match *self {
            // ... rest unchanged
            Self::List(ref t, _) => Type::List(Box::new(t.clone())),
            Self::Union(..) | Self::Struct(..) | Self::Array(..) | Self::Map(..) => todo!(),
            Self::Enum(..) => Type::Enum,
        }
    }
}

But I'm getting a bit confused when I looked into this function

impl<'a> From<&'a Value> for ValueRef<'a> {
    #[inline]
    fn from(value: &'a Value) -> Self {
        match *value {
            // ... rest unchanged
            Value::List(ref t, ref v) => {
                // How should I choose between arrow's ListArray or LargeListArray?
                // Just looking at the list's size doesn't seem like the correct solution.
                // Should this also be stored in the value::List's metadata?
                let list_type: ListType<'a> = todo!();
                // What does the second usize in the ValueRef::List type represent?
                // Is it it's length? Why is that information then duplicated?
                let len: usize = v.len(); 
                ValueRef::List(list_type, len)
            },
        }
    }
}