Uh oh!
There was an error while loading. Please reload this page.
Add list_transform expression - #8663
Closed
mhk197 wants to merge 2 commits into
Closed
Conversation
Adds a ListTransform scalar fn implementing DuckDB-style list_transform(l, lambda x: ...) as a Vortex expression. The lambda body is an ordinary Expression stored in the fn's options, evaluated with the list's elements array as its root scope (root() inside the body refers to the element). Execution rewraps the list structure around a deferred apply() of the body to the elements child, so offsets and list validity pass through untouched and no element values are computed eagerly. Includes: - identity (body == root) and fusion (transform-of-transform) rewrites in simplify_untyped, plus list_length(list_transform(l, f)) -> list_length(l) - proto serde of the options-embedded body via the session registry - fallibility delegation to the body, since generic tree walks cannot see options-embedded expressions - design doc under scalar_fn/fns/list_transform/design.md Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Correctness: - A fallible body is no longer evaluated over element positions that no visible list references. ListView inputs (gaps, overlaps) and Lists with null rows (whose ranges Arrow permits to hold arbitrary values) now gather exactly the referenced elements into a compact List before applying the body. Infallible bodies keep the zero-cost structure-preserving path. - Zero-row batches short-circuit before the constant fast path, which could eagerly evaluate a fallible body over elements no row observes. - Fusion now collapses a whole transform chain in one rewrite (deep chains previously exhausted the optimizer's per-node iteration budget and errored on valid expressions) and skips bodies with more than one root reference (substitution previously duplicated the inner body per occurrence, compounding exponentially across fusion steps). - The identity rewrite moved to the typed simplify hook, gated on the input actually being list-typed, so ill-typed expressions still fail return_dtype instead of being rewritten into well-typed non-list ones. - serialize() now honors the vtable contract by returning Ok(None) when the body contains a non-serializable fn, via a new Expression::try_serialize_proto helper. Also: - The typed simplify hook optimizes the body against its element scope, since the options-embedded body is invisible to the optimizer's children traversal. - is_fallible uses a short-circuiting walk instead of a full label_tree labeling per call. - fmt_sql reuses the options Display impl so the lambda rendering lives in one place. - Regression tests for all of the above, including one pinning that the null-scalar branch protects fallible bodies from a null FixedSizeList constant's default-filled placeholder elements. - Test hygiene: imports at the top of the tests module, vortex_bail instead of panic, removed an assertion subsumed by assert_arrays_eq. Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Merging this PR will improve performance by 12.85%
|
| Mode | Benchmark | BASE | HEAD | Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | chunked_varbinview_canonical_into[(1000, 10)] | 154.7 µs | 190.2 µs | -18.68% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[128] | 273.6 ns | 215.3 ns | +27.1% |
| ⚡ | Simulation | chunked_varbinview_opt_canonical_into[(1000, 10)] | 206.2 µs | 168.8 µs | +22.11% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[1024] | 333.9 ns | 275.6 ns | +21.17% |
| ⚡ | Simulation | chunked_varbinview_opt_into_canonical[(1000, 10)] | 219.9 µs | 182.8 µs | +20.24% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[2048] | 427.8 ns | 369.4 ns | +15.79% |
| ⚡ | Simulation | encode_varbin[(1000, 8)] | 161.2 µs | 142.2 µs | +13.39% |
| ⚡ | Simulation | encode_varbin[(1000, 4)] | 159.4 µs | 141.3 µs | +12.82% |
| ⚡ | Simulation | encode_varbin[(1000, 32)] | 164.9 µs | 147.3 µs | +11.92% |
| ⚡ | Simulation | chunked_varbinview_opt_canonical_into[(100, 100)] | 341 µs | 305.1 µs | +11.79% |
| ⚡ | Simulation | encode_varbin[(1000, 2)] | 156.3 µs | 140.8 µs | +10.98% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mk/list-transform (c915742) with develop (a6e3d3e)
Footnotes
42 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Contributor
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 |
Contributor
This PR was closed because it has been inactive for 7 days since being marked as stale. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
list_transform— DuckDB'slist_transform(l, lambda x: x + 1)— as a Vortex expression, the first higher-order list function. The design doc (research into DuckDB/ClickHouse/Spark/Velox/Polars/DataFusion lambda implementations plus the Vortex design rationale) is included atvortex-array/src/scalar_fn/fns/list_transform/design.md.Design
Two decisions carry the feature (see design.md §4 for the trade-off tables):
Expressionstored in the fn'sOptions, not a child. Children are evaluated eagerly at row cardinality, but the body must run at element cardinality against a scope that only exists insideexecute. Options already support this (ExpressionisEq + Hash, and options serde has the session hook needed to deserialize a nested expression). This is DuckDB's bind-data approach; scope-independent properties (fallibility, serialization, body optimization) delegate explicitly.root()is the lambda parameter. Within the body,root()refers to the element — no variable nodes or name resolution.x -> x.a + 1ischecked_add(get_item("a", root()), lit(1)). Nested transforms rebind root naturally; captures and the(x, i)index form have a forward path via a struct scope (design.md §8) with no v1 changes.Semantics & execution
apply(body)— dictionary-encoded elements evaluate the body over dictionary values only.Rewrites
list_transform(l, x -> x)→l(typed, gated on the input being list-typed so type errors still surface).list_transform(list_transform(l, f), g)→list_transform(l, g[root := f]), collapsing whole chains in one rewrite, skipped for bodies with >1 root reference (substitution would duplicatefper occurrence).list_length(list_transform(l, f))→list_length(l)— the composition reads offsets only.simplifyhook (it's invisible to the optimizer's children traversal by design).The second commit addresses findings from an 8-angle adversarial review (each finding independently verified with repro tests): the fallible-body/unreferenced-elements gather path, a zero-row constant guard, the fusion chain/occurrence guards, type-error preservation, and assorted cleanups.