Skip to content

Commit 70ba67b

Browse files
committed
refactor: extract LambdaExpr breaking change to apache#22853
Per maintainer request on PR apache#22689, the `LambdaExpr::try_new` / `expressions::lambda(...)` signature change (adding `outer_columns_count`) is being reviewed separately in apache#22853 because it's a breaking change to the physical-expr public API and warrants its own attention, distinct from this additive UDF. This commit reverts the `lambda.rs` / `higher_order_function.rs` / `planner.rs` files to their `upstream/main` state, removes the sqllogictest file (every query in it uses `(k, v) -> body` lambdas that require the upstream fix), and marks the unit tests that exercise multi-parameter lambdas with `#[ignore = "blocked on apache#22853: multi-param lambda projection fix"]`. `transform_values_uses_keys_via_case` and `transform_values_all_null_rows_returns_null_array` still pass because the former references both `k` and `v` (so projection is a no-op) and the latter short-circuits before evaluating the lambda. This PR will be rebased onto main once apache#22853 merges, at which point the ignore markers will be removed and the sqllogictest file restored.
1 parent b342185 commit 70ba67b

5 files changed

Lines changed: 23 additions & 165 deletions

File tree

datafusion/functions-nested/src/transform_values.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,7 @@ mod tests {
337337
}
338338

339339
#[test]
340+
#[ignore = "blocked on apache/datafusion#22853: multi-param lambda projection fix"]
340341
fn transform_values_doubles_values() {
341342
let map = create_str_int_map(
342343
vec!["a", "b", "c"],
@@ -389,6 +390,7 @@ mod tests {
389390
}
390391

391392
#[test]
393+
#[ignore = "blocked on apache/datafusion#22853: multi-param lambda projection fix"]
392394
fn transform_values_preserves_null_rows() {
393395
let map = create_str_int_map(
394396
vec!["a", "b", "c", "d"],
@@ -422,6 +424,7 @@ mod tests {
422424
}
423425

424426
#[test]
427+
#[ignore = "blocked on apache/datafusion#22853: multi-param lambda projection fix"]
425428
fn transform_values_empty_map() {
426429
let map = create_str_int_map(
427430
vec![],
@@ -440,6 +443,7 @@ mod tests {
440443
}
441444

442445
#[test]
446+
#[ignore = "blocked on apache/datafusion#22853: multi-param lambda projection fix"]
443447
fn transform_values_mixed_empty_null_populated_rows() {
444448
// Row 0: empty {}, row 1: NULL, row 2: {a: 1, b: 2, c: 3}, row 3: {d: 4}
445449
let map = create_str_int_map(
@@ -474,6 +478,7 @@ mod tests {
474478
}
475479

476480
#[test]
481+
#[ignore = "blocked on apache/datafusion#22853: multi-param lambda projection fix"]
477482
fn transform_values_on_sliced_map_should_not_evaluate_on_unreachable_values() {
478483
// Build 4 map rows then slice off the first row. The value `0` lives in
479484
// the unreachable prefix; if the lambda `100/v` were evaluated on it we

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

Lines changed: 9 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,6 @@ 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,
5146
}
5247

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

6762
impl LambdaExpr {
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> {
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> {
8165
if !all_unique(&params) {
8266
return plan_err!(
8367
"lambda params must be unique, got ({})",
@@ -87,14 +71,10 @@ impl LambdaExpr {
8771

8872
check_async_udf(&body)?;
8973

90-
Ok(Self::new(params, body, outer_columns_count))
74+
Ok(Self::new(params, body))
9175
}
9276

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

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

11393
projection.sort();
11494

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();
12495
let column_index_map = projection
12596
.iter()
12697
.enumerate()
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-
})
98+
.map(|(projected, original)| (*original, projected))
13599
.collect::<HashMap<_, _>>();
136100

137101
let projected_body = Arc::clone(&body)
@@ -165,7 +129,6 @@ impl LambdaExpr {
165129
body,
166130
projected_body,
167131
projection,
168-
outer_columns_count,
169132
}
170133
}
171134

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

225188
check_async_udf(body)?;
226189

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

234193
fn fmt_sql(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
235194
write!(f, "({}) -> {}", self.params.join(", "), self.body)
236195
}
237196
}
238197

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.
198+
/// Create a lambda expression
243199
pub fn lambda(
244200
params: impl IntoIterator<Item = impl Into<String>>,
245201
body: Arc<dyn PhysicalExpr>,
246-
outer_columns_count: usize,
247202
) -> Result<Arc<dyn PhysicalExpr>> {
248203
Ok(Arc::new(LambdaExpr::try_new(
249204
params.into_iter().map(Into::into).collect(),
250205
body,
251-
outer_columns_count,
252206
)?))
253207
}
254208

@@ -286,13 +240,13 @@ mod tests {
286240

287241
#[test]
288242
fn test_lambda_evaluate() {
289-
let lambda = lambda(["a"], Arc::new(NoOp::new()), 0).unwrap();
243+
let lambda = lambda(["a"], Arc::new(NoOp::new())).unwrap();
290244
let batch = RecordBatch::new_empty(Arc::new(Schema::empty()));
291245
assert!(lambda.evaluate(&batch).is_err());
292246
}
293247

294248
#[test]
295249
fn test_lambda_duplicate_name() {
296-
assert!(lambda(["a", "a"], Arc::new(NoOp::new()), 0).is_err());
250+
assert!(lambda(["a", "a"], Arc::new(NoOp::new())).is_err());
297251
}
298252
}

datafusion/physical-expr/src/higher_order_function.rs

Lines changed: 5 additions & 7 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())), 0).unwrap()],
631+
vec![lambda(["a"], Arc::new(Literal::new(expected.clone()))).unwrap()],
632632
&Schema::empty(),
633633
Arc::new(ConfigOptions::new()),
634634
)
@@ -664,12 +664,10 @@ mod tests {
664664
let hof = HigherOrderFunctionExpr::try_new_with_schema(
665665
fun,
666666
vec![
667-
not(lambda(
668-
["a"],
669-
Arc::new(Literal::new(ScalarValue::Int32(Some(42)))),
670-
0,
667+
not(
668+
lambda(["a"], Arc::new(Literal::new(ScalarValue::Int32(Some(42)))))
669+
.unwrap(),
671670
)
672-
.unwrap())
673671
.unwrap(),
674672
],
675673
&Schema::empty(),
@@ -709,7 +707,7 @@ mod tests {
709707
.unwrap();
710708

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

715713
assert_contains!(

datafusion/physical-expr/src/planner.rs

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -513,20 +513,10 @@ pub fn create_physical_expr(
513513
config_options,
514514
)?))
515515
}
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-
}
516+
Expr::Lambda(Lambda { params, body }) => expressions::lambda(
517+
params,
518+
create_physical_expr(body, input_dfschema, execution_props)?,
519+
),
530520
Expr::LambdaVariable(LambdaVariable {
531521
name,
532522
field,

datafusion/sqllogictest/test_files/map/transform_values.slt

Lines changed: 0 additions & 89 deletions
This file was deleted.

0 commit comments

Comments
 (0)