Skip to content

Conversation

@Weijun-H
Copy link
Contributor

@Weijun-H Weijun-H commented Nov 29, 2025

parts of #3

What changes in this pr?

  • Single-argument casts now delegate to parquet_variant_compute::cast_to_variant, so all Arrow types that kernel supports (strings/string views/large strings, fixed-size binary, numerics, temporal, lists/structs/maps/unions/etc.) work.
    Existing Variant struct arrays are detected via VariantArray::try_new and passed through unchanged.

  • Two-argument metadata/value path kept as-is (binary-only).

  • Added return_field_from_args to emit a field with the VariantType extension and the canonical metadata/value struct shape.

  • New tests cover string arrays (StringArray, StringViewArray), fixed-size binary (16-byte), return-field extension tagging, plus prior scalar/array numeric and binary cases.

Copy link
Contributor

@sdf-jkl sdf-jkl left a comment

Choose a reason for hiding this comment

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

Everything looks great!

fn from_array(_array: &ArrayRef) -> Result<ColumnarValue> {
todo!()
fn from_array(array: &ArrayRef) -> Result<ColumnarValue> {
// If the array is already a Variant array, pass it through unchanged
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

ScalarValue::UInt64(i) => build_variant_array(i.as_ref().map(|i| *i)),

_ => todo!(),
if let ScalarValue::Struct(struct_array) = scalar_value
Copy link
Contributor

Choose a reason for hiding this comment

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

very clean.


use super::*;

#[test]
Copy link
Contributor

@sdf-jkl sdf-jkl Dec 3, 2025

Choose a reason for hiding this comment

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

tests look great!

Should we include other types in a next PR? (null, bool, decimal, timestamp/date/time, non-primitive ones like struct, list, map, union, dict, REE)

Copy link
Member

Choose a reason for hiding this comment

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

Here's an issue to track test coverage in cast_to_variant

Copy link
Member

@friendlymatthew friendlymatthew left a comment

Choose a reason for hiding this comment

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

Thanks @Weijun-H and @sdf-jkl. And sorry for the late review, I didn't wire up notifications properly)

This change makes sense to me. It would be great to add more test coverage, including both unit tests and SLT tests

@friendlymatthew friendlymatthew merged commit bc39cbb into datafusion-contrib:main Dec 5, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants