expr: refactor VariadicFunc to trait-based dispatch with typed inputs #34985
Conversation
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
Convert VariadicFunc from a monolithic enum with hand-written match arms to the same trait-based pattern used by UnaryFunc and BinaryFunc. Each of the 42 variants becomes its own struct implementing either LazyVariadicFunc (for lazy-evaluated functions like And/Or) or EagerVariadicFunc (for eager functions), with a derive_variadic! macro generating the dispatch enum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use the modern Rust module layout convention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change switch_and_or, unit_of_and_or, and zero_of_and_or to return Option instead of panicking on non-And/Or variants. Refactor call sites to use the Option directly, removing redundant And/Or match guards. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce a `VariadicInput` trait that converts a datum slice into typed function inputs, replacing the untyped `&[Datum<'a>]` parameter on `EagerVariadicFunc::call`. This follows the same pattern as `EagerBinaryFunc` with `Input1`/`Input2`/`Output` associated types. Key design points: - Two lifetimes `<'a: 'b, 'b>` where `'a` is datum content and `'b` is the slice borrow, allowing `type Input = &'b [Datum<'a>]` without requiring the slice to live for `'a`. - `try_from_datums(&mut &'b [Datum<'a>])` peels consumed elements off the front, enabling composable parsing. - Blanket `impl<T: DatumType> VariadicInput for T` bridges single elements; `OptionalArg<T>` and `&'b [Datum<'a>]` have direct impls. Tuple impls delegate to each element's `VariadicInput`. - `impl_lazy_from_eager!` macro replaces the blanket `LazyVariadicFunc` impl since Rust HRTB cannot express `for<'a: 'b, 'b>`. - Default `propagates_nulls`/`introduces_nulls`/`could_error` are derived from `Input`/`Output` types, removing boilerplate from most function impls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pre-merge checklist
|
Change VariadicInput::try_from_datums to take an iterator of Result<Datum<'a>, E> instead of a slice reference. This avoids allocating a Vec for fixed-arity variadic functions by lazily evaluating expressions from the iterator. Add Present<T> wrapper for DatumType elements in tuple inputs to avoid coherence issues with blanket impls. Update all 30 eager function implementations to use Present<T> wrapping and Vec<Datum> for truly variadic inputs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
Replace `Result<Datum<'a>, EvalError>` with concrete types where
possible so that `introduces_nulls`, `could_error`, and `output_type`
are correctly derived from the type system rather than needing manual
overrides.
Functions updated:
- String returns (&'a str): Concat, ConcatWs, PadLeading, Substr,
Replace, Translate, SplitPart, RegexpReplace
- Bytes returns (&'a [u8]): HmacString, HmacBytes, hmac_inner
- Timestamp returns: DateBinTimestamp, DateBinTimestampTz
- Integer returns (i64): DateDiff{Timestamp,TimestampTz,Date,Time}
- Time returns (NaiveTime): TimezoneTime
- ACL returns: MakeAclItem (AclItem), MakeMzAclItem (MzAclItem)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Generalize the Vec VariadicInput impl from Vec<Datum<'a>> to Vec<T: VariadicInput>, enabling structured element types like Present<Option<&str>> and (Present<K>, Present<V>) tuples. Key changes: - Concat/ConcatWs: Vec<Present<Option<&str>>> with owned String output - JsonbBuild*: Vec<Present<Datum>> / Vec<(Present<Datum>, Present<Datum>)> - MapBuild: Vec<(Present<Option<&str>>, Present<Datum>)> - ArrayIndex: (Present<Array>, Vec<Present<i64>>) with inlined impl - ListIndex: (Present<DatumList>, Vec<Present<i64>>) - ArrayToString: Present<Array> input with owned String output - MakeTimestamp: Result<Option<CheckedTimestamp<...>>, EvalError> output - Replace/Translate: owned String output (no temp_storage needed) - Add Deref impls and derive macros for Present<T> and OptionalArg<T> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The prior commit removing is_infix_op was too aggressive — And/Or need infix display formatting for EXPLAIN output (e.g., `(a AND b AND c)` instead of `AND(a, b, c)`). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free
to 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.
Summary
VariadicFuncenum to a trait-based dispatch pattern withEagerVariadicFunc/LazyVariadicFuncand aderive_variadic!macro, mirroring the existingBinaryFuncarchitecture.VariadicInputtrait with typedInput/Outputassociated types onEagerVariadicFunc, replacing untyped&[Datum]parameters. Uses composable datum-peeling (&mut &[Datum]) with blanketDatumType→VariadicInputbridge,OptionalArg, and tuple impls.propagates_nulls/introduces_nulls/could_errorderived fromInput/Outputtypes, removing boilerplate from most function impls.Test plan
cargo check -p mz-expr— core crate compilescargo check -p mz-sql -p mz-transform -p mz-expr-parser— consumers compilecargo test -p mz-expr— all 532 tests pass🤖 Generated with Claude Code