ryux1 commented on code in PR #24984:
URL: https://github.com/apache/datafusion/pull/24984#discussion_r3944377733
##########
datafusion/physical-plan/src/unnest.rs:
##########
@@ -328,6 +333,57 @@ impl ExecutionPlan for UnnestExec {
Some(self.metrics.clone_inner())
}
+ fn gather_filters_for_pushdown(
+ &self,
+ _phase: FilterPushdownPhase,
+ parent_filters: Vec<Arc<dyn PhysicalExpr>>,
+ _config: &ConfigOptions,
+ ) -> Result<FilterDescription> {
+ // Filters that reference only non-unnested (passthrough) columns
commute
+ // with the unnest. Non-unnest values values are replicated onto every
row
+ // produced from an input row, so dropping an input row before the
unnest
+ // removes exactly the output rows the filter would have dropped after
it.
+ // Filters referencing unnested list/struct columns must stay above
this
+ // node.
+ let input_schema = self.input.schema();
+ let unnested_input_indices: HashSet<usize> = self
+ .list_column_indices
+ .iter()
+ .map(|list_unnest| list_unnest.index_in_input_schema)
+ .chain(self.struct_column_indices.iter().copied())
+ .collect();
+
+ // Output indices of passthrough columns, resolved by name the same way
+ // `compute_properties` builds its projection mapping.
+ let allowed_output_indices: std::collections::HashSet<usize> =
+ (0..input_schema.fields().len())
+ .filter(|input_idx|
!unnested_input_indices.contains(input_idx))
+ .filter_map(|input_idx| {
+ let name = input_schema.field(input_idx).name();
+ self.schema
+ .fields()
+ .iter()
+ .position(|output_field| output_field.name() == name)
Review Comment:
This name-based lookup can make an unnested output incorrectly eligible when
generated and passthrough fields collide. For example, if the input has struct
`s { f1 }` before a passthrough field physically named `s.f1`, the output can
contain two `s.f1` fields; `position` selects the generated one, so a filter on
that generated index is accepted and `FilterRemapper` maps it to the unrelated
passthrough input field. Physical expressions are index-based precisely so
duplicate names can be represented. Could this mapping be derived positionally
from the list/struct expansion (or reject ambiguous matches), with a collision
regression test?
--
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]
