Skip to content

Add transform_values UDF - #22689

Open
Adam-Alani wants to merge 8 commits into
apache:mainfrom
Adam-Alani:main
Open

Add transform_values UDF#22689
Adam-Alani wants to merge 8 commits into
apache:mainfrom
Adam-Alani:main

Conversation

@Adam-Alani

@Adam-AlaniAdam-Alani commented Jun 1, 2026

Copy link
Copy Markdown

Blocked on #22853 (the LambdaExpr projection fix). This PR has been reverted to drop the breaking change per maintainer request; the unit tests that exercise (k, v) -> body lambdas are temporarily #[ignore]-marked and the sqllogictest file has been removed.

Rationale for this change

We want to give SQL users a way to rewrite the values of a map without re-implementing the key/value plumbing themselves. DataFusion already has array_transform from #18921 as a precedent for higher-order array functions, so this is the natural map-side equivalent.

What changes are included in this PR?

  • Add transform_values(map, (k, v) -> expr) as a higher-order UDF in datafusion-functions-nested. The function applies the lambda to every entry, returns a new map with the original keys and the lambda's results as the new values, and propagates null rows.
  • New helper get_map_key_value_fields in functions-nested/src/utils.rs so other map UDFs can share the "get the key/value field refs out of a Map" pattern.
  • Unit tests covering happy-path transformation, key references, null-row propagation, empty maps, mixed (empty + null + populated) rows in one batch, and sliced inputs (offset handling).

The breaking change to LambdaExpr that originally lived in this PR has been extracted to #22853.

Adam-Alaniand others added 3 commits June 1, 2026 10:29
Introduce a new higher-order UDF that returns a new map by applying a
lambda `(k, v) -> expr` to each entry of the input map, transforming the
values while preserving the keys. Registered under the alias
`transform_values` via `with_aliases` from apache#22593.
Also fix a latent indexing bug in `LambdaExpr`: when a multi-parameter
lambda referenced only a subset of its declared parameters (e.g.
`(k, v) -> v * 2`), the compressed column index map could shift lambda
params into outer capture slots. `LambdaExpr` now tracks
`outer_columns_count` and remaps indices so outer captures and lambda
params keep stable positions regardless of which params are used.
Keep a single canonical name `map_transform` and remove the
`transform_values` alias to simplify registration (no more
`Arc::new(... .with_aliases(...))` dance in `lib.rs`).
Add map_transform UDF
@github-actionsgithub-actionsBot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Jun 1, 2026
@Adam-Alani
Adam-Alani marked this pull request as ready for review June 1, 2026 08:51
@Adam-AlaniAdam-Alani mentioned this pull request Jun 1, 2026
30 tasks
@github-actionsgithub-actionsBot added the auto detected api change Auto detected API change label Jun 1, 2026
Run `./dev/update_function_docs.sh` so the user-facing function
reference picks up the new `map_transform` entry from its
`#[user_doc(...)]` annotation.
@github-actionsgithub-actionsBot added the documentation Improvements or additions to documentation label Jun 1, 2026
Comment threaddatafusion/physical-expr/src/expressions/lambda.rs Outdated
Comment threaddatafusion/sqllogictest/test_files/map/map_transform.slt Outdated
Comment threaddatafusion/functions-nested/src/transform_values.rs
Comment threaddatafusion/functions-nested/src/map_transform.rs Outdated
Comment threaddatafusion/functions-nested/src/map_transform.rs Outdated
Comment threaddatafusion/functions-nested/src/map_transform.rs Outdated
Comment threaddatafusion/functions-nested/src/map_transform.rs Outdated
Comment threaddatafusion/functions-nested/src/map_transform.rs
Comment threaddatafusion/sqllogictest/test_files/map/transform_values.slt Outdated
Comment threaddatafusion/functions-nested/src/map_transform.rs Outdated
Comment threaddatafusion/functions-nested/src/map_transform.rs Outdated
Rename `map_transform` to `transform_values` (Spark/Trino spelling, leaves room
for a future `transform_keys`), extract shared helpers in this file and
`utils.rs` (`get_map_key_value_fields`, reuse `value_lambda_pair` from
`lambda_utils`), return an array of nulls (instead of a null scalar) when every
input row is null, and move the empty-entries fast path up next to the all-null
fast path. Also add a sqllogictest covering the all-null rows behavior and
regenerate the scalar function docs.
@Adam-AlaniAdam-Alani changed the title Add map_transform UDFAdd transform_values UDFJun 8, 2026

@LiaCastanedaLiaCastaneda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lgtm, from the understanding the bug this PR fixes (aside from adding transform_values) is:

  1. We have map_transform(my_map, (k, v) -> v * 2)
  2. The Recordbatch the lambda processes has two columns k and v (indices 0,1)
  3. LambdaExpr projects the batch so it keeps the columns the lambda body actually references (v with index 1)
  4. v ends up with index 0 in the projected schema
  5. The body will read the wrong column in index 0 (which is k and not v)

@LiaCastanedaLiaCastaneda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabotechs or @rluvaton -- whenever you have time, could you take a look at this PR? It adds transform_values and fixes a lambda indexing bug

Comment threaddatafusion/sqllogictest/test_files/map/transform_values.slt Outdated
Comment threaddatafusion/functions-nested/src/transform_values.rs Outdated
}

#[cfg(test)]
mod tests {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ I'm 99% sure you have a bug here since clear_null_values currently doesn't support MapArray (I will create a PR now to add support for that, so the test will probably fail until my pr is merged).

Please add tests like:

#[cfg(test)]
mod tests {
use arrow::{
array::{Array,AsArray},
buffer::{NullBuffer,OffsetBuffer},
};
usecrate::array_transform::array_transform_higher_order_function;
usecrate::lambda_utils::test_utils::{create_i32_list, eval_hof_on_i32_list, v};
use datafusion_expr::lit;
fndivide_100_by(
list:implArray + Clone + 'static,
) -> datafusion_common::Result<arrow::array::ArrayRef>{
eval_hof_on_i32_list(
array_transform_higher_order_function(),
list,
lit(100i32) / v(),
)
}
#[test]
fntransform_on_sliced_list_should_not_evaluate_on_unreachable_values(){
let list = create_i32_list(
vec![
// Have 0 here so if the expression is called on data that it will fail
0,4,100,25,20,5,2,1,10,
],
OffsetBuffer::<i32>::from_lengths(vec![1,3,4,1]),
None,
)
.slice(1,3);
let res = divide_100_by(list).unwrap();
let actual_list = res.as_list::<i32>();
let expected_list = create_i32_list(
vec![25,1,4,5,20,50,100,10],
OffsetBuffer::<i32>::from_lengths(vec![3,4,1]),
None,
);
assert_eq!(actual_list,&expected_list);
}
#[test]
fntransform_function_should_not_be_evaluated_on_values_underlying_null(){
let list = create_i32_list(
// 0 here for one of the values behind null, so if it will be evaluated
// it will fail due to divide by 0
vec![100,20,10,0,1,2,0,1,50],
OffsetBuffer::<i32>::from_lengths(vec![3,4,2]),
Some(NullBuffer::from(vec![true,false,true])),
);
let res = divide_100_by(list).unwrap();
let actual_list = res.as_list::<i32>();
let expected_list = create_i32_list(
vec![1,5,10,100,2],
OffsetBuffer::<i32>::from_lengths(vec![3,0,2]),
Some(NullBuffer::from(vec![true,false,true])),
);
assert_eq!(actual_list.data_type(), expected_list.data_type());
assert_eq!(actual_list,&expected_list);
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds four unit tests requested in PR review:
- `transform_values_empty_map`: single row, empty entries
- `transform_values_mixed_empty_null_populated_rows`: empty + null + populated
rows in one batch to exercise offset/null-mask plumbing
- `transform_values_on_sliced_map_should_not_evaluate_on_unreachable_values`:
slices a 4-row map and uses `100 / v` so a leading `0` would divide-by-zero
if offset handling is wrong
- `transform_values_function_should_not_be_evaluated_on_values_underlying_null`
(ignored): documents the dependency on apache#22847 which adds
`clear_null_values` support for `MapArray`
While adding the empty-map test, the previously-existing "empty entries"
fast-path was found to be broken — `ScalarValue::new_default` for a `Map`
return type produces a scalar that decodes back as a `Struct`, not a `Map`,
and the test downcast failed. The fast path was just an optimisation skipping
the lambda evaluation; the regular path already handles empty inputs
correctly, so the buggy fast path was removed.
Per rluvaton's nit on PR apache#22689: the all-null fast path can return a typed
null scalar (`ScalarValue::try_new_null(return_type)`) and let the caller
broadcast it back out to the input row count, instead of materialising a
full `new_null_array` ourselves.
@Adam-Alani
Adam-Alani requested a review from rluvatonJune 9, 2026 13:15
@rluvaton

Copy link
Copy Markdown
Member

Can you please extract the breaking change parts to a different PR since it requires a special attention as oppose to adding UDF that does not affect people who don't use it

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.
@Adam-Alani
Adam-Alani marked this pull request as draft June 9, 2026 14:26
@github-actionsgithub-actionsBot removed physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jun 9, 2026
@Adam-Alani
Adam-Alani marked this pull request as ready for review June 9, 2026 14:31
@Adam-Alani

Copy link
Copy Markdown
Author

Can you please extract the breaking change parts to a different PR since it requires a special attention as oppose to adding UDF that does not affect people who don't use it

@rluvaton Let me know if this is okay, split it up into this PR and #22853

@github-actionsgithub-actionsBot removed the auto detected api change Auto detected API change label Jun 9, 2026
@gabotechs

Copy link
Copy Markdown
Contributor

I get the feeling that this UDF might be too specific to be contributed and maintained in this repo.

Before moving forward, is there any precedence you see in other engines for this function? typically this project aims to host functions present in engines like Postgres, or Spark.

@LiaCastaneda

Copy link
Copy Markdown
Contributor

transform_values does exists in spark, it does the same as array_transform but for maps - https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/api/pyspark.sql.functions.transform_values.html

@Adam-Alani

Copy link
Copy Markdown
Author

I get the feeling that this UDF might be too specific to be contributed and maintained in this repo.

Before moving forward, is there any precedence you see in other engines for this function? typically this project aims to host functions present in engines like Postgres, or Spark.

@gabotechs Yep as lia said, I followed the logic for adding array_transform, same function, but for maps

@gabotechs

gabotechs commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Ok, did some research, and it looks like in Spark and Trino there's these functions:

 - transform_values(map, function) — transforms each value, keeping keys unchanged. Function signature: (k, v) -> new_v.
- transform_keys(map, function) — transforms each key, keeping values unchanged. Function signature: (k, v) -> new_k.

Spark

Trino

So indeed this project might be a good place to host them.

}

#[test]
#[ignore = "blocked on apache/datafusion#22853: multi-param lambda projection fix"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we need to merge #22853 First?

Rather than shipping this with ignored tests, I'd recommend to get that PR in first so that we can un-ignore them here. Otherwise, we might be missing important coverage in main

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding off until #23660 is merged in that case

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix got merged, we can rebase #24162

alamb pushed a commit to alamb/datafusion that referenced this pull request Aug 11, 2026
…e#24162)
## Which issue does this PR close?
basically this PR apache#22853 + a
few more tests
## Rationale for this change
The current lambdas in DF 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`
(apache#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](apache#22853 (comment))
comment
LiaCastaneda added a commit to DataDog/datafusion that referenced this pull request Aug 12, 2026
…e#24162) (#166)
* fix(lambda): only push referenced params into the merged batch (apache#24162)
## Which issue does this PR close?
basically this PR apache#22853 + a
few more tests
## Rationale for this change
The current lambdas in DF 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`
(apache#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](apache#22853 (comment))
comment
(cherry picked from commit 4e6acfe)
* Adjust to API change
kosiew pushed a commit to kosiew/datafusion that referenced this pull request Aug 12, 2026
…e#24162)
## Which issue does this PR close?
basically this PR apache#22853 + a
few more tests
## Rationale for this change
The current lambdas in DF 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`
(apache#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](apache#22853 (comment))
comment
jayshrivastava pushed a commit to DataDog/datafusion that referenced this pull request Aug 13, 2026
…e#24162) (#166)
* fix(lambda): only push referenced params into the merged batch (apache#24162)
## Which issue does this PR close?
basically this PR apache#22853 + a
few more tests
## Rationale for this change
The current lambdas in DF 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`
(apache#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](apache#22853 (comment))
comment
(cherry picked from commit 4e6acfe)
* Adjust to API change
LiaCastaneda added a commit to DataDog/datafusion that referenced this pull request Aug 14, 2026
…Partition child (#169)
* Revert "fix(lambda): only push referenced params into the merged batch (apache#24162) (#166)"
This reverts commit 79de5e9.
* fix: keep a CoalescePartitionsExec required by a SinglePartition child (apache#23948)
- None filed; happy to open one if preferred.
A valid query can be planned into a physical plan that `SanityCheckPlan`
then rejects:
```
SanityCheckPlan
caused by
Error during planning: Plan: ["HashJoinExec: mode=CollectLeft, join_type=Left, on=[(id@0, id@0)], projection=[id@0]",
" DataSourceExec: file_groups={4 groups: [...]}, projection=[id], file_type=parquet",
" RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1",
" CoalescePartitionsExec",
" ProjectionExec: expr=[first_value(t.id) ORDER BY [...]@1 as id]",
" AggregateExec: mode=FinalPartitioned, gby=[id@0 as id], aggr=[first_value(t.id) ORDER BY [...]]",
" RepartitionExec: partitioning=Hash([id@0], 8), input_partitions=4",
" AggregateExec: mode=Partial, gby=[id@1 as id], aggr=[first_value(t.id) ORDER BY [...]]",
" DataSourceExec: file_groups={4 groups: [...]}, projection=[ts, id], file_type=parquet"]
does not satisfy distribution requirements: SinglePartition. Child-0 output partitioning: UnknownPartitioning(4)
```
The `HashJoinExec` is in `CollectLeft` mode, which requires
`Distribution::SinglePartition` on its build (left) child, but child 0
is a bare 4-partition `DataSourceExec` with no `CoalescePartitionsExec`
above it.
Self-contained reproducer with `datafusion-cli` (the four `COPY`
statements are what make the scan multi-partition):
```sql
set datafusion.execution.target_partitions = 8;
set datafusion.optimizer.repartition_file_scans = false;
create table src (id int, ts int) as values (1, 10), (2, 20), (3, 30);
copy (select * from src) to 'data/0.parquet' stored as parquet;
copy (select * from src) to 'data/1.parquet' stored as parquet;
copy (select * from src) to 'data/2.parquet' stored as parquet;
copy (select * from src) to 'data/3.parquet' stored as parquet;
create external table t stored as parquet location 'data/';
select a.id
from t a
left join (select distinct on (id) id, ts from t order by id, ts) f on a.id = f.id
order by a.id;
```
Setting `datafusion.optimizer.repartition_sorts = false` makes it plan
fine, which points at the sort-parallelization phase.
`EnsureRequirements` does insert the coalesce for the `SinglePartition`
requirement (`enforce_distribution.rs`, `Distribution::SinglePartition
=> add_merge_on_top(...)`). Its own phase 3a (`parallelize_sorts`) then
takes it back out: `remove_bottleneck_in_subplan` removes a
`CoalescePartitionsExec` found at `children[0]` positionally, without
consulting the parent's distribution requirement for that child.
That parent is reached because `update_coalesce_ctx_children` marks a
node as connected when *any* child qualifies. It correctly excludes a
`SinglePartition`-requiring child from *setting* the flag, but the
join's other child (`UnspecifiedDistribution`, connected to a coalesce
below) sets it, so the traversal descends into the join and rewrites
child 0 anyway. Nothing re-enforces distribution afterwards, so
`SanityCheckPlan` is the first thing to notice. Note the surviving
`CoalescePartitionsExec` on the probe side in the plan above: it is what
propagated the flag, and it is untouched because the `if` returns
without recursing into child 1.
The sibling helper on the phase 2b path already does consult the
requirement (`update_child_to_remove_unnecessary_sort` /
`remove_corresponding_sort_from_sub_plan` re-add a merge using the
per-child `child_distribution(child_idx)`); only this path is missing
it.
The same failure shows up with a build child that is already
hash-partitioned on the join key (`Child-0 output partitioning:
Hash([k@0], 8)`), which is what a `JoinSelection` input swap leaves
behind — a `CollectLeft` join reported as `join_type=Right` with an
embedded projection.
`remove_bottleneck_in_subplan` now checks the parent's per-child
distribution requirement before removing a coalesce, both for
`children[0]` and when recursing into the other children.
The node `parallelize_sorts` is itself rewriting (the root of the call)
is exempt, since the caller drops that node and rebuilds the sort
cascade around the result — that is the rule's intended transformation,
and gating it too would disable sort parallelization below a global
sort. This is threaded through as an `is_root` flag on a private `_impl`
function; the public entry point keeps its signature.
Yes, at two levels:
- An end-to-end sqllogictest in
`datafusion/sqllogictest/test_files/joins.slt` reproducing it from SQL
(the reproducer above, with the data written by `COPY` inside the test).
On `main` it fails with exactly the distribution error above.
- Two tests in
`datafusion/core/tests/physical_optimizer/ensure_requirements.rs`
covering both shapes of the build child (`UnknownPartitioning(n)` and
`Hash([k], n)`), running the full `EnsureRequirements` rule and then
`SanityCheckPlan` via the existing `optimize_and_sanity_check` helper,
plus the idempotency check.
`cargo test -p datafusion-physical-optimizer`, `cargo test -p datafusion
--test core_integration -- physical_optimizer` (530 tests) and the full
`sqllogictest` suite (498 files) pass.
No API changes. Plans that were previously rejected by `SanityCheckPlan`
now plan and execute; a coalesce that is genuinely required is retained
where it was previously (incorrectly) removed.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* fix(lambda): only push referenced params into the merged batch (apache#24162) (#166)
* fix(lambda): only push referenced params into the merged batch (apache#24162)
## Which issue does this PR close?
basically this PR apache#22853 + a
few more tests
## Rationale for this change
The current lambdas in DF 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`
(apache#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](apache#22853 (comment))
comment
(cherry picked from commit 4e6acfe)
* Adjust to API change
---------
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Lía Adriana <lia.castaneda@datadoghq.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationfunctionsChanges to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@Adam-Alani@rluvaton@gabotechs@LiaCastaneda@gstvg@miretskiy