You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "LiaCastaneda (via GitHub)" <gi...@apache.org> on 2026/08/07 10:34:25 UTC
[PR] fix(lambda): only push referenced params into the merged batch [datafusion]
LiaCastaneda opened a new pull request, #24162:
URL: https://github.com/apache/datafusion/pull/24162
## Which issue does this PR close?
reopens https://github.com/apache/datafusion/pull/22853
## Rationale for this change
current lambdas only take a single parameter `(v -> ...)`, so nobody had noticed that `LambdaExpr` mishandles lambdas with more than one parameter. The bug surfaced while working on `transform_values` (#22689), which needs `(k, v) -> expr ` two parameters, one of which is very often unused (e.g. `(k, v) -> v * 2`, k never referenced).
The bug is that when a higher order function with more than 1 param evaluates a lambda, it fills each parameter into a slot based on its declared position — for example for `(k, v) -> v` `k` always goes into slot 0, `v` always into slot 1. `LambdaExpr` separately scans the body and renumbers whatever it finds referenced into a dense `0..n` range, to avoid carrying around columns nothing uses (like `v` in this case). That renumbering is fine for outer captures, but applying it to the lambda's own parameters is wrong, because it changes where the body looks for a value without changing where the evaluator put it.
### Example:
in `(k, v) -> v` `v` is declared second (slot 1), but since it's the only parameter the body references, the renumbering logic reassigns it to slot 0. The evaluator, unaware of this, writes `k`'s values into slot 0 and `v`'s into slot 1. So the body ends up reading slot 0 expecting `v` — and gets `k` instead. So the results end up being incorrect.
## What changes are included in this PR?
- `LambdaExpr` now computes `used_params`: which is the subset of its own declared parameters that are actually referenced in the body.
- `LambdaArgument::new` takes `used_params` and only pushes the referenced parameters in the body into the merged batch, in original declaration order — so the body's indices always line up with what's actually built.
- `HigherOrderFunctionExpr::evaluate` forwards `lambda.used_params()` to `LambdaArgument::new`
## Are these changes tested?
yes, added two new tests one for the unused-parameter case and nested-lambda for the shadowing case.
## Are there any user-facing changes?
The only public api change is on `LambdaArgument::new ` which now requires a new argument: `used_params: &HashSet<String>`, however LambdaArgument::new is very unlikely to be called outside datafusion, see [this](https://github.com/apache/datafusion/pull/22853#discussion_r3527236493) comment
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#issuecomment-5242701306
> This is a case where the current lambda functions are all working because they're single variable, right?
Yep exactly, this came up while @Adam-Alani (the original author of the fix) was working on a PR to add `transform_values`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#issuecomment-5242641525
This is a case where the current lambda functions are all working because they're single variable, right? I am wondering if this is necessary to get into #22393 or if it's okay going in the next release.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#issuecomment-5251514343
> The cost is a new failure mode. LambdaVariable can be built directly (proto decode, lambda_variable.rs:189, tests) with a field name that doesn't match the declared param. Before this PR a mismatch was harmless; now it silently drops the param from used_params, the column never gets pushed, and the body reads the wrong slot — no error, wrong answers.
this is unlikely but not impossible, if it does differ it would be a coding bug, however its true its probably cleanest to just handle the indices instead of names
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#discussion_r3756483013
##########
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:
when a lambda has nested lambdas like `(k, v) -> func(col, (k, v2) -> k + v2 + v)`, the innermost `k` "shadows"/"overrides" the outermost `k` bc even though they share a name, the innermost `k` is its own separate parameter, not a reference to the outer one. So when collecting which parameters a lambda's body references n a nested lambda, we need to take this possible name conflict into account, otherwise we'd wrongly count the outer `k` as used just because the name appears in the body of the nested lambda.
Its true its a bit unclear by just reading it in the param description, I moved the explanation to `CollectUsedVisitor` that actually handles the logic for "shadowed" params
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#discussion_r3759804228
##########
datafusion/expr/src/higher_order_function.rs:
##########
@@ -261,22 +274,27 @@ impl LambdaArgument {
params: Vec<FieldRef>,
body: Arc<dyn PhysicalExpr>,
captures: Option<RecordBatch>,
+ used_param_indices: &[usize],
) -> Self {
- let fields = match &captures {
+ let used_param_indices = used_param_indices.to_vec();
+ let effective_params = used_param_indices.iter().map(|i| Arc::clone(¶ms[*i]));
Review Comment:
make sense, done.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#issuecomment-5216017849
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-expr v54.1.0 (current)
Built [ 41.683s] (current)
Parsing datafusion-expr v54.1.0 (current)
Parsed [ 0.074s] (current)
Building datafusion-expr v54.1.0 (baseline)
Built [ 28.227s] (baseline)
Parsing datafusion-expr v54.1.0 (baseline)
Parsed [ 0.074s] (baseline)
Checking datafusion-expr v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 1.373s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure method_parameter_count_changed: pub method parameter count changed ---
Description:
A publicly-visible method now takes a different number of parameters, not counting the receiver (self) parameter.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/method_parameter_count_changed.ron
Failed in:
datafusion_expr::LambdaArgument::new takes 3 parameters in /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4b792979f6c319c0b4d1659e959af80888909453/datafusion/expr/src/higher_order_function.rs:260, but now takes 4 parameters in /home/runner/work/datafusion/datafusion/datafusion/expr/src/higher_order_function.rs:285
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 72.731s] datafusion-expr
Building datafusion-physical-expr v54.1.0 (current)
Built [ 29.921s] (current)
Parsing datafusion-physical-expr v54.1.0 (current)
Parsed [ 0.049s] (current)
Building datafusion-physical-expr v54.1.0 (baseline)
Built [ 29.695s] (baseline)
Parsing datafusion-physical-expr v54.1.0 (baseline)
Parsed [ 0.051s] (baseline)
Checking datafusion-physical-expr v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.325s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 61.827s] datafusion-physical-expr
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#discussion_r3756621582
##########
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:
sort of introduced by our changes. Before, we pushed all params (used or not) into `columns` so the variable was never empty. Now we just push the used params only, so for lambdas that use 0 params (like `(k,v) -> constant`) `columns` is empty, so we need to derive the row count manually (otherwise `RecordBatch::try_new(schema, columns) `wont do it, like before)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer merged PR #24162:
URL: https://github.com/apache/datafusion/pull/24162
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#issuecomment-5216286023
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24162?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `96.15385%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.02%. Comparing base ([`0646a31`](https://app.codecov.io/gh/apache/datafusion/commit/0646a310cdeb25ba6a091e643dddd06da7ea181e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a2b3dee`](https://app.codecov.io/gh/apache/datafusion/commit/a2b3dee659676166d4dfa592b02f03247481ae53?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24162?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/expr/src/higher\_order\_function.rs](https://app.codecov.io/gh/apache/datafusion/pull/24162?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Fhigher_order_function.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9oaWdoZXJfb3JkZXJfZnVuY3Rpb24ucnM=) | 81.81% | [2 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24162?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24162 +/- ##
========================================
Coverage 81.02% 81.02%
========================================
Files 1105 1105
Lines 380669 380803 +134
Branches 380669 380803 +134
========================================
+ Hits 308446 308561 +115
- Misses 53994 54008 +14
- Partials 18229 18234 +5
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24162?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#issuecomment-5251284577
> I am wondering if this is necessary to get into https://github.com/apache/datafusion/issues/22393
it would be nice if its on the new release
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#issuecomment-5251705974
@timsaucer I addressed most of the review points :)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#discussion_r3758757440
##########
datafusion/expr/src/higher_order_function.rs:
##########
@@ -261,22 +274,27 @@ impl LambdaArgument {
params: Vec<FieldRef>,
body: Arc<dyn PhysicalExpr>,
captures: Option<RecordBatch>,
+ used_param_indices: &[usize],
) -> Self {
- let fields = match &captures {
+ let used_param_indices = used_param_indices.to_vec();
+ let effective_params = used_param_indices.iter().map(|i| Arc::clone(¶ms[*i]));
Review Comment:
There is a potential for a panic here if `used_param_indices` ever contains an index that is larger than the length of `params`. In practice, not a problem but it looks like this is a `pub` function so we at least need to document the invariant. Maybe even a `debug_assert`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#discussion_r3751337251
##########
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:
I don't even think we need doc for this function, the function itself is very self-explanatory (and also [it will probably never be used ](https://github.com/apache/datafusion/pull/22853#discussion_r3527236493)outside datafusion)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(lambda): only push referenced params into the merged batch [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24162:
URL: https://github.com/apache/datafusion/pull/24162#discussion_r3751323217
##########
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:
yeah, I read through it and didn't find it irrelevant but its true it can be simplified since it only talks about the `params` param
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org