sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836344840
##########
native/spark-expr/src/json_funcs/from_json.rs:
##########
@@ -180,11 +180,15 @@ fn json_string_to_struct(arr: &Arc<dyn Array>, schema:
&DataType) -> Result<Arra
.map(finish_builder)
.collect::<Result<Vec<_>>>()?;
let null_buffer = NullBuffer::from(struct_nulls);
- Ok(Arc::new(StructArray::new(
- fields.clone(),
- arrays,
- Some(null_buffer),
- )))
+ // `StructArray::new` derives its length from the first child array, so it
panics when
+ // `fields` is empty (a legitimate zero-field target schema, e.g.
`from_json(_, 'struct<>')`).
+ // `new_empty_fields` takes the length explicitly instead.
+ let struct_array: ArrayRef = if fields.is_empty() {
+ Arc::new(StructArray::new_empty_fields(num_rows, Some(null_buffer)))
Review Comment:
[P2] Preserve Spark NULLs for blank JSON with an empty struct schema
With `spark.comet.expression.JsonToStructs.allowIncompatible=true`, this
branch now makes `from_json(col, 'struct<>')` execute natively when `col` is
`''` or whitespace. The native parser marks every parse error as a valid
struct, so the exact-head expression produces a non-null `Row()` and
`from_json(col, 'struct<>') IS NULL` is false; Spark 3.5 and 4.0 intentionally
return NULL for blank inputs (SPARK-19543). Before this change the empty schema
took the Spark/codegen path, so this is a newly introduced wrong-result case.
The added test uses only `{}`; please preserve the NULL validity bit for blank
input and add empty/whitespace regression rows.
##########
native/spark-expr/src/json_funcs/from_json.rs:
##########
@@ -393,7 +397,14 @@ fn finish_builder(builder: FieldBuilder) ->
Result<ArrayRef> {
.map(finish_builder)
.collect::<Result<Vec<_>>>()?;
let null_buf = arrow::buffer::NullBuffer::from(null_buffer);
- Arc::new(StructArray::new(fields, nested_arrays, Some(null_buf)))
+ // `StructArray::new` derives its row count from the first child
array; a zero-field
+ // schema (e.g. a `struct<>`-typed field nested inside a larger
schema) has no child
+ // arrays to derive it from, so the count is supplied explicitly
here instead.
+ if fields.is_empty() {
+ Arc::new(StructArray::new_empty_fields(null_buf.len(),
Some(null_buf)))
Review Comment:
[P2] Format this nested constructor so required CI can run
`cargo fmt --all -- --check` now exits 1 on this line; both the pinned base
and previously reviewed head pass. The Linux and macOS build workflows execute
that exact command in their gating lint jobs, so as soon as the currently
action-required workflows are approved every downstream build and test will be
blocked. Please run `cargo fmt` on this call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
