expr: Give the binary lazy blanket impl a concrete iterator type by antiguru · Pull Request #39061 · MaterializeInc/materialize

antiguru · 2026-09-23T12:56:46Z

The blanket LazyBinaryFunc impl for EagerBinaryFunc fed InputDatumType::try_from_iter an iterator built from a closure, which is a new type for every function and every Eval implementor. Evaluating both arguments into an array gives every call site one iterator type, so argument unwrapping is instantiated once per input type. Hand-written EagerBinaryFunc impls get this too.

cargo llvm-lines -p mz-expr drops from 1,786,549 to 1,703,578 lines, 4.6% fewer. Results don't change for two arguments, the only count planning produces, and the macro snapshots are untouched.

Based on #38941.

🤖 Generated with Claude Code

mgree

Neat!

Claude had some thoughts around the assertions and checks.

Details

Strong suggestion

Keep assert_none!(datums.next(), ...) instead of assert_eq!(exprs.len(), 2) (binary.rs:176-180). The new assert is weaker, and the four-line comment defending it relies on a rule nothing enforces. The macro guarantees one datum per component, but a hand-written EagerBinaryFunc doesn't have to. For example, one with Input = (A,) would silently drop the second argument, and the old check would have caught that. The iterator's type is now always array::IntoIter<_, 2>, so keeping the direct check adds no per-function code. With it, the comment and the invariant it has to justify both go away.

Nits

Arity is checked in two places. Too few arguments is caught by the let-else before evaluation, and too many by an assert after evaluation. If exprs.len() == 2 stays, put it next to the let-else, or match let [a, b] = exprs else { ... }, so all arity handling happens before any eval. (Matching [a, b] would turn three arguments from a panic into an Internal error. Either is fine since the state can't be reached.)

antiguru · 2026-10-14T14:47:26Z

+        let [expr_a, expr_b, ..] = exprs else {
+            return Err(EvalError::Internal("unexpectedly missing parameter".into()));
+        };


Can we turn this into an exact match, and return an error if the arity doesn't match, instead of the assert further down.

Done in 83b4099. let [expr_a, expr_b] = exprs now matches exactly and returns an internal error for any other count before evaluating. The post-evaluation assert is replaced by a datums.next() check that returns an internal error, which also catches an Input that leaves a datum unread, the case from the earlier review.

Posted by Claude Code.

mgree

LGTM!

mgree · 2026-10-14T16:26:32Z

-            .map(|expr| expr.eval(datums, temp_storage));
+        let [expr_a, expr_b] = exprs else {
+            return Err(EvalError::Internal(
+                format!("binary function called with {} arguments", exprs.len()).into(),


`InputDatumType::try_from_iter` is generic over the iterator it reads from, and the blanket impl passed it a `map` closure over `exprs`. That closure is a fresh anonymous type for every function and every `Eval` implementor, so the tuple's unwrapping was instantiated once per combination rather than once per input type. Evaluating the two arguments into an array gives every call site the same iterator type, `core::array::iter::IntoIter<Result<Datum, EvalError>, 2>`, so the instantiations are keyed by the `Input` tuple and the types its unwrapping recurses into. Doing it in the blanket impl rather than at each call site means hand-written `EagerBinaryFunc` implementations get it too. Measured with `cargo llvm-lines -p mz-expr` on this tree, not reusing the figures from the superseded #36705. The `try_from_iter` instantiations reaching binary functions drop from 108,569 lines across 1,118 entries to 33,539 across 164. The crate total drops from 1,786,549 lines to 1,703,578, 4.6% fewer. Evaluation is unchanged for two arguments, which is the only count planning produces: both were already evaluated before any failure was inspected, so the array changes when they are evaluated relative to each other, not how often. All nine crossings of valid, NULL, evaluation error and wrong type return what they did before. Unreachable states change from panics to internal errors. An argument count other than two returns an internal error before evaluating any argument, where three or more arguments used to panic after evaluating the third. A single argument that is a non-NULL datum of the wrong type now reports this arity error rather than `invalid input type`. An `Input` that leaves one of the two datums unread returns an internal error, where it used to panic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ro (#39062) Converts the remaining hand-written scalar functions to `#[sqlfunc]`, one commit per source file or function: the `Cast*ToNumeric` casts (`int16`, `int32`, `int64`, `uint16`, `uint32`, `uint64`, `float32`, `float64`, `jsonb`), `PadChar`, `AdjustNumericScale`, `CastArrayToString`, `CastRangeToString`, and all of `date.rs`, `time.rs`, `map.rs`, `record.rs`, `list.rs`, `timestamp.rs` and `string.rs`. Each commit message accounts for every method the deleted impl overrode. No declared property changes. The registry reports only the macro provenance fields moving. Where a hand-written impl declared a property more conservatively than the body needs, the conversion pins the declared value: `could_error` on `record_get`, and `introduces_nulls`, `could_error` and a nullable output on `map_build_from_record_list`. #39814 corrects both. Supersedes #39079 to #39097, which held one conversion each. Based on #39061. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

…9903) #39061 made the blanket `LazyBinaryFunc` impl evaluate both arguments into an array of results. That made `case_literal/if_chain/*` 11-13% slower, which nightly cargo-bench flags. A named iterator that evaluates each argument on demand restores most of the previous speed and keeps #39061's code-size reduction (`cargo llvm-lines -p mz-expr --lib`: 1,933,557 lines vs. 1,934,453 on main). Local A/B against ae3cc46, #39061's parent (median of 3, pinned core): `if_chain/64_arms/last` 4741 ns before #39061, 5319 ns on main, 4816 ns with this PR. Most of the family lands within 2.3% of the parent. `4_arms/last` is the exception (306 / 340 / 333 ns), and it reproduced in two runs. Every binary function's input is a 2-tuple whose `try_from_iter` pulls both arguments before it checks for errors and NULLs, so evaluation semantics are unchanged. A new case in `error_semantics.slt` pins that an eval error wins over a NULL in either argument position. Closes: [CPU-315](https://linear.app/materializeinc/issue/CPU-315) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

antiguru requested a review from a team as a code owner September 23, 2026 12:56

antiguru added this pull request to stack #38942 September 23, 2026 13:12

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from bf4a86a to dc73d40 Compare September 23, 2026 14:23

antiguru changed the title ~~expr: Emit LazyBinaryFunc directly for generated binary structs~~ expr: Give the binary lazy blanket impl a concrete iterator type Sep 23, 2026

antiguru mentioned this pull request Sep 23, 2026

expr: Convert the remaining hand-written functions to the sqlfunc macro #39062

Merged

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from dc73d40 to f96d625 Compare September 23, 2026 14:33

antiguru requested a review from a team as a code owner September 23, 2026 17:54

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from f96d625 to 1405739 Compare September 23, 2026 17:54

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from 1405739 to 6388d87 Compare September 23, 2026 18:09

mgree approved these changes Sep 23, 2026

View reviewed changes

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from 6388d87 to 99836fa Compare September 24, 2026 10:14

antiguru requested a review from petrosagg September 24, 2026 11:17

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from 99836fa to 4d9c433 Compare September 24, 2026 14:29

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from 4d9c433 to 88841f6 Compare September 25, 2026 16:00

antiguru removed the request for review from petrosagg October 13, 2026 15:56

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from 88841f6 to f79eb4a Compare October 14, 2026 09:11

antiguru requested a review from mgree October 14, 2026 10:21

antiguru commented Oct 14, 2026

View reviewed changes

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from f79eb4a to e136d2c Compare October 14, 2026 15:18

antiguru mentioned this pull request Oct 14, 2026

expr: Correct over-conservative properties of record_get and map_build_from_record_list #39814

Merged

antiguru removed this pull request from stack #38942 October 14, 2026 15:45

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from e136d2c to 83b4099 Compare October 14, 2026 15:46

antiguru added this pull request to stack #39823 October 14, 2026 15:46

mgree approved these changes Oct 14, 2026

View reviewed changes

antiguru force-pushed the sqlfunc-binary-direct-unwrap branch from 83b4099 to 413f441 Compare October 14, 2026 19:12

antiguru merged commit 882dcbc into main Oct 14, 2026
83 checks passed

antiguru deleted the sqlfunc-binary-direct-unwrap branch October 14, 2026 19:50

antiguru mentioned this pull request Oct 16, 2026

expr: Evaluate binary function arguments through a named iterator #39903

Merged

Navigation Menu

Repository navigation

expr: Give the binary lazy blanket impl a concrete iterator type - #39061

expr: Give the binary lazy blanket impl a concrete iterator type#39061
antiguru merged 1 commit into
sqlfunc-stateful-unary-binaryfrom
sqlfunc-binary-direct-unwrap

antiguru commented Sep 23, 2026 •

edited

Loading

Uh oh!

mgree left a comment

Uh oh!

antiguru Oct 14, 2026

Uh oh!

antiguru Oct 14, 2026

Uh oh!

mgree left a comment

Uh oh!

mgree Oct 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

antiguru commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgree left a comment

Choose a reason for hiding this comment

Strong suggestion

Nits

Uh oh!

antiguru Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

antiguru Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

mgree left a comment

Choose a reason for hiding this comment

Uh oh!

mgree Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

antiguru commented Sep 23, 2026 •

edited

Loading