Skip to content

Commit 26bc713

Browse files
committed
address pr review
1 parent 0a3f2e6 commit 26bc713

6 files changed

Lines changed: 57 additions & 11 deletions

File tree

‎datafusion/expr/src/higher_order_function.rs‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -262,25 +262,33 @@ impl LambdaArgument {
262262
/// `args` should evaluate to the value of each parameter
263263
/// of the correspondent lambda returned in [HigherOrderUDF::lambda_parameters].
264264
///
265-
/// `adjust` should adjust the captured columns of this
266-
/// lambda, if any, relative to it's parameters
265+
/// `spread_captures` is responsible for transforming the captured column arrays
266+
/// so they align with the evaluation batch. Captures are snapshotted from the
267+
/// outer batch at construction time, giving one value per outer row, but the
268+
/// function may evaluate the lambda body over a batch with a different number
269+
/// of rows. It is the function responsibility to provide the appropriate `spread_captures`
270+
/// closure to expand (or otherwise reshape) the captures to match.
271+
/// Function working on lists, for example `array_transform(arr, v -> v + 1)`
272+
/// flattens all list elements into a single batch, duplicating captured
273+
/// values for rows with multiple elements and dropping them for empty lists.
274+
/// If the lambda has no captures, `spread_captures` is never called.
267275
pub fn evaluate(
268276
&self,
269277
args: &[&dyn Fn() -> Result<ArrayRef>],
270-
adjust: impl FnOnce(&[ArrayRef]) -> Result<Vec<ArrayRef>>,
278+
spread_captures: impl FnOnce(&[ArrayRef]) -> Result<Vec<ArrayRef>>,
271279
) -> Result<ColumnarValue> {
272-
let adjusted_captures = self
280+
let spread_captures = self
273281
.captures
274282
.as_ref()
275283
.map(|captures| {
276-
let adjusted_columns = adjust(captures.columns())?;
284+
let spread_columns = spread_captures(captures.columns())?;
277285

278-
RecordBatch::try_new(captures.schema(), adjusted_columns)
286+
RecordBatch::try_new(captures.schema(), spread_columns)
279287
})
280288
.transpose()?;
281289

282290
let merged = merge_captures_with_variables(
283-
adjusted_captures.as_ref(),
291+
spread_captures.as_ref(),
284292
Arc::clone(&self.schema),
285293
&self.params,
286294
args,

‎datafusion/physical-expr/src/expressions/case.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,9 @@ use datafusion_common::{
3333
internal_datafusion_err, internal_err,
3434
};
3535
use datafusion_expr::ColumnarValue;
36-
use indexmap::{IndexMap, IndexSet};
36+
use indexmap::IndexMap;
3737
use std::borrow::Cow;
38+
use std::collections::BTreeSet;
3839
use std::hash::Hash;
3940
use std::sync::Arc;
4041

@@ -128,7 +129,8 @@ impl CaseBody {
128129
/// Derives a [ProjectedCaseBody] from this [CaseBody].
129130
fn project(&self) -> Result<ProjectedCaseBody> {
130131
// Determine the set of columns that are used in all the expressions of the case body.
131-
let mut used_column_indices = IndexSet::<usize>::new();
132+
// Use an ordered set so lambda variables continue to be positioned after columns
133+
let mut used_column_indices = BTreeSet::<usize>::new();
132134
let mut collect_column_indices = |expr: &Arc<dyn PhysicalExpr>| {
133135
expr.apply(|expr| {
134136
if let Some(column) = expr.downcast_ref::<Column>() {

‎datafusion/physical-expr/src/expressions/column.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ impl PhysicalExpr for Column {
124124
}
125125

126126
fn return_field(&self, input_schema: &Schema) -> Result<FieldRef> {
127+
self.bounds_check(input_schema)?;
127128
Ok(input_schema.field(self.index).clone().into())
128129
}
129130

‎datafusion/physical-expr/src/expressions/lambda_variable.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,8 @@ impl PhysicalExpr for LambdaVariable {
107107

108108
if self.field.as_ref() != batch.schema_ref().field(self.index) {
109109
return exec_err!(
110-
"Physical LambdaVariable field doesn't match batch field during evaluation {} != {}",
110+
"Field of physical LambdaVariable with index {} doesn't match batch field during evaluation {} != {}",
111+
self.index,
111112
self.field,
112113
batch.schema_ref().field(self.index)
113114
);

‎datafusion/physical-expr/src/higher_order_function.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,7 @@ mod tests {
568568
match &args.args[0] {
569569
ValueOrLambda::Lambda(lambda) => lambda.evaluate(
570570
&[&|| Ok(Arc::new(NullArray::new(args.number_rows)))],
571-
|_| unreachable!(),
571+
|arrays| Ok(arrays.to_vec()),
572572
),
573573
ValueOrLambda::Value(value) => Ok(value.clone()),
574574
}

‎datafusion/sqllogictest/test_files/array/array_transform.slt‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,40 @@ a [1, 50] 10 [[[11, 11]]]
221221
b [4, 50] 40 [[[48, 94]]]
222222
c [7, 50] 60 [[[74, 117]]]
223223

224+
# case with inner nested higher order function where the condition is a lambda function
225+
query T?I?
226+
select
227+
t.text,
228+
t.list,
229+
t.number,
230+
case
231+
when array_any_match(t.list, v -> v = 7) then array_transform(
232+
[[t.list]],
233+
list -> array_transform(
234+
list,
235+
list -> array_transform(
236+
list,
237+
v -> number + v + list[1]
238+
)
239+
)
240+
)
241+
else array_transform(
242+
[[t.list]],
243+
list -> array_transform(
244+
list,
245+
list -> array_transform(
246+
list,
247+
v -> number + list[1]
248+
)
249+
)
250+
)
251+
end
252+
from t
253+
order by t.number;
254+
----
255+
a [1, 50] 10 [[[11, 11]]]
256+
b [4, 50] 40 [[[44, 44]]]
257+
c [7, 50] 60 [[[74, 117]]]
224258

225259
# array_transform coercion rules
226260
query TT

0 commit comments

Comments
 (0)