Repository navigation
Conversation
A scalar struct input returned its field as a one-row array, which does not match the batch's row count beside a column. Return a scalar instead, wrapping a list field directly so its element field keeps its metadata. Assisted-by: Claude Code (claude-opus-5-5)
sunchao
left a comment
There was a problem hiding this comment.
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.
| DataType::FixedSizeList(_, _) => { | ||
| ScalarValue::FixedSizeList(Arc::new(array.as_fixed_size_list().clone())) | ||
| } | ||
| _ => ScalarValue::try_from_array(&array, 0)?, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Assisted-by: Claude Code (claude-opus-5-5)
rich7420
left a comment
There was a problem hiding this comment.
LGTM. Twelve focused native tests passed; restoring the old scalar branch fails the regression. Spark integration and current-head CI are unverified.
comphead
left a comment
There was a problem hiding this comment.
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.
| INSERT INTO test_struct_scalar SELECT /*+ REPARTITION(1) */ * FROM VALUES (1), (-1), (NULL), (4) AS v(a) | ||
|
|
||
| query | ||
| SELECT a, CAST(named_struct('x', (SELECT max(a) FROM test_struct_scalar)) AS STRUCT<x: BIGINT>).x, CAST(named_struct('x', (SELECT max(a) FROM test_struct_scalar WHERE a > 100)) AS STRUCT<x: BIGINT>).x FROM test_struct_scalar |
There was a problem hiding this comment.
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.
| for list in lists { | ||
| let fields = Fields::from(vec![Field::new("l", list.data_type().clone(), true)]); | ||
| let scalar = ScalarValue::Struct(Arc::new(StructArray::new(fields, vec![list], None))); | ||
| let expr = GetStructField::new(Arc::new(Literal::new(scalar)), 0); | ||
| match expr.evaluate(&batch).unwrap() { | ||
| ColumnarValue::Scalar(value) => { | ||
| assert_eq!(value.data_type(), expr.data_type(&schema).unwrap()) | ||
| } | ||
| ColumnarValue::Array(array) => panic!("expected a scalar, got array {array:?}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
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?
sunchao
left a comment
There was a problem hiding this comment.
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.
| DataType::LargeListView(_) => { | ||
| ScalarValue::LargeListView(Arc::new(array.as_list_view::<i64>().clone())) | ||
| } | ||
| _ => ScalarValue::try_from_array(&array, 0)?, |
There was a problem hiding this comment.
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.
Which issue does this PR close?
Closes #7380.
Rationale for this change
When the input of
GetStructFieldis 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 madeCreateNamedStructreturn a scalar when every field value is a scalar. A DataFusion scalar UDF would have its one-row result turned back into a scalar byScalarFunctionExpr, butGetStructFieldis aPhysicalExprand gets no such conversion. Split out of #5526.What changes are included in this PR?
GetStructFieldreturns a scalar when its input is a scalar struct. The field still goes throughchild_with_parent_nulls, so the field of a null struct is null. A list field is wrapped as a list scalar directly rather than throughScalarValue::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?
struct/get_struct_field.sqlreads a field of a castnamed_structover a scalar subquery, once with a value and once with NULL, over four rows in one batch. It fails onmainwithArray length 1 does not match expected length 4.GetStructFieldover 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)