Skip to content

fix: rebase lambda params after projection schema changes - #24406

Open
shinzoxD wants to merge 3 commits into
apache:mainfrom
shinzoxD:fix/lambda-variable-schema-rebase
Open

fix: rebase lambda params after projection schema changes#24406
shinzoxD wants to merge 3 commits into
apache:mainfrom
shinzoxD:fix/lambda-variable-schema-rebase

Conversation

@shinzoxD

@shinzoxDshinzoxD commented Aug 16, 2026

Copy link
Copy Markdown

Which issue does this PR close?

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 ColumnandLambdaVariable 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 ProjectionExecs, 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:

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, and a nested-lambda ProjectLambdaBody rebasing test (outer captured column, outer param captured by the inner lambda, inner param that shadows an outer name).
  • SQLLogic tests for both Projection rewrites do not rebase physical LambdaVariable bindings across schema boundaries #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.

LambdaExpr mixed own-parameter indexes into the outer-batch capture
projection. After a projection rewrite grew the input schema, a stale
param index such as x@1 became a real input column and evaluation
failed with a Field mismatch.
Bind own params by name into the captures++params layout instead, and
leave nested-lambda params for the inner LambdaExpr to remap.
Closesapache#24372
CopilotAI lite review requested due to automatic review settings August 16, 2026 00:14

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kosiewkosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shinzoxD,

Thanks for working on this. The fix looks good overall, and I don't see any blocking issues. I left one suggestion for some additional nested-lambda coverage.

impl TreeNodeRewriter for ProjectLambdaBody<'_> {
type Node = Arc<dyn PhysicalExpr>;

fn f_down(&mut self, node: Self::Node) -> Result<Transformed<Self::Node>> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6f871bf: 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).

@github-actionsgithub-actionsBot added the auto detected api change Auto detected API change label Aug 19, 2026
@codecov-commenter

codecov-commenter commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.19355% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.24%. Comparing base (e7e037d) to head (6f871bf).
⚠️ Report is 73 commits behind head on main.

Files with missing linesPatch %Lines
datafusion/physical-expr/src/expressions/lambda.rs99.19%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #24406 +/- ##
==========================================
+ Coverage 81.18% 81.24% +0.06% 
==========================================
Files 1110 1113 +3 Lines 388906 392838 +3932 Branches 388906 392838 +3932 ==========================================
+ Hits 315733 319172 +3439 - Misses 54576 54905 +329 - Partials 18597 18761 +164 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jefffrey

Copy link
Copy Markdown
Contributor

for anyone looking at this PR please see my comment here:

@alamb

alamb commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

for anyone looking at this PR please see my comment here:

Thi seems to have been resolved successfully -- is this PR now ready to merge once it passes CI?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-exprChanges to the physical-expr cratessqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Projection rewrites do not rebase physical LambdaVariable bindings across schema boundaries

6 participants

@shinzoxD@codecov-commenter@Jefffrey@alamb@kosiew