gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3865359658
##########
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:
This is sometimes collapsable because take the example I put in the PR
description. You might have this:
```text
ProjectionExec: arrow_metadata(i, 'event_field') AS metadata
ProjectionExec: i@0 AS i
output metadata = {"event_field": "true"}
DataSourceExec: i
metadata = {}
```
The outer query does not have override metadata but the inner does. The
correct result is `true`.
The previous projection logic would collapse these, but the outer projection
expr reads the metadata. This wa the CI failure. So it would make:
```text
ProjectionExec: arrow_metadata(i, 'event_field') AS metadata
DataSourceExec: i
metadata = {}
```
Giving use result as `NULL` now.
This is kinda ocnservative as we could probably just chekc if everything in
the outer projection is just referencing columns but I am just trying to get to
correctness first. Then could do the optimixation. What you think?
--
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]
