Preserve field name when casting List

[timsaucer]

Which issue does this PR close?

Closes #13481

This also pertains to delta-io/delta-rs#2886

Rationale for this change

During a cast operation, the field name for a list can be changed to "item" from whatever it is previously. This change adds an option to set the field name.

What changes are included in this PR?

Adds an option for array_into_list_array and the other variants to pass in the field name for the schema.

Are these changes tested?

Unit tests added and tested against the example in #13481

Are there any user-facing changes?

Internal change only. No user facing changes.

[gruuya]

Thanks for looking into this!

FWIW, I've also been trying to see what the problem is, and the following is a minimal repro (test case) along the lines of the delta-rs test I've been able to come up with

    #[tokio::test]
    async fn test_list_item() -> Result<()> {
        use datafusion_functions_nested::expr_fn::make_array;

        let element_field = Arc::new(Field::new("element", DataType::Int32, true));
        let items_field = Field::new(
            "items",
            DataType::List(element_field.clone()),
            true,
        );
        let schema = Schema::new(vec![items_field.clone()]);

        let mut items_builder =
            ListBuilder::new(Int32Builder::new()).with_field(element_field.clone());
        items_builder.append_value([Some(1)]);
        let batch = RecordBatch::try_new(Arc::new(schema), vec![Arc::new(items_builder.finish())])?;

        let ctx = SessionContext::new();
        let df = ctx.read_batch(batch).expect("source DataFrame")
            .with_column("condition", lit(false))?
            .select(vec![case(col("condition")).when(lit(false), make_array(vec![lit(2), lit(3)])).otherwise(col("items"))?.alias("items")])?;


        let _ = df.create_physical_plan().await?;

        Ok(())
    }

This might be a red herring, but I'm also looking at whether the issue arises somewhere in TypeCoercion analyzer, specifically in coerce_case_expression. This is because the CASE clause here is crucial for the repro, as without it the outermost plan doesn't get coerced to the child plan's list type (i.e. the one with items named element), but instead retains the types of the expression itself (with the generic item names).

[findepi]

During a cast operation, the field name for a list can be changed to "item" from whatever it is previously. This change adds an option to set the field name.

option, or is it always the case?

btw in #12622 we probably would want to have all List(x) to not be discern by their field name. cc @notfilippo @alamb

[timsaucer]

@findepi I put it as an option because we also use these same functions to build arrays from things like iterators that don't have an associated field name.

[gruuya]

I'm also looking at whether the issue arises somewhere in TypeCoercion analyzer

Ok, looks like TypeCoercion works as intended, and it seems like SimplifyExpressions is the culprit behind the particular problem in delta-rs. From the new issue #13481 I've opened:

Basically TypeCoercion injects the CAST, while SimplifyExpressions omits it. Consequently OptimizeProjections folds the two projections, but the schema is computed from the outermost projection expressions, which now lack the proper type info (i.e. the non-standard list item field names).

EDIT: Oh, I see now that the proposed fix in this PR resolves the bug with SimplifyExpressions described in #13481

[timsaucer]

Ok, this should be good to go. I did test it against the examples shown in #13481 also. It does resolve the test_list_item test but not the test_simplify_case_cast_list. I will try to investigate that under a different PR.

[gruuya]

but not the test_simplify_case_cast_list

Just a note that it should only really make the second assert pass (the one asserting on the output data type), not the first (i.e. the cast should be transmuted into the output data type).

[gruuya]

Maybe make this Option<impl Into<String>> to align with the type in Field::new?

[timsaucer]

We can but that means we'd now have to add type annotations to everywhere we call this, including those that are going to pass None because they don't have field elements. I think this would add bloat that doesn't really add much value. If you feel strongly, I'll make the adjustment.

[gruuya]

Yeah fair point, let's keep it as is.

[alamb]

Since this function is public I don't think we can change its signature in a minor release

https://docs.rs/datafusion/latest/datafusion/common/utils/fn.array_into_list_array.html

Thus I suggest we keep the existing functions but mark them as deprecated and add a new API

Maybe we can do something builder style to make future modifications easier

let list_array = ListArrayWrapper::new(arr)
  .with_nullable(false)
  .with_field(field)

Or something -- that would also give us a way to take the FieldRef directly as well as document what the defaults are 🤔

[gruuya]

I know this is not a concern of this PR, but looks as if this naming convention should be formalized/centralized somewhere (at least a constant), since otherwise there's a whole lot of "item" literals throughout the code.

[alamb]

100% agree -- I think it should be done in arrow-rs

Maybe we could add some comments on https://docs.rs/arrow/latest/arrow/datatypes/struct.Field.html#method.new_list or something

[gruuya]

I think it should be done in arrow-rs

Yeah, makes sense—I can pick that up if there's no objection, maybe something like this

impl Field {
+    /// Default list member field name
+    pub const LIST_FIELD_DEFAULT_NAME: &'static str = "item";
+
     /// Creates a new field with the given name, type, and nullability
     pub fn new(name: impl Into<String>, data_type: DataType, nullable: bool) -> Self {
         Field {
@@ -144,7 +147,7 @@ impl Field {
     /// );
     /// ```
     pub fn new_list_field(data_type: DataType, nullable: bool) -> Self {
-        Self::new("item", data_type, nullable)
+        Self::new(Self::LIST_FIELD_DEFAULT_NAME, data_type, nullable)
     }

could add some comments

I think new_list_field already has an adequate comment (Field::new_list probably doesn't need one since the name is explicitly passed there).

[alamb]

at the very least having a default constant exposed would be a nice improvement I think

[timsaucer]

Are we okay to merge?

[alamb]

The code looks good and makes sense to me -- than you @timsaucer and @gruuya

My only concern is that we won't be able to release this change to 43.0.0 as it would be a breaking API change.

[alamb]

at the very least having a default constant exposed would be a nice improvement I think

[alamb]

Since this function is public I don't think we can change its signature in a minor release

https://docs.rs/datafusion/latest/datafusion/common/utils/fn.array_into_list_array.html

Thus I suggest we keep the existing functions but mark them as deprecated and add a new API

Maybe we can do something builder style to make future modifications easier

let list_array = ListArrayWrapper::new(arr)
  .with_nullable(false)
  .with_field(field)

Or something -- that would also give us a way to take the FieldRef directly as well as document what the defaults are 🤔

[timsaucer]

Since it would be a breaking change and there is only one place that we internally need this feature, I just switched it to have a different function. I can change over to a builder method if you like, but I wasn't convinced this small use needed it. Happy to support if you think that's valuable.