gabotechs commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3865163140
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -71,18 +71,31 @@ pub trait ExprSchemable {
-> Result<(DataType, bool)>;
}
-/// Derives the output field for a cast expression from the source field.
+/// Derives the output field for a cast expression from the source and target
+/// fields. Type-only casts preserve source metadata, while an explicit target
+/// field supplies its own metadata.
+///
/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
fn cast_output_field(
source_field: &FieldRef,
- target_type: &DataType,
+ target_field: &FieldRef,
force_nullable: bool,
) -> Arc<Field> {
+ // `Cast::new` and `TryCast::new` use this field when only a target
+ // type is known. In that case, retain the source field's metadata.
+ let type_only_target = target_field.name().is_empty()
+ && target_field.is_nullable()
+ && target_field.metadata().is_empty();
+ let metadata = if type_only_target {
Review Comment:
Why is it not sufficient to do something simpler like:
Does `target_field` have metadata? if yes, then use that metadata, if not,
then use the `source_field` metadata.
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1326,17 +1340,44 @@ pub fn update_join_filter(
})
}
+/// Returns whether a projection defines metadata that its expressions and
input
+/// schema cannot reproduce.
+///
+/// Such a projection is an execution boundary: a parent expression such as
+/// `arrow_metadata` can observe its output field metadata.
+fn projection_overrides_metadata(projection: &ProjectionExec) -> Result<bool> {
Review Comment:
Nit: a `ProjectionExec::overrides_metadata` method rather than a standalone
function sounds very slightly more elegant.
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1326,17 +1340,44 @@ pub fn update_join_filter(
})
}
+/// Returns whether a projection defines metadata that its expressions and
input
+/// schema cannot reproduce.
+///
+/// Such a projection is an execution boundary: a parent expression such as
+/// `arrow_metadata` can observe its output field metadata.
+fn projection_overrides_metadata(projection: &ProjectionExec) -> Result<bool> {
+ let derived_schema = projection
+ .projector
+ .projection()
+ .project_schema(projection.input().schema().as_ref())?;
+ let output_schema = projection.schema();
+ Ok(derived_schema.metadata() != output_schema.metadata()
+ || derived_schema
+ .fields()
+ .iter()
+ .zip(output_schema.fields())
+ .any(|(derived, output)| derived.metadata() != output.metadata()))
+}
+
/// Collapse a chain of consecutive [`ProjectionExec`]s into one. Returns
/// `None` if nothing could be merged.
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if projection_overrides_metadata(outer)? {
+ return Ok(None);
+ }
+
let mut current_exprs: Vec<ProjectionExpr> = outer.expr().to_vec();
let mut current_input: Arc<dyn ExecutionPlan> = Arc::clone(outer.input());
let mut column_ref_map: HashMap<Column, usize> = HashMap::new();
let mut collapsed_any = false;
'outer: while let Some(inner_proj) =
current_input.downcast_ref::<ProjectionExec>() {
+ if projection_overrides_metadata(inner_proj)? {
+ break;
+ }
+
// Collect the column references usage in the outer projection.
Review Comment:
Imagine this situation:
```
ProjectionExec: <- does not override metadata
ProjectionExec: <- overrides metadata
```
This is collapsible right? but the current code will omit collapsing it.
--
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]
