expr: Correct over-conservative properties of record_get and map_build_from_record_list

[antiguru]

Two functions declared properties more conservatively than their bodies need, inherited from hand-written impls and kept by their conversion to #[sqlfunc] in #39062. This drops the pins and lets the macro derive the properties from the output type.

• record_get returns a field of its input record and never produces an EvalError, but it declared could_error = true, the LazyUnaryFunc default. It now derives false from the non-Result output.
• map_build_from_record_list skips NULL records and keys and always returns a map, but it declared introduces_nulls and could_error true and a nullable output. Both flags now derive false from the DatumMap output, and the output type is non-nullable.

The registry reports exactly these moves. The current LIR_VERSION hasn't shipped, so the snapshots are regenerated in place. The sqllogictest files that mention record_get show no plan changes, and the sqllogictest shards passed on an earlier version of the stack that carried the map_build_from_record_list change.

Based on #39105.

🤖 Generated with Claude Code

[antiguru]

Claude, please file a linear issue for avoiding panics in function implementations to enforce invariants.

[antiguru]

Filed as CPU-308. It covers this unwrap and the unreachable! in output_type_expr, the other panic sites under func/impls/, and the arity assert from #39061.

Posted by Claude Code.

[antiguru]

This now also carries the map_build_from_record_list correction (e94c12d), which moved out of the map.rs conversion so that #39062 changes no declared properties. @ggevay, your approval predates it.

Posted by Claude Code.

[mgree]

LGTM. For posterity, https://linear.app/materializeinc/issue/CPU-308/avoid-panics-in-scalar-function-implementations-to-enforce-invariants documents the tension between panics and internal errors.

[def-]

QA LLM Review (Post Merge)

@antiguru — an automated review of commit 2170a3342c found the following potential MEDIUM+ issue(s) after this PR was merged.

1. HIGH -- Narrowing map_build's output to NOT NULL stalls existing Avro sinks on upgrade

src/expr/src/scalar/func/impls/map.rs:71

Changing map_build's declared output from .nullable(true) to .nullable(false) changes the persisted column type of every existing materialized view that calls map_build on a non-null list, for example the documented map_build(headers) pattern. On upgrade, Kafka sinks re-derive their Avro schema from that desc and publish a schema without the null branch. Confluent and Redpanda registries reject it under their default BACKWARD compatibility, so the sink stays stalled until someone intervenes by hand.

Details

At boot the MV desc is rebuilt from the plan (src/adapter/src/catalog/state.rs:1640), and the sink's Avro schema is regenerated from that desc (src/sql/src/plan/statement/ddl.rs:3341, src/interchange/src/json.rs:407). Persist accepts the narrowed desc because every Arrow column is nullable. That is why the break only shows up downstream, at the schema registry. Iceberg sinks follow the same path to the same failure: field_is_compatible requires required to match exactly (src/storage/src/sink/iceberg.rs:1000). The Iceberg case comes from reading the code. I did not run it.

Reproduced with plain docker, Redpanda's schema registry, and an in-place upgrade on a shared /mzdata:

CREATE TABLE t (k text NOT NULL, v text);
CREATE MATERIALIZED VIEW mv AS SELECT k, map_build(LIST[ROW('v', v)]) AS m FROM t;
CREATE SINK s FROM mv INTO KAFKA CONNECTION kc (TOPIC 'mzrv')
  FORMAT AVRO USING CONFLUENT SCHEMA REGISTRY CONNECTION csr ENVELOPE DEBEZIUM;

• On v26.46.0, m is nullable and the registered field is ["null", {"type":"map",...}].
• After upgrading to this commit's image, m is NOT NULL and mz_sink_statuses shows stalled: ... schema being registered is incompatible with an earlier schema. The sink retries every 5 s and keeps failing.
• Control: the same upgrade to the parent commit's image (expr: Return specific output types instead of Datum #39105) leaves m nullable and the sink running.

Fix: keep this change's could_error = false, but leave map_build's declared output type nullable until MV column nullability is stable across upgrades. The record_get hunk does not change any output type and can stay as is.

     }
-    .nullable(false)
+    .nullable(true),
+    // NOTE: the body never returns NULL, but narrowing this changes the desc of existing
+    // materialized views, and Avro and Iceberg sinks on them reject the regenerated schema.
+    introduces_nulls = true
 )]

[antiguru]

@def- Thanks, confirmed. #39843 restores the nullable map_build output and introduces_nulls = true, and keeps could_error = false and the record_get change.

Posted by Claude Code.