You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2025/04/20 02:23:22 UTC
[PR] move reassign_predicate_columns onto PhysicalExpr [datafusion]
adriangb opened a new pull request, #15779:
URL: https://github.com/apache/datafusion/pull/15779
(no comment)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] replace reassign_predicate_columns helper with PhysicalExpr::with_schema [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #15779:
URL: https://github.com/apache/datafusion/pull/15779#discussion_r2059092200
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -333,6 +333,15 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + DynEq + DynHash {
// This is a safe default behavior.
Ok(None)
}
+
+ /// Adapt this [`PhysicalExpr`] to a new schema.
+ /// For example, `Column("b", 1)` can be adapted to `Column("b", 0)`
+ /// given the schema `Schema::new(vec![("b", DataType::Int32)])`.
+ fn with_schema(&self, _schema: &Schema) -> Result<Option<Arc<dyn PhysicalExpr>>> {
Review Comment:
Sure would love to see a PR with your suggestion that accomplishes what this one does!
Fundamentally, if I understand your proposal correctly, I would suggest splitting into something like:
```rust
trait ExecutionPlan {
fn update_expression_for_input_schema(&self, expr: Arc<dyn PhysicalExpr>) -> Arc<dyn PhysicalExpr> {
expr.with_schema(self.input.schema())
}
fn update_expression_for_output_schema(&self, expr: Arc<dyn PhysicalExpr>) -> Arc<dyn PhysicalExpr> {
expr.with_schema(self.schema())
}
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] replace reassign_predicate_columns helper with PhysicalExpr::with_schema [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #15779:
URL: https://github.com/apache/datafusion/pull/15779#discussion_r2058548271
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -333,6 +333,15 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + DynEq + DynHash {
// This is a safe default behavior.
Ok(None)
}
+
+ /// Adapt this [`PhysicalExpr`] to a new schema.
+ /// For example, `Column("b", 1)` can be adapted to `Column("b", 0)`
+ /// given the schema `Schema::new(vec![("b", DataType::Int32)])`.
+ fn with_schema(&self, _schema: &Schema) -> Result<Option<Arc<dyn PhysicalExpr>>> {
Review Comment:
How are you going to do that deep inside of ParquetSource, where what you want to provide is the physical file schema that has nothing to do with an ExecutionPlan?
I think I'll need to see some working code with your API replacing this PR to understand.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] replace reassign_predicate_columns helper with PhysicalExpr::with_schema [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #15779:
URL: https://github.com/apache/datafusion/pull/15779#discussion_r2057512794
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -333,6 +333,15 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + DynEq + DynHash {
// This is a safe default behavior.
Ok(None)
}
+
+ /// Adapt this [`PhysicalExpr`] to a new schema.
+ /// For example, `Column("b", 1)` can be adapted to `Column("b", 0)`
+ /// given the schema `Schema::new(vec![("b", DataType::Int32)])`.
+ fn with_schema(&self, _schema: &Schema) -> Result<Option<Arc<dyn PhysicalExpr>>> {
Review Comment:
> Since during the physical planning duplicate names are possible, a column shouldn't be used in a place apart from its reference schema.
Ah good point. It's unclear to me where duplicate columns are allowed and where they aren't. E.g. `DFSchema` does not allow duplicate columns IIRC.
> There should be an API at the ExecutionPlan level, like update_expression(&self, expr: Arc, down: bool) -> Option<Arc>
There's some missing types. Do you mean:
```rust
trait ExecutionPlan {
fn update_expression(&self, expr: Arc<dyn PhysicalExpr>, down: bool) -> Option<Arc<dyn PhysicalExpr>>>;
}
```
?
I don't fully follow. What is this function expected to do for different operators, and how does it behave differently across operators? It would help me if you shared the larger vision of how this gets used for this PRs use case, in other places, etc. The code proposed in this PR is being used in places where there is no `ExecutionPlan` (e.g. in parquet pruning within `ParquetOpener`).
Tangential but I generally don't like the idea of having a `bool` parameter like that and would rather it be two functions `update_expression_for_input_schema` and `update_expression_for_output_schema` or something like that.
Back on topic: how is this different than say `PhysicalExpression::with_schema(expr, ExecutionPlan::schema(plan))`?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] replace reassign_predicate_columns helper with PhysicalExpr::with_schema [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #15779:
URL: https://github.com/apache/datafusion/pull/15779#issuecomment-2817948487
> Hi @adriangb. I've a suggestion. As I said, if I don't misestimate the need, this requirement has arisen in some other places as well. So, let's solve it for all
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] replace reassign_predicate_columns helper with PhysicalExpr::with_schema [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #15779:
URL: https://github.com/apache/datafusion/pull/15779#discussion_r2058837968
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -333,6 +333,15 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + DynEq + DynHash {
// This is a safe default behavior.
Ok(None)
}
+
+ /// Adapt this [`PhysicalExpr`] to a new schema.
+ /// For example, `Column("b", 1)` can be adapted to `Column("b", 0)`
+ /// given the schema `Schema::new(vec![("b", DataType::Int32)])`.
+ fn with_schema(&self, _schema: &Schema) -> Result<Option<Arc<dyn PhysicalExpr>>> {
Review Comment:
DataSource's and FileSources can also share a similar API. Let me open a draft PR for this?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] replace reassign_predicate_columns helper with PhysicalExpr::with_schema [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb closed pull request #15779: replace reassign_predicate_columns helper with PhysicalExpr::with_schema
URL: https://github.com/apache/datafusion/pull/15779
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] replace reassign_predicate_columns helper with PhysicalExpr::with_schema [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #15779:
URL: https://github.com/apache/datafusion/pull/15779#discussion_r2052131746
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -333,6 +333,15 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + DynEq + DynHash {
// This is a safe default behavior.
Ok(None)
}
+
+ /// Adapt this [`PhysicalExpr`] to a new schema.
+ /// For example, `Column("b", 1)` can be adapted to `Column("b", 0)`
+ /// given the schema `Schema::new(vec![("b", DataType::Int32)])`.
+ fn with_schema(&self, _schema: &Schema) -> Result<Option<Arc<dyn PhysicalExpr>>> {
Review Comment:
I think introducing such an API is dangerous. Since during the physical planning duplicate names are possible, a column shouldn't be used in a place apart from its reference schema.
I guess this need occurs because expressions need to be updated while going up and down across the plan tree. I've actually thought on that, and have an idea:
There should be an API at the ExecutionPlan level, like update_expression(&self, expr: Arc<dyn PhysicalExpr>, down: bool) -> Option<Arc<dyn PhysicalExpr>>
if `down` is true, it means that this expression should be valid at the **output** of this operator, and we need that expression in a way that how it's represented at the input schema of this operator. If the input schema doesn't have any part of this expression, or the expression cannot be built on the output schema columns of this operator, then the function return None.
Likewise, if the down flag is false, this means that this expression references to the input schema of this operator, and we request it corresponding to the output schema of this operator. Similar failure cases returns None here as well.
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -333,6 +333,15 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + DynEq + DynHash {
// This is a safe default behavior.
Ok(None)
}
+
+ /// Adapt this [`PhysicalExpr`] to a new schema.
+ /// For example, `Column("b", 1)` can be adapted to `Column("b", 0)`
+ /// given the schema `Schema::new(vec![("b", DataType::Int32)])`.
+ fn with_schema(&self, _schema: &Schema) -> Result<Option<Arc<dyn PhysicalExpr>>> {
Review Comment:
BTW, a similar approach is done in projection_pushdown rule, but this design is better IMO
##########
datafusion/physical-expr/src/expressions/column.rs:
##########
@@ -141,6 +141,18 @@ impl PhysicalExpr for Column {
fn fmt_sql(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.name)
}
+
+ fn with_schema(&self, schema: &Schema) -> Result<Option<Arc<dyn PhysicalExpr>>> {
+ // Find our index in the new schema
+ let new_index = schema.index_of(&self.name)?;
Review Comment:
This `index_of` works with `find()`, which have caused many bugs on tables having same named columns
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] replace reassign_predicate_columns helper with PhysicalExpr::with_schema [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #15779:
URL: https://github.com/apache/datafusion/pull/15779#discussion_r2058262957
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -333,6 +333,15 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + DynEq + DynHash {
// This is a safe default behavior.
Ok(None)
}
+
+ /// Adapt this [`PhysicalExpr`] to a new schema.
+ /// For example, `Column("b", 1)` can be adapted to `Column("b", 0)`
+ /// given the schema `Schema::new(vec![("b", DataType::Int32)])`.
+ fn with_schema(&self, _schema: &Schema) -> Result<Option<Arc<dyn PhysicalExpr>>> {
Review Comment:
> There's some missing types. Do you mean:
Yes, exactly like that
> I don't fully follow. What is this function expected to do for different operators, and how does it behave differently across operators?
Every operator takes an input schema, and provides an output schema, possibly after doing some modifications on the input schema. So, that modification of schemas should be trackable by some API's. For example, this is an example schema tree:
```
A: [a+b] + [Sum(x)] + [y]
B: -- [a+b] + [Sum(x)] + [y]
C: ---- [a+b] + [x] + [y]
D: ------[a+b]
E: --------[a] + [b] + [c]
F: ------[x] + [y]
G: --------[z] + [y] + [x]
```
We should be able to ask, for example to D, "update `(a+b)@0*2` as if it's in your input schema", and it will say it is `(a@0+b@1)*2`. The inverse question should also be possible. This "foo" expression is written according to your input schema, give me as if it is in your output schema.
TLDR, what I ask from the ExecutionPlan is, operators should be able to track the projection mappings of their input to output columns.
> Tangential but I generally don't like the idea of having a bool parameter like that and would rather it be two functions update_expression_for_input_schema and update_expression_for_output_schema or something like that.
We can do that in 2 different methods for sure
> Back on topic: how is this different than say PhysicalExpression::with_schema(expr, ExecutionPlan::schema(plan))?
When you provide only 1 schema, and if you don't know the relation between the schema and expression, you cannot resolve the same name issues. Your suggestion seems to provide a flexibility, as you can just give a custom schema and an expression, but what I try to point out is, we cannot do that easily, because we need to track from the source of the expression, until its target. Therefore; I'm suggesting an ExecutionPlan API which helps tracking the all passes.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org