Skip to content

Commit b9f816f

Browse files
committed
fix(lambda): keep multi-param lambda parameter positions stable
`LambdaExpr` previously compressed the column-index projection by enumerating all referenced indices and packing them into the front of the merged evaluation batch. That collapse is correct for outer captures, but it also moved lambda parameters around: a two-parameter lambda `(k, v) -> v` (where `k` is unused) would have its `LambdaVariable` for `v` re-projected from index 1 to index 0, so at runtime it would read the slot the higher-order function had filled with `k`. `LambdaExpr` now tracks `outer_columns_count` (the size of the outer schema the lambda was planned against). Indices below that boundary are treated as captures and compressed; indices at or above it are treated as lambda parameters and keep their fixed offset from the start of the parameter block, so unused parameters leave a gap rather than shifting the used ones. This is a breaking change to `LambdaExpr::try_new` and the `expressions::lambda(...)` helper: both now take an extra `outer_columns_count: usize` parameter, and `create_physical_expr` forwards it from the outer DFSchema. Extracted from #22689 because it needs separate review attention.
1 parent a0e05f6 commit b9f816f

3 files changed

Lines changed: 158 additions & 20 deletions

File tree

datafusion/physical-expr/src/expressions/lambda.rs

Lines changed: 137 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@ pub struct LambdaExpr {
4343
body: Arc<dyn PhysicalExpr>,
4444
projected_body: Arc<dyn PhysicalExpr>,
4545
projection: Vec<usize>,
46+
/// Number of columns in the outer input schema. Column/LambdaVariable
47+
/// indices below this value reference outer captures; indices at or above
48+
/// reference lambda parameters (whose position in the merged evaluation
49+
/// batch is fixed by the higher-order function, not by the projection).
50+
outer_columns_count: usize,
4651
}
4752

4853
// Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 [https://github.com/apache/datafusion/issues/13196]
@@ -60,8 +65,19 @@ impl Hash for LambdaExpr {
6065
}
6166

6267
impl LambdaExpr {
63-
/// Create a new lambda expression with the given parameters and body
64-
pub fn try_new(params: Vec<String>, body: Arc<dyn PhysicalExpr>) -> Result<Self> {
68+
/// Create a new lambda expression with the given parameters and body.
69+
///
70+
/// `outer_columns_count` is the number of columns in the outer input
71+
/// schema this lambda was planned against. Column/LambdaVariable indices
72+
/// strictly below `outer_columns_count` reference outer captures and get
73+
/// compressed to the front of the evaluation batch; indices at or above
74+
/// reference lambda parameters and keep their fixed position relative to
75+
/// the captures (so unused lambda parameters do not shift used ones).
76+
pub fn try_new(
77+
params: Vec<String>,
78+
body: Arc<dyn PhysicalExpr>,
79+
outer_columns_count: usize,
80+
) -> Result<Self> {
6581
if !all_unique(&params) {
6682
return plan_err!(
6783
"lambda params must be unique, got ({})",
@@ -71,10 +87,14 @@ impl LambdaExpr {
7187

7288
check_async_udf(&body)?;
7389

74-
Ok(Self::new(params, body))
90+
Ok(Self::new(params, body, outer_columns_count))
7591
}
7692

77-
fn new(params: Vec<String>, body: Arc<dyn PhysicalExpr>) -> Self {
93+
fn new(
94+
params: Vec<String>,
95+
body: Arc<dyn PhysicalExpr>,
96+
outer_columns_count: usize,
97+
) -> Self {
7898
let mut used_column_indices = HashSet::new();
7999

80100
body.apply(|node| {
@@ -92,10 +112,26 @@ impl LambdaExpr {
92112

93113
projection.sort();
94114

115+
// Captures (outer column refs) get compressed to the front of the
116+
// merged batch. Lambda variables (indices >= outer_columns_count)
117+
// keep their fixed offset from the start of the lambda parameter
118+
// block, because the higher-order function always pushes all
119+
// declared parameters into the merged batch in order.
120+
let used_captures_count = projection
121+
.iter()
122+
.filter(|i| **i < outer_columns_count)
123+
.count();
95124
let column_index_map = projection
96125
.iter()
97126
.enumerate()
98-
.map(|(projected, original)| (*original, projected))
127+
.map(|(captures_pos, original)| {
128+
let projected = if *original < outer_columns_count {
129+
captures_pos
130+
} else {
131+
used_captures_count + (*original - outer_columns_count)
132+
};
133+
(*original, projected)
134+
})
99135
.collect::<HashMap<_, _>>();
100136

101137
let projected_body = Arc::clone(&body)
@@ -129,6 +165,7 @@ impl LambdaExpr {
129165
body,
130166
projected_body,
131167
projection,
168+
outer_columns_count,
132169
}
133170
}
134171

@@ -187,22 +224,31 @@ impl PhysicalExpr for LambdaExpr {
187224

188225
check_async_udf(body)?;
189226

190-
Ok(Arc::new(Self::new(self.params.clone(), Arc::clone(body))))
227+
Ok(Arc::new(Self::new(
228+
self.params.clone(),
229+
Arc::clone(body),
230+
self.outer_columns_count,
231+
)))
191232
}
192233

193234
fn fmt_sql(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
194235
write!(f, "({}) -> {}", self.params.join(", "), self.body)
195236
}
196237
}
197238

198-
/// Create a lambda expression
239+
/// Create a lambda expression.
240+
///
241+
/// `outer_columns_count` is the number of columns in the outer input schema
242+
/// this lambda was planned against. See [`LambdaExpr::try_new`] for details.
199243
pub fn lambda(
200244
params: impl IntoIterator<Item = impl Into<String>>,
201245
body: Arc<dyn PhysicalExpr>,
246+
outer_columns_count: usize,
202247
) -> Result<Arc<dyn PhysicalExpr>> {
203248
Ok(Arc::new(LambdaExpr::try_new(
204249
params.into_iter().map(Into::into).collect(),
205250
body,
251+
outer_columns_count,
206252
)?))
207253
}
208254

@@ -234,19 +280,99 @@ fn check_async_udf(body: &Arc<dyn PhysicalExpr>) -> Result<()> {
234280

235281
#[cfg(test)]
236282
mod tests {
237-
use crate::expressions::{NoOp, lambda::lambda};
238-
use arrow::{array::RecordBatch, datatypes::Schema};
283+
use crate::expressions::{Column, LambdaVariable, NoOp, lambda::lambda};
284+
use arrow::{
285+
array::RecordBatch,
286+
datatypes::{DataType, Field, Schema},
287+
};
239288
use std::sync::Arc;
240289

290+
use super::LambdaExpr;
291+
241292
#[test]
242293
fn test_lambda_evaluate() {
243-
let lambda = lambda(["a"], Arc::new(NoOp::new())).unwrap();
294+
let lambda = lambda(["a"], Arc::new(NoOp::new()), 0).unwrap();
244295
let batch = RecordBatch::new_empty(Arc::new(Schema::empty()));
245296
assert!(lambda.evaluate(&batch).is_err());
246297
}
247298

248299
#[test]
249300
fn test_lambda_duplicate_name() {
250-
assert!(lambda(["a", "a"], Arc::new(NoOp::new())).is_err());
301+
assert!(lambda(["a", "a"], Arc::new(NoOp::new()), 0).is_err());
302+
}
303+
304+
/// Multi-parameter lambdas that reference only a subset of their declared
305+
/// parameters must NOT shift the used parameter into the slot of an
306+
/// unused one. The higher-order function pushes all declared parameters
307+
/// into the merged batch in order, so a lambda `(k, v) -> v` (with `v` at
308+
/// LambdaVariable index 1) must keep its body referencing index 1, not
309+
/// get re-projected to index 0 just because `k` is unused.
310+
#[test]
311+
fn test_multi_param_lambda_keeps_param_positions_stable() {
312+
let v_field = Arc::new(Field::new("v", DataType::Int32, true));
313+
let body = Arc::new(LambdaVariable::new(1, Arc::clone(&v_field)));
314+
315+
let lambda = LambdaExpr::try_new(
316+
vec!["k".to_string(), "v".to_string()],
317+
body,
318+
0, // no outer captures
319+
)
320+
.unwrap();
321+
322+
assert_eq!(lambda.projection(), &[1]);
323+
324+
let projected_var = lambda
325+
.projected_body()
326+
.downcast_ref::<LambdaVariable>()
327+
.expect("projected body should be a LambdaVariable");
328+
assert_eq!(projected_var.index(), 1);
329+
}
330+
331+
/// With outer captures, used outer columns get compressed to the front of
332+
/// the projected batch, but lambda parameter positions stay at their
333+
/// fixed offset from the start of the lambda parameter block (so an
334+
/// unused parameter still leaves a gap rather than shifting later ones).
335+
#[test]
336+
fn test_lambda_compresses_outer_captures_but_pins_params() {
337+
// outer schema has 5 columns (indices 0..=4); lambda has 2 params at
338+
// indices 5 and 6. Body references outer column 3 and the second
339+
// lambda param (`v` at index 6); the first lambda param (`k` at 5)
340+
// is unused.
341+
let v_field = Arc::new(Field::new("v", DataType::Int32, true));
342+
let body = Arc::new(crate::expressions::BinaryExpr::new(
343+
Arc::new(Column::new("c3", 3)),
344+
datafusion_expr::Operator::Plus,
345+
Arc::new(LambdaVariable::new(6, Arc::clone(&v_field))),
346+
));
347+
348+
let lambda = LambdaExpr::try_new(
349+
vec!["k".to_string(), "v".to_string()],
350+
body,
351+
5, // outer_columns_count
352+
)
353+
.unwrap();
354+
355+
// Both originals are referenced (3 and 6), projection is sorted.
356+
assert_eq!(lambda.projection(), &[3, 6]);
357+
358+
// After projection:
359+
// outer col 3 -> position 0 (compressed to the captures block)
360+
// lambda var 6 -> position 2 (used_captures=1 + (6 - 5))
361+
// NOT position 1, because the unused `k` (var 5)
362+
// still occupies its slot.
363+
let binary = lambda
364+
.projected_body()
365+
.downcast_ref::<crate::expressions::BinaryExpr>()
366+
.expect("projected body should be a BinaryExpr");
367+
let left = binary
368+
.left()
369+
.downcast_ref::<Column>()
370+
.expect("left should be a Column");
371+
let right = binary
372+
.right()
373+
.downcast_ref::<LambdaVariable>()
374+
.expect("right should be a LambdaVariable");
375+
assert_eq!(left.index(), 0);
376+
assert_eq!(right.index(), 2);
251377
}
252378
}

datafusion/physical-expr/src/higher_order_function.rs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -628,7 +628,7 @@ mod tests {
628628

629629
let hof = HigherOrderFunctionExpr::try_new_with_schema(
630630
fun,
631-
vec![lambda(["a"], Arc::new(Literal::new(expected.clone()))).unwrap()],
631+
vec![lambda(["a"], Arc::new(Literal::new(expected.clone())), 0).unwrap()],
632632
&Schema::empty(),
633633
Arc::new(ConfigOptions::new()),
634634
)
@@ -664,10 +664,12 @@ mod tests {
664664
let hof = HigherOrderFunctionExpr::try_new_with_schema(
665665
fun,
666666
vec![
667-
not(
668-
lambda(["a"], Arc::new(Literal::new(ScalarValue::Int32(Some(42)))))
669-
.unwrap(),
667+
not(lambda(
668+
["a"],
669+
Arc::new(Literal::new(ScalarValue::Int32(Some(42)))),
670+
0,
670671
)
672+
.unwrap())
671673
.unwrap(),
672674
],
673675
&Schema::empty(),
@@ -707,7 +709,7 @@ mod tests {
707709
.unwrap();
708710

709711
let result = Arc::new(hof)
710-
.with_new_children(vec![lambda(["a"], Arc::new(NoOp::new())).unwrap()])
712+
.with_new_children(vec![lambda(["a"], Arc::new(NoOp::new()), 0).unwrap()])
711713
.unwrap_err();
712714

713715
assert_contains!(

datafusion/physical-expr/src/planner.rs

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -513,10 +513,20 @@ pub fn create_physical_expr(
513513
config_options,
514514
)?))
515515
}
516-
Expr::Lambda(Lambda { params, body }) => expressions::lambda(
517-
params,
518-
create_physical_expr(body, input_dfschema, execution_props)?,
519-
),
516+
Expr::Lambda(Lambda { params, body }) => {
517+
// The lambda was planned against a schema that appends the
518+
// lambda's parameters to the outer input schema. Subtract
519+
// `params.len()` to recover the outer column count, which the
520+
// physical lambda needs to distinguish outer captures from
521+
// lambda parameters during projection.
522+
let outer_columns_count =
523+
input_dfschema.fields().len().saturating_sub(params.len());
524+
expressions::lambda(
525+
params,
526+
create_physical_expr(body, input_dfschema, execution_props)?,
527+
outer_columns_count,
528+
)
529+
}
520530
Expr::LambdaVariable(LambdaVariable {
521531
name,
522532
field,

0 commit comments

Comments
 (0)