[grorge123]

Which issue does this PR close?

Closes #7380.

Rationale for this change

When the input of GetStructField is a scalar struct, the native kernel returns the field as a one-row array. Beside a column, that array does not match the batch's row count and the query fails. The scalar arm has done this since the expression was added, but a scalar struct rarely reached it until #5452 made CreateNamedStruct return a scalar when every field value is a scalar. A DataFusion scalar UDF would have its one-row result turned back into a scalar by ScalarFunctionExpr, but GetStructField is a PhysicalExpr and gets no such conversion. Split out of #5526.

What changes are included in this PR?

GetStructField returns a scalar when its input is a scalar struct. The field still goes through child_with_parent_nulls, so the field of a null struct is null. A list field is wrapped as a list scalar directly rather than through ScalarValue::try_from_array, which rebuilds the element field without its metadata (such as a Variant element's extension type), so the scalar's type would no longer match the declared type.

How are these changes tested?

• A query at the end of struct/get_struct_field.sql reads a field of a cast named_struct over a scalar subquery, once with a value and once with NULL, over four rows in one batch. It fails on main with Array length 1 does not match expected length 4.
• Rust unit tests evaluate GetStructField over a scalar struct and check that it returns a scalar, for a present struct, a null struct, and a null struct whose child still holds a value, and for a typed and a Null-typed field. Another checks that a list field whose element field carries metadata keeps the declared type. Each test fails without its part of the fix.

Assisted-by: Claude Code (claude-opus-5-5)

[sunchao]

I reproduced the four-row length error with the previous GetStructField implementation. On ddc787a, the Spark 4.1.3 get_struct_field.sql fixture passed locally, as did all five focused Rust tests. The local JNI build used --no-default-features. I found one remaining type mismatch in the native expression API.

[sunchao]

Preserve the element field for ListView and LargeListView too. I evaluated GetStructField on a one-row Literal(Struct) whose list-view element had ARROW:extension:name metadata. For both offset widths, expr.data_type() retained the metadata but expr.evaluate() returned a scalar without it. The base expression preserved it on the same input. DataFusion 55.2 rebuilds these two views in try_from_array just as it does the other list types. Wrapping their one-row arrays directly and extending the existing metadata test to both widths would keep the declared and evaluated types consistent. I reproduced this through the native PhysicalExpr API. Spark’s standard ArrayType maps to List, and I found no Spark SQL producer for this case.

[grorge123]

Thanks, I could reproduce that. ListView and LargeListView now wrap the one-row array directly, like the other list types. The metadata test now covers List, ListView and LargeListView. With the two new arms removed, it fails on ListView because the evaluated type has no metadata.

[rich7420]

LGTM. Twelve focused native tests passed; restoring the old scalar branch fails the regression. Spark integration and current-head CI are unverified.

[comphead]

Thanks @grorge123 for the fix. I checked the sibling accessors: GetArrayStructFields and ListExtract already go through into_array(batch.num_rows()), so a scalar input is broadcast there and they do not share this bug.

[comphead]

This only reads an integer field, so the list arms of single_row_scalar and the struct and map arms that go through try_from_array are not reached from SQL. Could the fixture also read an array field, a nested struct field and a map field of the same kind of cast named_struct over the scalar subquery, for example CAST(named_struct('l', array((SELECT max(a) FROM test_struct_scalar))) AS STRUCT<l: ARRAY<BIGINT>>).l, named_struct('s', named_struct('y', <subquery>)) and named_struct('m', map('k', <subquery>))? I expect CreateNamedStruct to return a scalar struct for these too since every field value is a scalar, but I have not run them. A NULL struct would also be worth a row if IF(<scalar subquery> > 100, named_struct(...), NULL) stays a scalar natively. Today the null-struct case is only covered by the Rust test.

[grorge123]

Thanks. All three run natively on a scalar struct and fail on main with the same length error, so I added them to get_struct_field.sql, one query each. For the map I used map_from_arrays(array('k'), array(<subquery>)): Comet runs map() through codegen dispatch, which does not take a scalar subquery, so with map() the query ran in Spark.

The IF(<subquery> > 100, named_struct(...), NULL) form did not reach native GetStructField at first. PushFoldableIntoBranches pushes .x into both branches because the NULL branch is foldable. With that rule excluded, native gets a null scalar struct and main fails the same way, so the file now excludes it and has that query, with a non-null branch next to it. The earlier queries in the file still pass, with a native GetStructField, when the rule is excluded.

These queries now use expect_native(getstructfield), so a rewrite that removes the field access fails the test instead of passing it. Without the ExcludeRules line, the null struct query fails that check.

[comphead]

This loop only compares value.data_type() for a present list, so it would still pass if the wrapped list held the wrong row or lost the parent's nulls. Could it also assert the value (the [1, 2] list) and add a null-struct case for each list width, since that is where child_with_parent_nulls feeds the directly wrapped array?

[grorge123]

Done. The test now covers every list type that single_row_scalar wraps directly: List, LargeList, FixedSizeList, ListView and LargeListView. For each one it checks that the value is the [1, 2] list, and that a null struct whose child slot still holds the list gives a null of the declared type. It fails on the null struct when the scalar arm skips child_with_parent_nulls, and on the value when the List arm wraps a different list.

[sunchao]

The direct ListView and LargeListView cases from my earlier review are fixed. On Spark 4.1.3 I ran the checked-in SQL fixture and two additional fixtures covering array, nested struct, and map fields with a locally built cd95b17 JNI library (--no-default-features). All three passed. For the map case I put a constant map and a scalar subquery in sibling struct fields so the projection stayed native. I found one remaining schema mismatch through the Rust expression API.

[sunchao]

Preserve the list element field when extracting a dictionary scalar. I used CreateNamedStruct with a literal Dictionary<Int32, List<Int32>> whose list element carries ARROW:extension:name, then evaluated GetStructField on a one-row batch. The dictionary reaches this fallback, where DataFusion recursively converts its list value and drops the element metadata. data_type() still declares that metadata, so RecordBatch::try_new rejects the resulting column. The same batch validates on base. Could the dictionary conversion extract the referenced value through the same metadata-preserving path and retain the key type? A regression test can assert that the result constructs a batch with the expression’s declared schema. I reproduced this through the native API and found no Spark SQL producer for this encoding.

[grorge123]

Thanks, I could reproduce that: before the change, RecordBatch::try_new rejects the column with "column types must match schema types". A dictionary now converts the value its key points at through single_row_scalar, so a list value is wrapped directly, and it keeps its key type. A null key gives a null of the value type, which DataFusion builds from the list's field, so that keeps the metadata too. The new test builds a batch with the declared schema for a present key, a null key and a null struct, and checks that the key type is still Int32 and the value is the list the key points at.