Skip to content

expr-derive-impl: Canonicalize the sqlfunc macro behind one generator - #38929

Open
antiguru wants to merge 21 commits into
mainfrom
sqlfunc-canonicalization
Open

expr-derive-impl: Canonicalize the sqlfunc macro behind one generator#38929
antiguru wants to merge 21 commits into
mainfrom
sqlfunc-canonicalization

Conversation

@antiguru

@antiguru antiguru commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

The #[sqlfunc] macro had three independent generator arms totalling 762 lines. They built the same eleven optional override methods with 19 near-identical quote! blocks, enforced modifier legality with 12 separately written rejections, and each repeated emission of the struct, Display, FuncName, and the annotated function. Adding one capability meant writing it three times.

This collapses them into one generate parameterized by a Shape that answers the six questions where the arities genuinely differ. One of those is a naming divergence rather than a signature difference: EagerVariadicFunc has no output_sql_type, its core method is output_type, so Shape::output_method carries the name alongside the parameter list and the split is preserved rather than normalized.

Modifier legality becomes one table per arity. Adding a modifier to an arity is now a row in shape.rs.

No capability is added and no generated output changes. Stateful unary and binary functions still do not work, and unary still rejects a RowArena. Those arrive in the pull requests stacked on this one.

The crate is split into an acyclic module graph: shape.rs and signature.rs are leaves, modifiers.rs depends on shape, generate.rs on those three, and sqlfunc.rs is the entry point.

Evidence that output is unchanged. Every pre-existing snapshot is byte-identical, including the one asserting that unary functions reject a RowArena. cargo check -p mz-expr is not sufficient evidence on its own, because reordering methods inside an impl compiles identically and a dropped override silently falls back to a trait default. The existing suite exercised 6 of the 11 modifiers and never paired could_error with an emitted introduces_nulls, so three *_all_modifiers.snap files were added, one per arity, each setting every modifier that arity accepts. They were generated against the old generator first and confirmed byte-identical under the new one.

Two user-visible diagnostic changes. Passing is_infinity_monotone to a unary or variadic function is now an error. It was silently discarded before, both arms destructured it as is_infinity_monotone: _. All six uses in the tree are binary mul_* and div_*, so nothing in the tree is affected. Separately, legality validation now runs before signature extraction, so a malformed attribute reports the modifier error rather than Unsupported argument type.

Reference documentation for the macro lives in doc/developer/sqlfunc.md and in the module docs under src/expr-derive-impl/.

Supersedes #36697 and #36705, which are closed. Their conversions predate #37961 and hardcode Box<MirScalarExpr>, which is no longer correct now that LirScalarExpr also implements Eval. Two decisions from them are carried over: skip_display for state-dependent names, and giving EagerUnaryFunc::call the arena that binary and variadic already had.

Merged with the scalar function registry (#38868). That change threaded the raw attribute tokens into each of the three arity generators so it could record every function's declaration, types-only signature, and body fingerprint. With one generator there is one call site: generate invokes sqlfunc_source once and splices its members into the FuncName impl. The registry's token-rendering and type-probing helpers move to source.rs, a new leaf module alongside the others that only read syn types.

The merge gives two independent checks on the claim above. All twelve pre-existing macro snapshots merged unchanged, so the integration emits byte-identical code for every function the macro already covered. func_registry_snapshot passes, and it covers every scalar function rather than only the snapshot-covered ones.

🤖 Generated with Claude Code

antiguru and others added 19 commits September 17, 2026 17:08
@antiguru @claude
doc: Add design for sqlfunc macro canonicalization
aed87a0 
The `#[sqlfunc]` macro generates scalar function trait implementations for
535 functions, but 69 remain hand-written. Sixty of those carry struct
state, which the unary and binary generator arms reject because they index
arguments from position zero and always emit a unit struct.

The design unifies the three generator arms behind a shape descriptor
before adding the missing capabilities, so that `&self` support, arena
support, and `skip_display` are written once rather than once per arity.
It then converts 53 of the 69 functions, leaving the nine casts generic
over `E: Eval` and the seven short-circuiting variadics hand-written for
documented reasons.

Supersedes the approaches in #36697 and #36705, whose conversions predate
the MIR and LIR separation in #37961.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
doc: Address spec review of sqlfunc canonicalization design
e9da692 
Corrections from an adversarial review of the design, each verified against
the tree:

* `EagerBinaryFunc::call` and `EagerVariadicFunc::call` take a plain `&self`,
  not `&'a self`. The claim that giving the unary trait an `&'a self` receiver
  mirrors them was wrong. Only the arena parameter mirrors. Whether the
  receiver should change at all is now an open question, with the variance
  cost stated and plain `&self` as the starting point.
* `EagerVariadicFunc` has no `output_sql_type`. Its core method is named
  `output_type`. The shape descriptor carries the name alongside the
  signature and PR1 preserves the split.
* The `func_name!` block in `src/expr/src/scalar/func.rs` holds one entry per
  hand-written implementation and the macro emits the same `FuncName` impl,
  so 52 of the 53 conversions must delete a line from that shared file. The
  conversion PRs are not file-disjoint. Records the restack consequence.
* The verification discipline now covers every override method rather than
  nullability alone, since `preserves_uniqueness`, `is_monotone`, `inverse`,
  and `is_eliminable_cast` change plans without changing answers and so
  escape both sqllogictest and the goldens.
* `src/expr/src/scalar.rs:1413` documents non-strictness for `And`, `Or`, and
  `ErrorIfNull` only, not for the other four excluded variadics.
* Both predecessor PRs are open drafts, not closed. Closing them is now an
  explicit deliverable.
* Corrects the arm line count to 762, scopes the stateful count to the unary
  and binary subset, scopes the branch conflict figure to the branch it
  measures, adds the stale rustdoc in `src/expr-derive/src/lib.rs` to PR2,
  gives PR3 a decision rule if re-measurement disappoints, and removes an
  unresolvable exception clause from the success criteria.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
doc: Settle the conversion PR cut at one per source file
ea3738b 
One pull request per source file, applied without exception, giving 20
conversion pull requests for the 53 functions. No file is subdivided and no
two files are combined, including the twelve that hold a single function.

The alternative was grouping by function family, which would have reduced the
count to eight but mixes two cut rules, since some units would be a file and
others a family spanning several files. A single rule keeps each unit's
boundary predictable from its name.

Records the consequence for the shared `func_name!` block: all 20 pull
requests touch `src/expr/src/scalar/func.rs`, so each landing restacks
everything above it. The conflicts are mechanical, because every conversion
only deletes lines from that block and no two pull requests delete the same
line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
doc: Add implementation plan for sqlfunc canonicalization, part 1
470cec2 
Covers PR1 through PR4 of the 23 in the design: canonicalizing the three
generator arms, adding arena and stateful support plus `skip_display`, the
direct-unwrap `LazyBinaryFunc` change behind a measurement gate, and the first
conversion as the pattern-setter.

PR5 through PR23, the remaining 19 conversions, are deferred to part 2. Their
steps depend on what PR2's generated output actually looks like, and PR4 is what
establishes the recipe, so writing them now would be guesswork. Twenty
near-identical task bodies would also be the wrong artifact.

Records one cost the design glossed: giving `EagerUnaryFunc::call` a `RowArena`
forces a mechanical re-signing of all 40 hand-written unary `call` methods across
15 files, which the conversion PRs then delete.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Add per-arity shape and modifier tables
b44e992 
Declares the three scalar function arities and, for each, which modifiers it
accepts and the return type of the method each modifier generates. Nothing
consumes the tables yet.

The tables replace legality rules that are currently spread across 18
separately written rejections in the three generator arms.

Table order matches the `#*_fn` interpolation sequence emitted by each
generator arm today, since a later task drives emission from these tables in
order and must reproduce today's generated code byte-for-byte.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Validate modifiers against the shape tables
aacbfaf 
Replaces the per-modifier rejections across the three generator arms with
one loop over the modifiers that are present, checked against the arity's
table.

Passing `is_infinity_monotone` to a unary or variadic function is now an
error. Both arms previously destructured it as `is_infinity_monotone: _`
and discarded it without complaint. All six uses in the tree are on binary
mul_*/div_* functions, so nothing in the tree changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
doc: Correct the sqlfunc rejection count to 12
486c904 
The design and plan both said 18 separately written `unknown_field`
rejections. The tree has 18 `unknown_field` calls, but six of them are two
checks that are not about modifier legality, repeated once per arm: whether
`output_type` conflicts with `output_type_expr`, and whether
`output_type_expr` is missing its required `introduces_nulls`. Those three
checks survive the refactor, as does variadic's arity check.

Twelve rejections are modifier-legality rejections, four per arm, and those
are what the shape tables replace.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Emit override methods from one helper
6ac5426 
The three generator arms each built the same `fn name(&self) -> Ret { expr }`
shape, once per modifier, for 19 near-identical blocks. One helper now walks the
arity's table and emits them, taking the return type from the table so binary's
`is_monotone` keeps its `(bool, bool)`.

Emission follows table order rather than attribute order, so generated code does
not depend on how a call site spells its modifiers. The tables are ordered to
reproduce the previous output exactly.

Each arm still hand-builds `introduces_nulls`, since it can synthesize the body
from `output_type` when the modifier is absent. `insert_introduces_nulls` splices
it back into the position `override_methods` would have produced for it, so
clearing the modifier before calling `override_methods` does not change method
order in the generated code.

Also drops shape.rs's now-stale `allow(dead_code)` (every item is consumed by
production code), corrects a doc comment referencing a nonexistent `skip_display`
modifier, and fixes `Modifiers::iter`'s doc comment, which claimed to yield
modifiers in table order when no such single order exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Fix stale comments from the override-methods refactor
35e34da 
Three comments claimed `crate::generate` owns things it does not: it holds only
`override_methods` and `insert_introduces_nulls`, never touching `sqlname`,
`output_type`, `output_type_expr`, or `test`, which the three generator arms in
`crate::sqlfunc` still handle directly. Renamed the owner where these comments
named the wrong module.

Two comments in shape.rs referred to "a later task" driving emission from the
tables, which is now stale since this IS that task. Re-tensed them to describe
what `crate::generate::override_methods` does today.

Cut a four-line comment duplicated verbatim across all three generator arms down
to a pointer at `insert_introduces_nulls`'s own doc, which is the sole owner of
that reasoning. Dropped `Modifiers::iter`'s doc comment's caller enumeration,
since the contract (fixed order, matches no per-arity table) carries it without
naming who calls it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Add full-coverage snapshot tests for all three arities
fafb491 
cargo check -p mz-expr is order-blind: reordering methods inside an impl block
compiles identically, and a dropped override falls back to the trait default and
also compiles fine. The 535 macro expansions it exercises contribute no evidence
about ordering or completeness, so the 13 existing snapshots are the only actual
oracle, and they emit only 6 of the 11 modifiers between them. None pairs
could_error with a derived introduces_nulls, so insert_introduces_nulls's index-1
branch, the reason it computes an index instead of always inserting at 0, had no
regression coverage.

Adds one test per arity passing every modifier that arity's table in shape.rs
accepts, with could_error alongside output_type so introduces_nulls is derived
rather than given directly. Verified the new snapshots are byte-identical to
what the pre-override-methods generator (486c904) produces for the same
three tests, checked out standalone before this change existed, so these
snapshots capture existing behavior rather than pinning a new one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Emit struct, Display, and FuncName once
3c7d9c9 
The unit-struct definition, the `Display` impl, the `FuncName` impl, and
re-emitting the annotated function are identical across the three arities. They
now live in one function that wraps whatever trait impl an arity produced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Trim variadic_func's stale emission doc
045c6cf 
The two-mode struct/Display description was orphaned when that emission
moved into generate::emit. Point at emit instead of restating it, and
name FuncName, which the old comment never mentioned.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Drop the pointer from variadic_func's doc
0e5a081 
A one-line doc matching its unary and binary siblings carries no
duplicated fact. Pointing at generate::emit is defensible but
unnecessary when the reader is one call away from it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Drive all three arities from one generator
53dc2c5 
`unary_func`, `binary_func`, and `variadic_func` are now thin calls into one
`generate`, parameterized by a `Shape` that answers the questions where the
three traits genuinely differ: trait path, whether `call` takes an arena, the
output-type method's name, its parameter, and the nullability formula. The
signature analysis all three share stays in `sqlfunc`, whose type helpers become
`pub(crate)` so the single generator can reach them.

The output-type method's name is a real divergence rather than a wart to paper
over. `EagerVariadicFunc` declares no `output_sql_type`, its core method is
`output_type`, so `Shape::output_method` returns the name alongside the
parameter and emission uses whichever it gets.

`override_methods` now skips `introduces_nulls` unconditionally, because
`generate` synthesizes that method from the output type and places it with
`insert_introduces_nulls`. Enforcing it there replaces a convention each call
site had to remember, and removes the need for the by-value destructure that
`Modifiers` and `SqlName` carried `Clone` for. `Arity` keeps its `arena` flag
only on `Unary`, where the rejection reads it.

Generated output is unchanged. All 16 snapshots are byte identical, including
the one asserting that unary functions still reject a `RowArena`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
doc: Drop the line-count prediction and fix the snapshot count
494c36e 
Two claims the implementation falsified.

The design predicted the 762 lines of arm code would become roughly 300. The
three arms did collapse and `sqlfunc.rs` shrank by 687 lines, but the
descriptor, the tests pinning its table order, and the doc comments stating
the new contracts cost more than the copy-pasted arms did, so the crate's
total went up by 183. The win is that a capability is declared in one place
instead of three, which is what the section now says. Line count was the
wrong measure to promise.

The snapshot suite is 16, not 13. Three `*_all_modifiers` snapshots were
added during the refactor, because `cargo check -p mz-expr` is order-blind
and cannot catch either a reordered method or a dropped override, so the
existing 13 were the only guard and they exercised 6 of the 11 modifiers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Split the leaf modules out of sqlfunc
cf4b60e 
`sqlfunc` held three unrelated things: the attribute macro's entry point, the
parsed modifiers, and 600 lines of `syn` type analysis that referenced neither of
the other two. With the generator in a sibling module, reaching that analysis
meant exporting it, and `pub(crate)` then hid a mutual dependency between two
peers from the compiler.

The type analysis moves verbatim to `signature`, and `Modifiers`, `SqlName`, and
`reject_inapplicable` move to `modifiers`. Both are leaves, so the modules now
form a DAG. `shape` and `signature` depend on nothing, `modifiers` on `shape`,
`generate` on those three, and `sqlfunc` on all of them.

Ten modifier fields go back to private now that the direction is right. Only
`sqlname`, `output_type`, `output_type_expr`, and `introduces_nulls` are read by
name outside `modifiers`, and the rest are reached through `Modifiers::iter`.
`test` stays private behind a `generates_test` accessor. `camel_case`'s doc line,
which sat on `is_nullable_type`, is re-attached to the function it describes.

This is code motion. All 16 snapshots in this crate, and the six that `mz-expr`
generates, are byte identical.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
doc: Point at where sqlfunc modifier legality is declared
c1358d9 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
expr-derive-impl: Narrow visibility, merge impl blocks, add a TODO
396a289 
override_methods, insert_introduces_nulls, Emission, and emit lost their
only external callers when the three generator arms collapsed into
generate; narrow them from pub(crate) to private now that generate.rs is
the only caller. Merge the two impl Modifiers blocks (generates_test and
iter) into one. Note the inconsistent darling::Error constructor at the
two output_type/output_type_expr conflict checks, which still render as
"Unknown field: ..." while every other modifier legality error in the
crate uses Error::custom; the message text is pinned by a snapshot, so
this is left as a TODO rather than changed.

Also match the module doc-comment header style across all five modules:
drop the blank line between the license block and the //! doc that
shape.rs and generate.rs had and the other three modules did not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru @claude
doc: Fix the sqlfunc design doc against the shipped code
c5f3ffc 
The design described only shape.rs and generate; add a Module layout
subsection under Solution Proposal covering all five modules
(shape/signature as leaves, modifiers depending on shape, generate
depending on all three, sqlfunc as the entry point) and why the
resulting graph being acyclic matters. Rewrite the Shape sketch to match
the shipped impl Shape exactly: drop input_assoc and call_params, which
do not exist, and add label and takes_arena, which do.

Fix two false statements: the test modifier is consumed in sqlfunc.rs
through Modifiers::generates_test, not left explicit in generate; and
PR1's acceptance criterion is that no pre-existing snapshot file
changes, not that no snapshot file changes at all, since the branch
deliberately adds three new *_all_modifiers.snap files.

Document is_infinity_monotone in doc/developer/sqlfunc.md's modifier
list: a boolean, binary only, defaulting to true, describing whether
is_monotone's endpoint-sampling guarantee still holds when an operand
may be infinite.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
antiguru
antiguru commented Sep 18, 2026
View reviewed changes
Comment thread doc/developer/design/20260917_sqlfunc_canonicalization.md Outdated

@antiguru antiguru Sep 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Please drop the design files. They're useful for doing the changes, but don't have lasting value.

@antiguru antiguru Sep 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 9c63e3b. Both the design and the plan are dropped. Posted by Claude Code.

The reference material they carried is already in places a reader will look: doc/developer/sqlfunc.md documents the modifiers and the arity table, and the module docs under src/expr-derive-impl/ describe the layout and why the dependency graph is acyclic. The pull request body carries the rationale for this change.

Two knock-on notes.

The stacked branch above this one has been rebased onto the new head, so it does not resurrect the files. It is not pushed yet, since the work on it is unfinished.

The comments on the closed #36697 and #36705 still cite the design document's path, which no longer resolves. I have left them, since they are historical comments on closed pull requests, but say the word and I will edit the path out.

@antiguru @claude
doc: Drop the sqlfunc design and plan documents
9c63e3b 
They served the work that produced this stack and do not carry lasting
value. What the macro does now is documented where a reader will look for
it: `doc/developer/sqlfunc.md` for the modifiers and the arity table, and
the module docs in `src/expr-derive-impl/`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru
antiguru added this pull request to stack #38942 September 19, 2026 21:17
@antiguru antiguru mentioned this pull request Sep 19, 2026
Open
@antiguru @claude
Merge upstream/main into sqlfunc-canonicalization
d63f350 
Upstream's scalar function registry (#38868) threaded the raw attribute
tokens into each of the three arity generators so it could record every
function's declaration, signature, and body fingerprint. This branch replaced
those three generators with one, so the registry's emission moves with them:
`generate` calls `sqlfunc_source` once and splices its members into the
`FuncName` impl.

The registry's token-rendering and type-probing helpers move to a new leaf
module, `source.rs`, alongside the other modules that do nothing but read
`syn` types. Collapsing the three arity wrappers into the dispatch match was
cheaper than threading a fifth argument through three pass-throughs that
differed only in their `Shape`.

The twelve pre-existing macro snapshots merged unchanged, which says the
integration emits byte-identical code for every function the macro already
covered. `func_registry_snapshot` passes, which says the same for the scalar
functions the registry tracks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@antiguru