Repository navigation
Conversation
Signed-off-by: Matt Katz <[email protected]>
| if expr.as_higher_order().is_some() { | ||
| return vec![]; | ||
| } |
There was a problem hiding this comment.
higher_order is a scalar?
Merging this PR will regress 3 benchmarks4 warnings, 1 note
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
| /// binds the lambda that merges an element into the accumulator before the one that finishes. | ||
| fn bind_lambdas( | ||
| &self, | ||
| options: &Self::Options, | ||
| args: &[DType], | ||
| lambdas: &[Lambda], | ||
| ) -> VortexResult<Vec<BoundLambda>>; |
There was a problem hiding this comment.
does this live in the bind method?
| children: Arc<Vec<BoundExpression>>, | ||
| /// The lambdas as written, kept to display them and to rebind them when the children's | ||
| /// dtypes change. | ||
| lambdas: Arc<Vec<Lambda>>, |
There was a problem hiding this comment.
can get rid of this?
|
This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days |
Summary
An alternative design to #10502 and #10503, for comparison. It has the same features (variables, lambdas and
list_transform), but a higher-order function is its own kind of node in expressions and arrays, holding its lambdas next to its children, instead of a scalar function that keeps its lambda in its options.It's a draft to decide between the two designs, not meant to merge as is.
Changes
Variables and lambdas, as in Add lambdas and variables for higher-order scalar functions #10502:
var("x")buildsExpression::Variable, which displays as@x.Lambda { params, body }is the written form.Lambda::bindrewrites each@xto$.xand binds the body to a struct of the parameters, giving aBoundLambda.BoundLambda::evaluateruns it over parameter arrays.HigherOrderFnVTable, in a newhigher_order_fnmodule:bind_lambdas), in whatever order it needs;executereceives the bound lambdas).HigherOrderFnRefis its type-erased handle.Expression::HigherOrder { function, children, lambdas }andBoundExpression::HigherOrder { …, lambdas, bound }. The bound node keeps its written lambdas, so it displays@xand rebinds them when a rewrite changes its children's dtypes.HigherOrderArrayruns a bound node. It has slice and filter pushdown, so a lambda never runs on rows that aren't selected.list_transform(list, lambda)now lives inhigher_order_fn::fns:Listinput only, takes one parameter, and is strict;ListArray: null lists are emptied, thenlist_from_list_viewmakes the elements back to back;The new variants are handled in binding,
return_dtype,apply,apply_bound, display, optimize, traversal, validity (strict functions only) and serialization (an error for now).ScalarFnVTableis unchanged.Compared with #10502 and #10503
list_transformitself(x) -> ($.x + 1i32)(x) -> (@x + 1i32)Six places assumed that a node which isn't a scalar function is the root or has no children, and compiled anyway:
list_transformfailed.apply_boundreturned its input unchanged.$.applypanicked.All six are fixed here.
Not done
HigherOrderArray;an_infallible_transform_pushes_into_dictionary_valuesis ignored. The scan's own dictionary rule in vortex-layout doesn't know about the new node either.Testing
cargo nextest run -p vortex-array -p vortex-layout -p vortex-filecargo clippy -p vortex-array -p vortex-layout --all-targets --all-features -- -D warningscargo +nightly-2026-09-10 fmt -p vortex-array -p vortex-layout -p vortex-file --checkcargo test --doc -p vortex-array -- lambda exprs::var list_transform higher_orderRUSTDOCFLAGS="-D warnings --cfg docsrs" cargo doc --no-deps --document-private-items -p vortex-array -p vortex-layout --features vortex-array/_test-harnesslist_transform#10503 review, about 1,500 lines covering file scans and 6,000 randomized list cases. They all pass after the fixes above.