You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "shinzoxD (via GitHub)" <gi...@apache.org> on 2026/08/16 00:14:46 UTC
[PR] fix: rebase lambda params after projection schema changes [datafusion]
shinzoxD opened a new pull request, #24406:
URL: https://github.com/apache/datafusion/pull/24406
## Which issue does this PR close?
Closes #24372
## Rationale for this change
Physical `LambdaVariable` bindings are planned in a combined `input_schema ++ params` index space. `LambdaExpr` then built its outer-batch *capture* projection from every `Column` **and** `LambdaVariable` index in the body.
That is safe only while the input width stays the same as at plan time. After a later rewrite moves the expression across a schema boundary (collapsing consecutive `ProjectionExec`s, or pushing a join filter below `NestedLoopJoinExec`), a stale param index such as `x@1` can become a real input column. Evaluation then fails:
```
Field of physical LambdaVariable with index 0 doesn't match batch field
during evaluation Field { x: nullable Int64 } != Field { column2: Int64 }
```
The two issue reproducers were:
```sql
SELECT array_transform(arr, x -> x)
FROM (
SELECT arr
FROM (VALUES ([1, 2], 7)) AS t(arr, padding)
) AS q;
```
and the `array_filter` join-filter pushdown case.
## What changes are included in this PR?
- Own lambda parameters are no longer added to the outer-batch capture projection.
- They are rebound by name into the `captures ++ used_params` layout that `LambdaArgument::new` already builds.
- Nested-lambda parameters stay shadowed and are remapped by the inner `LambdaExpr`.
## Are these changes tested?
- Unit tests in `datafusion/physical-expr/src/expressions/lambda.rs`, including a regression that a planner-assigned `x@1` is not treated as an outer-batch capture.
- SQLLogic tests for both #24372 reproducers in `array_transform.slt` and `array_filter.slt`.
- Existing nested-lambda slt files (`array_transform`, `array_filter`, `array_any_match`, `array_all`, `array_any_value`, `array_first`) still pass.
## Are there any user-facing changes?
No public API change. Queries that previously failed after a projection/join-filter rewrite now return the correct result.
--
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] fix: rebase lambda params after projection schema changes [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24406:
URL: https://github.com/apache/datafusion/pull/24406#issuecomment-6350012188
Marking as draft until we figure out what to do -- I am trying to clean up the PRs
--
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] fix: rebase lambda params after projection schema changes [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24406:
URL: https://github.com/apache/datafusion/pull/24406#issuecomment-5344568065
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-physical-expr v54.1.0 (current)
Built [ 30.107s] (current)
Parsing datafusion-physical-expr v54.1.0 (current)
Parsed [ 0.041s] (current)
Building datafusion-physical-expr v54.1.0 (baseline)
Built [ 23.415s] (baseline)
Parsing datafusion-physical-expr v54.1.0 (baseline)
Parsed [ 0.042s] (baseline)
Checking datafusion-physical-expr v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.395s] 223 checks: 221 pass, 2 fail, 0 warn, 31 skip
--- failure function_missing: pub fn removed or renamed ---
Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/function_missing.ron
Failed in:
function datafusion_physical_expr::expressions::sql_similar_to_regex, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/6eb2dd7e4b70d2305875b6754fa28edc91ff189d/datafusion/physical-expr/src/expressions/similar_to_pattern.rs:31
--- failure struct_missing: pub struct removed or renamed ---
Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_missing.ron
Failed in:
struct datafusion_physical_expr::expressions::SqlSimilarToPattern, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/6eb2dd7e4b70d2305875b6754fa28edc91ff189d/datafusion/physical-expr/src/expressions/similar_to_pattern.rs:62
Summary semver requires new major version: 2 major and 0 minor checks failed
Finished [ 54.810s] datafusion-physical-expr
Building datafusion-sqllogictest v54.1.0 (current)
Built [ 80.159s] (current)
Parsing datafusion-sqllogictest v54.1.0 (current)
Parsed [ 0.019s] (current)
Building datafusion-sqllogictest v54.1.0 (baseline)
Built [ 80.191s] (baseline)
Parsing datafusion-sqllogictest v54.1.0 (baseline)
Parsed [ 0.019s] (baseline)
Checking datafusion-sqllogictest v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.095s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 164.810s] datafusion-sqllogictest
```
</details>
--
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] fix: rebase lambda params after projection schema changes [datafusion]
Posted by "shinzoxD (via GitHub)" <gi...@apache.org>.
shinzoxD commented on code in PR #24406:
URL: https://github.com/apache/datafusion/pull/24406#discussion_r3815214858
##########
datafusion/physical-expr/src/expressions/lambda.rs:
##########
@@ -253,6 +261,72 @@ impl TreeNodeVisitor<'_> for CollectUsedVisitor<'_> {
}
}
+/// Rewrites a lambda body so captured columns occupy `0..n_captures` and
+/// this lambda's own parameters occupy `n_captures..`. Nested lambdas are
+/// tracked on `shadow_stack` so an inner parameter that reuses a name is
+/// not remapped as if it were this lambda's param.
+struct ProjectLambdaBody<'a> {
+ column_index_map: &'a HashMap<usize, usize>,
+ param_name_to_projected: &'a HashMap<String, usize>,
+ shadow_stack: Vec<HashSet<String>>,
+}
+
+impl TreeNodeRewriter for ProjectLambdaBody<'_> {
+ type Node = Arc<dyn PhysicalExpr>;
+
+ fn f_down(&mut self, node: Self::Node) -> Result<Transformed<Self::Node>> {
Review Comment:
Addressed in 6f871bf46453567fdf90569eab49f27bf7d5395a: added `test_nested_lambda_project_lambda_body_rebasing`, which reconstructs nested lambdas through `ProjectLambdaBody` and asserts the `captures ++ used_params` index layout at both levels. It covers an outer captured column (`col@2` rebased to `col@0`), an outer parameter captured by the inner lambda (`v`), and an inner parameter that shadows an outer name (`k`).
--
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] fix: rebase lambda params after projection schema changes [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24406:
URL: https://github.com/apache/datafusion/pull/24406#issuecomment-5344814545
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24406?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `98.66667%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.18%. Comparing base ([`e7e037d`](https://app.codecov.io/gh/apache/datafusion/commit/e7e037dbeb6e578f13a6ba16d528366e888c253a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`35cb174`](https://app.codecov.io/gh/apache/datafusion/commit/35cb174fd958500378e8679d49f648fdcd0df42b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 37 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24406?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-expr/src/expressions/lambda.rs](https://app.codecov.io/gh/apache/datafusion/pull/24406?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Flambda.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9sYW1iZGEucnM=) | 98.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24406?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24406 +/- ##
=======================================
Coverage 81.18% 81.18%
=======================================
Files 1110 1110
Lines 388906 388953 +47
Branches 388906 388953 +47
=======================================
+ Hits 315733 315779 +46
- Misses 54576 54577 +1
Partials 18597 18597
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24406?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] fix: rebase lambda params after projection schema changes [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #24406:
URL: https://github.com/apache/datafusion/pull/24406#issuecomment-5350198003
for anyone looking at this PR please see my comment here:
- https://github.com/apache/datafusion/pull/24407#issuecomment-5350186733
--
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] fix: rebase lambda params after projection schema changes [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24406:
URL: https://github.com/apache/datafusion/pull/24406#discussion_r3813619737
##########
datafusion/physical-expr/src/expressions/lambda.rs:
##########
@@ -253,6 +261,72 @@ impl TreeNodeVisitor<'_> for CollectUsedVisitor<'_> {
}
}
+/// Rewrites a lambda body so captured columns occupy `0..n_captures` and
+/// this lambda's own parameters occupy `n_captures..`. Nested lambdas are
+/// tracked on `shadow_stack` so an inner parameter that reuses a name is
+/// not remapped as if it were this lambda's param.
+struct ProjectLambdaBody<'a> {
+ column_index_map: &'a HashMap<usize, usize>,
+ param_name_to_projected: &'a HashMap<String, usize>,
+ shadow_stack: Vec<HashSet<String>>,
+}
+
+impl TreeNodeRewriter for ProjectLambdaBody<'_> {
+ type Node = Arc<dyn PhysicalExpr>;
+
+ fn f_down(&mut self, node: Self::Node) -> Result<Transformed<Self::Node>> {
Review Comment:
Could we add a focused nested-lambda rebasing assertion here? It would be useful to cover an outer captured column, an outer parameter captured by the inner lambda, and an inner parameter that shadows an outer name. The existing shadowing test checks `used_param_indices`, but it doesn't verify the new `ProjectLambdaBody` index layout across both reconstruction levels.
--
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] fix: rebase lambda params after projection schema changes [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24406:
URL: https://github.com/apache/datafusion/pull/24406#issuecomment-5591769362
> for anyone looking at this PR please see my comment here:
>
> * [fix: roll back DiskManager usage when spill write I/O fails #24407 (comment)](https://github.com/apache/datafusion/pull/24407#issuecomment-5350186733)
Thi seems to have been resolved successfully -- is this PR now ready to merge once it passes CI?
--
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