You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2026/02/06 16:41:24 UTC
[PR] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
alamb opened a new pull request, #20193:
URL: https://github.com/apache/datafusion/pull/20193
## Which issue does this PR close?
- Part of https://github.com/apache/datafusion/issues/20173
## Rationale for this change
I am going through how the various layers of expression pushdown and schema rewrites work, and I spent a long time confused about what the `sync_with_child` parameter on `update_expr(.., true)` vs `update_expr(..., false)` did
After some study I realized it controls which way the rewrite is done (either project the expressions to refer to the projection expressions, or the opposite, 'unproject' an expression)
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes by existing ci
## Are there any user-facing changes?
There are new APIs, but no changes to existing APIs
--
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] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20193:
URL: https://github.com/apache/datafusion/pull/20193#discussion_r2775061120
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -793,37 +793,27 @@ impl DataSource for FileScanConfig {
config: &ConfigOptions,
) -> Result<FilterPushdownPropagation<Arc<dyn DataSource>>> {
// Remap filter Column indices to match the table schema (file + partition columns).
- // This is necessary because filters may have been created against a different schema
- // (e.g., after projection pushdown) and need to be remapped to the table schema
- // before being passed to the file source and ultimately serialized.
- // For example, the filter being pushed down is `c1_c2 > 5` and it was created
- // against the output schema of the this `DataSource` which has projection `c1 + c2 as c1_c2`.
- // Thus we need to rewrite the filter back to `c1 + c2 > 5` before passing it to the file source.
+ // This is necessary because refer to the output shema of this `DataSource`
+ // (e.g., after projection pushdown has been applied) and need to be remapped to the table schema
+ // before being passed to the file source
+ //
+ // For example, consider a filter `c1_c2 > 5` being pushed down. If the
+ // `DataSource` has a projection `c1 + c2 as c1_c2`, the filter must be rewritten
+ // to refer to the table schema `c1 + c2 > 5`
let table_schema = self.file_source.table_schema().table_schema();
- // If there's a projection with aliases, first map the filters back through
- // the projection expressions before remapping to the table schema.
let filters_to_remap = if let Some(projection) = self.file_source.projection() {
- use datafusion_physical_plan::projection::update_expr;
filters
.into_iter()
- .map(|filter| {
- update_expr(&filter, projection.as_ref(), true)?.ok_or_else(|| {
- internal_datafusion_err!(
- "Failed to map filter expression through projection: {}",
- filter
- )
- })
- })
+ .map(|filter| projection.unproject_expr(&filter))
Review Comment:
The whole point of the PR is to pull this function call into a method on `ProjectionExprs` and add documentation
##########
datafusion/physical-expr/src/projection.rs:
##########
@@ -842,7 +901,7 @@ pub fn combine_projections(
pub fn update_expr(
expr: &Arc<dyn PhysicalExpr>,
projected_exprs: &[ProjectionExpr],
- sync_with_child: bool,
+ unproject: bool,
Review Comment:
I found this parameter name to be **super** confusing as there are no "children" involved.
`unproject` as the best I could come up with -- but would love some thoughts on better names
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -793,37 +793,27 @@ impl DataSource for FileScanConfig {
config: &ConfigOptions,
) -> Result<FilterPushdownPropagation<Arc<dyn DataSource>>> {
// Remap filter Column indices to match the table schema (file + partition columns).
- // This is necessary because filters may have been created against a different schema
- // (e.g., after projection pushdown) and need to be remapped to the table schema
- // before being passed to the file source and ultimately serialized.
- // For example, the filter being pushed down is `c1_c2 > 5` and it was created
- // against the output schema of the this `DataSource` which has projection `c1 + c2 as c1_c2`.
- // Thus we need to rewrite the filter back to `c1 + c2 > 5` before passing it to the file source.
+ // This is necessary because refer to the output shema of this `DataSource`
+ // (e.g., after projection pushdown has been applied) and need to be remapped to the table schema
+ // before being passed to the file source
+ //
+ // For example, consider a filter `c1_c2 > 5` being pushed down. If the
+ // `DataSource` has a projection `c1 + c2 as c1_c2`, the filter must be rewritten
+ // to refer to the table schema `c1 + c2 > 5`
let table_schema = self.file_source.table_schema().table_schema();
- // If there's a projection with aliases, first map the filters back through
- // the projection expressions before remapping to the table schema.
let filters_to_remap = if let Some(projection) = self.file_source.projection() {
- use datafusion_physical_plan::projection::update_expr;
filters
.into_iter()
- .map(|filter| {
- update_expr(&filter, projection.as_ref(), true)?.ok_or_else(|| {
- internal_datafusion_err!(
- "Failed to map filter expression through projection: {}",
- filter
- )
- })
- })
+ .map(|filter| projection.unproject_expr(&filter))
.collect::<Result<Vec<_>>>()?
} else {
filters
};
// Now remap column indices to match the table schema.
- let remapped_filters: Result<Vec<_>> = filters_to_remap
+ let remapped_filters = filters_to_remap
Review Comment:
driveby cleanup (no functional change)
--
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] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20193:
URL: https://github.com/apache/datafusion/pull/20193#issuecomment-3893249957
Thanks for the review @adriangb
--
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] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20193:
URL: https://github.com/apache/datafusion/pull/20193#discussion_r2775070343
##########
datafusion/physical-expr/src/projection.rs:
##########
@@ -842,7 +901,7 @@ pub fn combine_projections(
pub fn update_expr(
expr: &Arc<dyn PhysicalExpr>,
projected_exprs: &[ProjectionExpr],
- sync_with_child: bool,
+ unproject: bool,
Review Comment:
I found this parameter name to be **super** confusing as there are no "children" involved. I suspect this is historical and predates the work to push expressions down
`unproject` as the best I could come up with -- but would love some thoughts on better names
--
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] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20193:
URL: https://github.com/apache/datafusion/pull/20193#discussion_r2775086602
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -793,37 +793,27 @@ impl DataSource for FileScanConfig {
config: &ConfigOptions,
) -> Result<FilterPushdownPropagation<Arc<dyn DataSource>>> {
// Remap filter Column indices to match the table schema (file + partition columns).
- // This is necessary because filters may have been created against a different schema
- // (e.g., after projection pushdown) and need to be remapped to the table schema
- // before being passed to the file source and ultimately serialized.
- // For example, the filter being pushed down is `c1_c2 > 5` and it was created
- // against the output schema of the this `DataSource` which has projection `c1 + c2 as c1_c2`.
- // Thus we need to rewrite the filter back to `c1 + c2 > 5` before passing it to the file source.
+ // This is necessary because filters refer to the output schema of this `DataSource`
Review Comment:
Clarified that the "different schema" is the "table 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] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20193:
URL: https://github.com/apache/datafusion/pull/20193#discussion_r2775185496
##########
datafusion/physical-expr/src/projection.rs:
##########
@@ -842,7 +901,7 @@ pub fn combine_projections(
pub fn update_expr(
Review Comment:
I contemplated going even farther and deprecating this function (and routing everything through ProjectionExprs) but I think that will result in some non trivial API churn and I figured we could always do it later
--
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] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #20193:
URL: https://github.com/apache/datafusion/pull/20193
--
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] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20193:
URL: https://github.com/apache/datafusion/pull/20193#issuecomment-3862694105
> I have not been able to wrap my head around `update_expr` either. I will review this later today or tomorrow 😄
Thanks -- no rush.
--
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] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #20193:
URL: https://github.com/apache/datafusion/pull/20193#issuecomment-3861914710
I have not been able to wrap my head around `update_expr` either. I will review this later today or tomorrow 😄
--
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] Introduce ProjectionExprs::unproject_exprs/project_exprs and improve docs [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #20193:
URL: https://github.com/apache/datafusion/pull/20193#discussion_r2783315040
##########
datafusion/physical-expr/src/projection.rs:
##########
@@ -842,7 +901,7 @@ pub fn combine_projections(
pub fn update_expr(
Review Comment:
I think if we were to do this a first step would be to move the logic from `update_expr` into the functions on `ProjectionExprs` and make `update_expr` call those, maybe adding a note that it's "soft deprecated" and we recommend calling `ProjectionExprs` instead? I think that could fit in this PR.
--
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