comphead commented on code in PR #5192:
URL: https://github.com/apache/datafusion-comet/pull/5192#discussion_r3700149112
##########
native/core/src/execution/planner.rs:
##########
@@ -1838,14 +1839,74 @@ impl PhysicalPlanner {
self.create_plan(&children[0], inputs, partition_count)?;
// Create the expression for the array to explode
- let child_expr = if let Some(child_expr) = &explode.child {
+ let raw_child_expr = if let Some(child_expr) = &explode.child {
self.create_expr(child_expr, child.schema())?
} else {
return Err(ExecutionError::GeneralError(
"Explode operator requires a child
expression".to_string(),
));
};
+ let child_schema = child.schema();
+ let child_field_name = raw_child_expr
+ .return_field(&child_schema)
+ .expect("Failed to get field from child expression")
+ .name()
+ .to_string();
+
+ // Bridge Spark's outer semantics: DataFusion's `UnnestExec`
with
+ // `preserve_nulls = true` emits one null row for a NULL list
but drops rows
+ // whose list is empty. Spark's
`explode_outer`/`posexplode_outer` must emit
+ // exactly one null row in both cases, so we mark empty rows
as null before
+ // unnesting. See
https://github.com/apache/datafusion/issues/19053. Once
+ // that upstream fix lands, `ListEmptyToNullExpr` and the
pre-projection
+ // below can be removed (TODO: link the Comet tracking issue
here).
Review Comment:
more correctly would be to track #5210 though, I can make it follow up PR
##########
native/core/src/execution/planner.rs:
##########
@@ -1838,14 +1839,74 @@ impl PhysicalPlanner {
self.create_plan(&children[0], inputs, partition_count)?;
// Create the expression for the array to explode
- let child_expr = if let Some(child_expr) = &explode.child {
+ let raw_child_expr = if let Some(child_expr) = &explode.child {
self.create_expr(child_expr, child.schema())?
} else {
return Err(ExecutionError::GeneralError(
"Explode operator requires a child
expression".to_string(),
));
};
+ let child_schema = child.schema();
+ let child_field_name = raw_child_expr
+ .return_field(&child_schema)
+ .expect("Failed to get field from child expression")
+ .name()
+ .to_string();
+
+ // Bridge Spark's outer semantics: DataFusion's `UnnestExec`
with
+ // `preserve_nulls = true` emits one null row for a NULL list
but drops rows
+ // whose list is empty. Spark's
`explode_outer`/`posexplode_outer` must emit
+ // exactly one null row in both cases, so we mark empty rows
as null before
+ // unnesting. See
https://github.com/apache/datafusion/issues/19053. Once
+ // that upstream fix lands, `ListEmptyToNullExpr` and the
pre-projection
+ // below can be removed (TODO: link the Comet tracking issue
here).
Review Comment:
more correctly would be to track #5210 though, I can address it in follow up
PR
--
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]
