sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836460165
##########
spark/src/main/scala/org/apache/comet/DataTypeSupport.scala:
##########
@@ -54,8 +54,9 @@ trait DataTypeSupport {
CalendarIntervalType =>
true
case StructType(fields) =>
- fields.nonEmpty && fields.forall(f =>
- isTypeSupported(f.dataType, f.name, fallbackReasons))
+ // A struct's `fields` can be empty -- e.g. Iceberg's `_partition`
metadata column is
+ // exactly that on an unpartitioned table. It's still a value Comet
can represent.
+ fields.forall(f => isTypeSupported(f.dataType, f.name,
fallbackReasons))
Review Comment:
[P2] Guard empty structs in FIRST_VALUE/LAST_VALUE windows
This also admits empty-struct inputs to native windows that cannot handle
them. Using the `marker: struct<>` LocalRelation from the new tests, enable
`spark.comet.exec.localTableScan.enabled=true` with native shuffle and run
`SELECT first_value(marker) OVER () FROM t` (no `ORDER BY`). The input can now
stay native through `CometWindowExec`, which maps this to DataFusion 54.1's
`FirstValue`. Its accumulator calls `ScalarValue::compact()`, whose
`compact_view_buffers` struct branch reconstructs the array with
`StructArray::new` even when there are no child fields, causing an Arrow panic.
I reproduced this through the dependency's `WindowExpr::evaluate` on a valid
three-row empty-struct batch; Spark returns three empty structs. `LAST_VALUE`
fails identically, and the compaction is recursive, so
nested/list/map-contained empty structs also fail. The old type gates kept
these inputs on Spark. Please keep these windows on Spark for schemas
containing empty structs until scalar compactio
n is fixed.
##########
native/spark-expr/src/json_funcs/from_json.rs:
##########
@@ -150,27 +150,35 @@ fn json_string_to_struct(arr: &Arc<dyn Array>, schema:
&DataType) -> Result<Arra
} else {
let json_str = string_array.value(row_idx);
- // Parse JSON (PERMISSIVE mode: return null fields on error)
- match serde_json::from_str::<serde_json::Value>(json_str) {
- Ok(json_value) => {
- if let serde_json::Value::Object(obj) = json_value {
- // Struct is not null, extract each field
- *struct_null = true;
- for (field, builder) in
fields.iter().zip(field_builders.iter_mut()) {
- let field_value = obj.get(field.name());
- append_field_value(builder, field, field_value)?;
+ if json_str.trim().is_empty() {
Review Comment:
[P2] Restrict blank-input detection to JSON whitespace
With `spark.comet.expression.JsonToStructs.allowIncompatible=true`, `trim()`
also treats non-JSON whitespace such as NBSP (U+00A0), vertical tab and form
feed as a blank document. For a column containing only NBSP, the exact-head
native expression now makes `from_json(col, 'struct<>') IS NULL` true, while
Spark 3.5.2 and 4.0.4 return a non-null empty struct. This also regresses
already-supported schemas such as `a INT`: both the base and previous native
head return the non-null, all-null-fields struct, but this new branch returns
SQL NULL. Restrict the blank check to JSON whitespace (space, tab, CR and LF)
and add a non-JSON-whitespace regression row alongside the ordinary blank-input
cases.
--
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]
