-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(lambda): only push referenced params into the merged batch #22853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ use crate::expr::{ | |
| use crate::type_coercion::functions::value_fields_with_higher_order_udf; | ||
| use crate::udf_eq::UdfEq; | ||
| use crate::{ColumnarValue, Documentation, Expr, ExprSchemable}; | ||
| use arrow::array::{ArrayRef, RecordBatch}; | ||
| use arrow::array::{ArrayRef, RecordBatch, RecordBatchOptions}; | ||
| use arrow::datatypes::{DataType, FieldRef, Schema}; | ||
| use arrow_schema::SchemaRef; | ||
| use datafusion_common::config::ConfigOptions; | ||
|
|
@@ -239,6 +239,15 @@ pub struct LambdaArgument { | |
| /// For example, for `array_transform([2], v -> -v)`, | ||
| /// this will be `vec![Field::new("v", DataType::Int32, true)]` | ||
| params: Vec<FieldRef>, | ||
| /// Indices into `params` of the parameters that are actually referenced | ||
| /// by `body` (taking nested-lambda shadowing into account). | ||
| /// | ||
| /// `None` means "no information, assume every declared parameter is used" | ||
| /// — that is the backwards-compatible behavior of [`Self::new`]. When set, | ||
| /// [`Self::evaluate`] skips evaluating and pushing the closures for the | ||
| /// parameters not listed here, so unused declared parameters do not shift | ||
| /// the columns the body's compressed indices expect. | ||
| used_param_indices: Option<Vec<usize>>, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we pass |
||
| /// The body of the lambda | ||
| /// | ||
| /// For example, for `array_transform([2], v -> -v)`, | ||
|
|
@@ -257,26 +266,64 @@ pub struct LambdaArgument { | |
| } | ||
|
|
||
| impl LambdaArgument { | ||
| /// Build a [`LambdaArgument`] that treats every declared parameter as | ||
| /// used. This is the backwards-compatible behavior. Prefer | ||
| /// [`Self::new_with_used_params`] when the caller knows which subset of | ||
| /// the lambda's parameters the body actually references — otherwise the | ||
| /// merged batch will still contain columns for unused parameters. | ||
| pub fn new( | ||
| params: Vec<FieldRef>, | ||
| body: Arc<dyn PhysicalExpr>, | ||
| captures: Option<RecordBatch>, | ||
| ) -> Self { | ||
| Self::new_with_used_params(params, body, captures, None) | ||
| } | ||
|
|
||
| /// Build a [`LambdaArgument`] knowing which subset of `params` (by name) | ||
| /// the lambda body actually references. | ||
| /// | ||
| /// When `used_params` is `Some(set)`, [`Self::evaluate`] only evaluates | ||
| /// and pushes the closures whose corresponding parameter name is in | ||
| /// `set`, in the original declaration order of `params`. Unused declared | ||
| /// parameters leave no slot in the merged batch, so the body's compressed | ||
| /// column indices line up directly. When `used_params` is `None`, | ||
| /// behavior is identical to [`Self::new`]. | ||
|
Comment on lines
+289
to
+290
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we just require |
||
| pub fn new_with_used_params( | ||
| params: Vec<FieldRef>, | ||
| body: Arc<dyn PhysicalExpr>, | ||
| captures: Option<RecordBatch>, | ||
| used_params: Option<HashSet<String>>, | ||
| ) -> Self { | ||
| let used_param_indices = used_params.map(|set| { | ||
| params | ||
| .iter() | ||
| .enumerate() | ||
| .filter(|(_, f)| set.contains(f.name())) | ||
| .map(|(i, _)| i) | ||
| .collect::<Vec<_>>() | ||
| }); | ||
|
|
||
| let effective_params: Vec<FieldRef> = match &used_param_indices { | ||
| Some(indices) => indices.iter().map(|i| Arc::clone(¶ms[*i])).collect(), | ||
| None => params.clone(), | ||
| }; | ||
|
|
||
| let fields = match &captures { | ||
| Some(batch) => batch | ||
| .schema_ref() | ||
| .fields() | ||
| .iter() | ||
| .cloned() | ||
| .chain(params.clone()) | ||
| .chain(effective_params) | ||
| .collect(), | ||
| None => params.clone(), | ||
| None => effective_params, | ||
| }; | ||
|
|
||
| let schema = Arc::new(Schema::new(fields)); | ||
|
|
||
| Self { | ||
| params, | ||
| used_param_indices, | ||
| body, | ||
| schema, | ||
| captures, | ||
|
|
@@ -344,6 +391,7 @@ impl LambdaArgument { | |
| spread_captures.as_ref(), | ||
| Arc::clone(&self.schema), | ||
| &self.params, | ||
| self.used_param_indices.as_deref(), | ||
| args, | ||
| )?; | ||
|
|
||
|
|
@@ -355,6 +403,7 @@ fn merge_captures_with_variables( | |
| captures: Option<&RecordBatch>, | ||
| schema: SchemaRef, | ||
| params: &[FieldRef], | ||
| used_param_indices: Option<&[usize]>, | ||
| variables: &[&dyn Fn() -> Result<ArrayRef>], | ||
| ) -> Result<RecordBatch> { | ||
| if variables.len() < params.len() { | ||
|
|
@@ -365,23 +414,56 @@ fn merge_captures_with_variables( | |
| ); | ||
| } | ||
|
|
||
| let push_param_arrays = |columns: &mut Vec<ArrayRef>| -> Result<()> { | ||
| match used_param_indices { | ||
| Some(indices) => { | ||
| for &i in indices { | ||
| columns.push(variables[i]()?); | ||
| } | ||
| } | ||
| None => { | ||
| for arg in &variables[..params.len()] { | ||
| columns.push(arg()?); | ||
| } | ||
| } | ||
| } | ||
| Ok(()) | ||
| }; | ||
|
|
||
| let columns = match captures { | ||
| Some(captures) => { | ||
| let mut columns = captures.columns().to_vec(); | ||
|
|
||
| for arg in &variables[..params.len()] { | ||
| columns.push(arg()?); | ||
| } | ||
|
|
||
| push_param_arrays(&mut columns)?; | ||
| columns | ||
| } | ||
| None => { | ||
| let mut columns = Vec::with_capacity( | ||
| used_param_indices | ||
| .map(<[usize]>::len) | ||
| .unwrap_or(params.len()), | ||
| ); | ||
| push_param_arrays(&mut columns)?; | ||
| columns | ||
| } | ||
| None => variables | ||
| .iter() | ||
| .take(params.len()) | ||
| .map(|arg| arg()) | ||
| .collect::<Result<_>>()?, | ||
| }; | ||
|
|
||
| if columns.is_empty() { | ||
| // Constant lambda body with no captures and no used parameters. We | ||
| // still need a row count for the merged batch, so evaluate one | ||
| // variable just to derive it. This is essentially free in the common | ||
| // case (the variables already exist as closures over arrays the | ||
| // caller computed up front). | ||
| let row_count = match variables.first() { | ||
| Some(first) => first()?.len(), | ||
| None => 0, | ||
| }; | ||
| return Ok(RecordBatch::try_new_with_options( | ||
| schema, | ||
| vec![], | ||
| &RecordBatchOptions::new().with_row_count(Some(row_count)), | ||
| )?); | ||
| } | ||
|
|
||
| Ok(RecordBatch::try_new(schema, columns)?) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Providing an api where we have to specify the indices of the parameters that will be used in the body feels a bit unergonomic. Have you considered somehow extracting this from the body and handling this internally so it's invisible to the caller?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the body is dynamic, which technically means for higher order functions indices positions can differ for each query no? for example if you have a Higher Order function with parameters (x,y,x) for a given query you can use x,y or y or all of them. This essentially means external callers of
LambdaArgument::new_with_used_paramswould have to walk the body themselves to figure out which params are referenced.edit: is it actually possible to build a
LambdaArgumentlike as an api? they are technically build in DF before calling the invoke_with_args api 🤔 so I guessnew_with_used_paramswould only be called inhigher_order_function.rsinevaluateinside DataFusion. If that's the case, I would consider resolving this insideLambdaExprinstead, keeping the fix self-contained thereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, is dynamic and require walking the body, but at least for this PR, it's folded with the walk that collect used indices.
Technically yes but not because I imagined a usecase for it outside datafusion, but only to be able to use it in
evaluatewhich is within another crateSome other ideas besides #22853 (comment):
LambdaExpritself (instead of it's body) as theArc<dyn PhyiscalExpr>parameter ofLambdaArgument::new, soevaluatecan get the used params by downcasting the expr toLambdaExpr(or returning an error if it fails)LambdaArgument::evaluate, at the small cost of a tree traversal per evaluation instead of only during planning