timsaucer commented on code in PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#discussion_r3751023832
##########
datafusion/expr/src/higher_order_function.rs:
##########
@@ -257,26 +270,49 @@ pub struct LambdaArgument {
}
impl LambdaArgument {
+ /// Build a [`LambdaArgument`] for a lambda whose body references the
+ /// subset of `params` named in `used_params`.
+ ///
+ /// [`Self::evaluate`] only materialises the closures whose parameter name
+ /// appears in `used_params`, preserving the original declaration order of
+ /// `params`. Unused declared parameters therefore leave no slot in the
+ /// merged batch, so the body's compressed column indices line up directly
+ /// with the columns the evaluator built.
+ ///
+ /// Callers with a `LambdaExpr` in hand should pass `lambda.used_params()`;
+ /// that method already computes the exact set required here (with
+ /// nested-lambda shadow tracking).
Review Comment:
Here's a problem I have with generated code documentation: LLMs of write
this kind of documentation based on the diff of the code they're writing and
*not* based on end user viewpoint. From a consumer of this function, the
purpose of this is to create a new `LambdaArgument`. Instead if we read the
docstring it's heavily focused on the purpose of `used_params`. Now this does
have documentation where the prior `new` function didn't, so that's generally
an improvement but the way this is written feels not obvious to me as a user of
datafusion who isn't narrowly looking at the problem that this PR addresses.
I'm assuming this was generated documentation, so please correct me if I'm
wrong. I've had this problem with my own PRs as well and it's one of the things
I've had to add extra instructions in my agents' context just to avoid these
kinds of documentation that are PR-facing rather than user-facing.
##########
datafusion/physical-expr/src/expressions/lambda.rs:
##########
@@ -43,6 +43,15 @@ pub struct LambdaExpr {
body: Arc<dyn PhysicalExpr>,
projected_body: Arc<dyn PhysicalExpr>,
projection: Vec<usize>,
+ /// Subset of `params` (by name) that the body actually references,
+ /// computed with nested-lambda shadow tracking. Empty when no parameter
Review Comment:
TBH the term "nested-lambda shadow tracking" doesn't have obvious meaning to
me. Is there an easy way to make the meaning more clear, or somewhere else in
the code I should have looked to understand what it means?
##########
datafusion/expr/src/higher_order_function.rs:
##########
@@ -365,23 +403,43 @@ fn merge_captures_with_variables(
);
}
+ let push_param_arrays = |columns: &mut Vec<ArrayRef>| -> Result<()> {
+ for &i in used_param_indices {
+ columns.push(variables[i]()?);
+ }
+ 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.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)),
+ )?);
+ }
+
Review Comment:
Is this a separate issue that's caught and included here or was this
introduced by the above changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
