nuno-faria commented on code in PR #19619:
URL: https://github.com/apache/datafusion/pull/19619#discussion_r2670231291
##########
datafusion/physical-plan/src/filter.rs:
##########
@@ -619,7 +686,23 @@ impl ExecutionPlan for FilterExec {
impl EmbeddedProjection for FilterExec {
fn with_projection(&self, projection: Option<Vec<usize>>) -> Result<Self> {
- self.with_projection(projection)
+ // Check if the projection is valid
+ can_project(&self.schema(), projection.as_ref())?;
+
+ let projection = match projection {
+ Some(projection) => match &self.projection {
+ Some(p) => Some(projection.iter().map(|i| p[*i]).collect()),
+ None => Some(projection),
+ },
+ None => None,
+ };
+
+ FilterExecBuilder::new(Arc::clone(&self.predicate),
Arc::clone(&self.input))
+ .with_projection(projection)
+ .with_default_selectivity(self.default_selectivity)
+ .with_batch_size(self.batch_size)
+ .with_fetch(self.fetch)
+ .build()
Review Comment:
I'm having some doubts about deprecating `with_projection` of `FilterExec`
(I highlight here the `with_projection` of `impl EmbeddedProjection`, which has
the same code). Mainly about this part here:
```rust
let projection = match projection {
Some(projection) => match &self.projection {
Some(p) => Some(projection.iter().map(|i| p[*i]).collect()),
None => Some(projection),
},
None => None,
};
```
It appears that the projection passed to the method tries to project the
existing projection. I'm not sure why this is the case, but I think if we
deprecate `with_projection` we lose the ability to do this. But I also haven't
seen a `with_projection` called on an existing `FilterExec`, it is always
accompanied with a `try_new`.
@adriangb @alamb what do 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]
