Conversation
antiguru
added this pull request to stack #38942
September 19, 2026 21:17
antiguru
changed the title
antiguru
marked this pull request as ready for review
September 19, 2026 21:18
`EagerBinaryFunc::call` and `EagerVariadicFunc::call` take an arena, unary did not. That gap blocks the compound-type casts that allocate their output, which have to stay hand-written `LazyUnaryFunc` impls to reach an arena at all. `EagerUnaryFunc::call` now takes `temp_storage: &'a RowArena`, and the blanket `LazyUnaryFunc` impl forwards the arena it already holds. `Shape::takes_arena` answers true for every arity, so `#[sqlfunc]` emits the parameter on unary `call` and forwards it when the annotated function asks for one. A unary function with a trailing `&RowArena` is therefore accepted rather than rejected. The hand-written unary impls take the parameter and ignore it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`#[sqlfunc(StructName, ..)]` on a function with a `&self` receiver now works for
unary and binary, as it already did for variadic. The macro attaches an inherent
method and the trait impl to the existing struct instead of defining a unit
struct.
Both arms previously indexed arguments from position zero, so the receiver reached
`arg_type` and produced `compile_error!("Unsupported argument type")`. Because the
arities share one generator, this is an offset and a call expression rather than
two copies of the variadic logic.
Also inlines `Shape::takes_arena`, which had become a constant-valued method with
a single call site now that every arity takes the arena, and drops the test that
looped over shapes asserting it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
generate's doc comment and the attribute-parsing comment in sqlfunc both described the struct-name argument as exclusive to variadic dispatch. Any arity accepts one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-four of the hand-written scalar functions format their name from struct
state, for example `extract_{unit}_ts`. Suppressing the generated `Display` lets
them keep the impl they already have, which is simpler than teaching `sqlname` to
evaluate an expression with `self` in scope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The insta_test_skip_display assert checked for "impl std :: fmt :: Display" with spaces around the double colons, the raw TokenStream::to_string() idiom. But test_sqlfunc formats output through prettyplease, which never spaces double colons, so the assert could never fire and only the snapshot enforced suppression. Match the unspaced form prettyplease actually produces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`RangeCreate` carries an `elem_type` field, so it needs the stateful shape the macro now accepts: the function takes `&self` and the macro emits the `call` body as a method on the existing struct. Its `Display` impl picks a name from `elem_type` rather than using a fixed string, so `skip_display` keeps the hand-written impl and the macro emits only `FuncName`. That removes the manual `func_name!` entry, which would otherwise collide with the generated impl. The output type is expressed with `output_type_expr` because it reads `elem_type` and ignores the input types, matching the hand-written body. `introduces_nulls = false` carries across the one nullability override; the remaining trait methods derive the same values from the signature that they derived from the associated types before, so the generated impl matches. Also corrects two stale claims in the macro documentation: unary functions now accept a `&RowArena` and a `&self` receiver, and `output_type_expr` applies to all three arities. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three documents describe which modifiers apply to which arity: the field docs on `Modifiers`, the macro's rustdoc, and `doc/developer/sqlfunc.md`. They had drifted from the tables in `shape.rs`, which are the actual authority. Corrects `output_type_expr` to all arities, `is_infix_op` to binary and variadic, `is_monotone` to all arities, and adds the three modifiers the rustdoc omitted entirely. Moves the struct name, `&self` receiver, and `&RowArena` sections out from under the variadic heading, since all three apply at every arity, and states that the generated struct and the generated `Display` are each conditional. Adds the `non_nullable_input_is_nullable` term to the null-handling formula. The formula omitted it, so it could not be used to reason about a function whose parameters do not all accept NULL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`sqlname` feeds only the generated `Display` impl, so pairing it with `skip_display` silently discards it. Rejecting the combination turns a modifier that does nothing into a compile error, and both modifier references now say so. Records the interlock behind unary's nullability tail. Unary omits the non-nullable-position term the other two arities carry, which is sound only because `propagates_nulls` is not a unary modifier, so unary's value always equals what that term would test. A test already pins that absence, but it did so without saying why, leaving a maintainer who deliberately adds the modifier free to delete the assertion. The assertion now names what depends on it. Corrects three claims the tree no longer supports. `is_infix_op` has a default on every `Eager*Func` trait, so it does not need to be specified. The nine `LazyUnaryFunc` implementations that stay hand-written are the ones generic over `Eval`, not every compound-type cast, and the ten others are convertible now that `EagerUnaryFunc::call` supplies a `RowArena`. `ListLengthMax` gave its `max_layer` field as the reason it could not use the macro, which a `&self` receiver now handles. Replaces the struct-name example, which showed a two-parameter signature that detects as binary under a name saying variadic, with the real `array_fill` signature. Normalizes the test module on `super::` for its own helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
antiguru
force-pushed
the
sqlfunc-stateful-unary-binary
branch
from
September 19, 2026 21:41
9ef1418 to
abbd17d
Compare
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 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.
EagerUnaryFunc::callnow receives a&RowArena, and the#[sqlfunc]macro accepts a&selfreceiver and a trailing&RowArenaat unary and binary arity rather than only at variadic. Together these unblock the ten hand-writtenLazyUnaryFuncimplementations undersrc/expr/src/scalar/func/impls/that exist in that form only because they need an arena to allocate their output.A
skip_displaymodifier suppresses the generatedDisplayimpl, for structs whose displayed name depends on their own state.RangeCreateis the first function converted on top of both capabilities. It carries anelem_typefield, so it needs the stateful shape, and it picks its name from that field, so it keeps a hand-writtenDisplay.The documentation commits correct the macro reference. Three surfaces describe which modifiers apply to which arity, and all three had drifted from the tables in
shape.rsthat actually decide it.Evidence the conversion preserves semantics. The scalar function registry from #38868 reports exactly one change across the whole catalog:
range_create'ssqlfunc_signaturegoes from null to the macro's signature. Every declared property is byte-identical, includingpropagates_nulls,introduces_nulls,could_error,input_types, andoutput_type.test/sqllogictest/range.sltpasses 1534 of 1534.One question for the reviewer. Regenerating
func_registry.jsonchanges version 1's digest infunc_registry_digests.json. The test's guidance is to bumpLIR_VERSIONif version 1 has shipped and to regenerate in place if it has not.plan.rsties the bump to a scalar function's declared properties changing, and none did, so this regenerates in place without bumping. But #38868 is contained inv26.43.0-rc.1, so if that counts as shipped the version should be bumped instead.Based on #38929.
🤖 Generated with Claude Code