andygrove commented on code in PR #5138:
URL: https://github.com/apache/datafusion-comet/pull/5138#discussion_r3685473580
##########
native/core/src/execution/operators/expand.rs:
##########
@@ -65,6 +66,47 @@ impl ExpandExec {
cache,
}
}
+
+ /// Derives the operator's output schema from *every* projection, not just
the first.
+ ///
+ /// Each projection produces the same output columns, so their types agree
except where a
+ /// native kernel drifts on nested field nullability. Widening each
column's nested `nullable`
+ /// flags across all projections gives a schema that is valid for all of
them; deriving it from
+ /// `projections[0]` alone yields a schema that the remaining projections
cannot be stamped
+ /// with. See <https://github.com/apache/datafusion-comet/issues/5137>.
+ pub fn build_schema(
+ projections: &[Vec<Arc<dyn PhysicalExpr>>],
+ input_schema: &SchemaRef,
+ ) -> Result<SchemaRef, DataFusionError> {
+ let mut data_types = projections
+ .first()
+ .ok_or_else(|| {
+ DataFusionError::Internal("Expand should have at least one
projection".to_string())
+ })?
+ .iter()
+ .map(|expr| expr.data_type(input_schema))
+ .collect::<Result<Vec<_>, _>>()?;
+
+ for projection in &projections[1..] {
+ if projection.len() != data_types.len() {
+ return Err(DataFusionError::Internal(format!(
+ "Expand projections produce differing column counts: {}
and {}",
+ data_types.len(),
+ projection.len()
+ )));
+ }
+ for (data_type, expr) in data_types.iter_mut().zip(projection) {
+ *data_type = widen_nested_nullability(data_type,
&expr.data_type(input_schema)?);
+ }
+ }
+
+ let fields: Vec<Field> = data_types
+ .into_iter()
+ .enumerate()
+ .map(|(idx, dt)| Field::new(format!("col_{idx}"), dt, true))
Review Comment:
Not strictly required for correctness, but I kept it deliberately.
`col_{idx}` is exactly what the planner produced before this logic moved
into `build_schema`, so keeping it means the only behavior change here is the
nullability derivation. There is also no better name available: an Expand
projection is an arbitrary `PhysicalExpr` (including literals like the grouping
ID), so there is nothing to inherit — and parent operators bind to these
columns by index, with a bound reference taking its name from the child schema
(`planner.rs`, `ExprStruct::Bound`), so the name is positional either way. It
is only visible in explain output.
Added a comment at the call site recording that.
--
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]
