Address comments

Signed-off-by: Moritz Hoffmann <mh@materialize.com>

Author: antiguru

Cargo.toml

@@ -273,6 +273,10 @@ differential-dataflow = { opt-level = 3 }
 mz-compute = { opt-level = 3 }
 mz-transform = { opt-level = 3 }
 
+# Recommended by insta
+insta = { opt-level = 3 }
+similar = { opt-level = 3 }
+
 [profile.release]
 # Compile time seems similar to "lto = false", runtime ~10% faster
 lto = "thin"

WORKSPACE

@@ -558,6 +558,8 @@ crates_repository(
         "rustc-demangle": [crate.annotation(rustc_flags = ["-Copt-level=3"])],
         "timely": [crate.annotation(rustc_flags = ["-Copt-level=3"])],
         "differential-dataflow": [crate.annotation(rustc_flags = ["-Copt-level=3"])],
+        "insta": [crate.annotation(rustc_flags = ["-Copt-level=3"])],
+        "similar": [crate.annotation(rustc_flags = ["-Copt-level=3"])],
     },
     cargo_config = "//:.cargo/config.toml",
     cargo_lockfile = "//:Cargo.lock",

src/expr-derive-impl/Cargo.toml

@@ -23,6 +23,9 @@ insta = { version = "1.42", features = ["serde"] }
 mz-ore = { path = "../ore", features = ["test"] }
 prettyplease = "0.2"
 
+[package.metadata.cargo-gazelle.lib]
+rustc_flags = ["-Copt-level=3"]
+
 [package.metadata.cargo-udeps.ignore]
 normal = ["workspace-hack"]
 

src/expr-derive-impl/src/lib.rs

@@ -8,6 +8,10 @@
 // by the Apache License, Version 2.0.
 
 //! Implementations of proc macros to derive SQL function traits.
+//!
+//! This is separate from the actual proc macro to allow exporting the
+//! function defining the proc macro itself. Proc macro crates cannot
+//! export anything but proc macros.
 
 mod sqlfunc;
 

src/expr-derive-impl/src/snapshots/mz_expr_derive_impl__test__add_int16.snap

@@ -47,7 +47,7 @@ impl<'a> crate::func::binary::EagerBinaryFunc<'a> for AddInt16 {
             )
     }
     fn introduces_nulls(&self) -> bool {
-        <i16 as mz_repr::DatumType<'_, ()>>::nullable()
+        <i16 as ::mz_repr::DatumType<'_, ()>>::nullable()
     }
     fn is_infix_op(&self) -> bool {
         true

src/expr-derive-impl/src/snapshots/mz_expr_derive_impl__test__complex_type.snap

@@ -47,7 +47,7 @@ impl<'a> crate::func::binary::EagerBinaryFunc<'a> for ComplexOutputTypeFn {
             )
     }
     fn introduces_nulls(&self) -> bool {
-        <Option<bool> as mz_repr::DatumType<'_, ()>>::nullable()
+        <Option<bool> as ::mz_repr::DatumType<'_, ()>>::nullable()
     }
     fn is_infix_op(&self) -> bool {
         true

src/expr-derive-impl/src/snapshots/mz_expr_derive_impl__test__unary_arena_fn.snap

@@ -2,39 +2,6 @@
 source: src/expr-derive-impl/src/lib.rs
 expression: "#[sqlfunc()]\nfn unary_fn<'a>(a: Datum<'a>, temp_storage: &RowArena) -> bool {\n    unimplemented!()\n}\n"
 ---
-#[derive(
-    proptest_derive::Arbitrary,
-    Ord,
-    PartialOrd,
-    Clone,
-    Debug,
-    Eq,
-    PartialEq,
-    serde::Serialize,
-    serde::Deserialize,
-    Hash,
-    mz_lowertest::MzReflect
-)]
-pub struct UnaryFn;
-impl<'a> crate::func::EagerUnaryFunc<'a> for UnaryFn {
-    type Input = Datum<'a>;
-    type Output = bool;
-    fn call(&self, a: Self::Input) -> Self::Output {
-        unary_fn(a, temp_storage)
-    }
-    fn output_type(&self, input_type: mz_repr::ColumnType) -> mz_repr::ColumnType {
-        use mz_repr::AsColumnType;
-        let output = Self::Output::as_column_type();
-        let propagates_nulls = crate::func::EagerUnaryFunc::propagates_nulls(self);
-        let nullable = output.nullable;
-        output.nullable(nullable || (propagates_nulls && input_type.nullable))
-    }
-}
-impl std::fmt::Display for UnaryFn {
-    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-        f.write_str(stringify!(unary_fn))
-    }
-}
-fn unary_fn<'a>(a: Datum<'a>, temp_storage: &RowArena) -> bool {
-    unimplemented!()
+::core::compile_error! {
+    "Unary functions do not yet support RowArena."
 }

src/expr-derive-impl/src/sqlfunc.rs

@@ -16,7 +16,7 @@ use syn::{Expr, Lifetime};
 /// Modifiers passed as key-value pairs to the `#[sqlfunc]` macro.
 #[derive(Debug, Default, darling::FromMeta)]
 pub(crate) struct Modifiers {
-    /// An optional expression that evaluates to a boolean indicating whether the function i
+    /// An optional expression that evaluates to a boolean indicating whether the function is
     /// monotone with respect to its arguments. Defined for unary and binary functions.
     is_monotone: Option<Expr>,
     /// The SQL name for the function. Applies to all functions.
@@ -37,6 +37,8 @@ pub(crate) struct Modifiers {
     could_error: Option<Expr>,
     /// Whether the function propagates nulls. Applies to binary functions.
     propagates_nulls: Option<Expr>,
+    /// Whether the function introduces nulls. Applies to all functions.
+    introduces_nulls: Option<Expr>,
 }
 
 /// Implementation for the `#[sqlfunc]` macro. The first parameter is the attribute
@@ -54,7 +56,10 @@ pub fn sqlfunc(
     let func = syn::parse2::<syn::ItemFn>(item.clone())?;
 
     let tokens = match determine_parameters_arena(&func) {
-        (1, arena) => unary_func(&func, modifiers, arena),
+        (1, false) => unary_func(&func, modifiers),
+        (1, true) => Err(darling::Error::custom(
+            "Unary functions do not yet support RowArena.",
+        )),
         (2, arena) => binary_func(&func, modifiers, arena),
         (other, _) => Err(darling::Error::custom(format!(
             "Unsupported function: {} parameters",
@@ -171,11 +176,7 @@ fn output_type(arg: &syn::ItemFn) -> Result<&syn::Type, syn::Error> {
 }
 
 /// Produce a `EagerUnaryFunc` implementation.
-fn unary_func(
-    func: &syn::ItemFn,
-    modifiers: Modifiers,
-    arena: bool,
-) -> darling::Result<TokenStream> {
+fn unary_func(func: &syn::ItemFn, modifiers: Modifiers) -> darling::Result<TokenStream> {
     let fn_name = &func.sig.ident;
     let struct_name = camel_case(&func.sig.ident);
     let input_ty = arg_type(func, 0)?;
@@ -190,16 +191,23 @@ fn unary_func(
         negate,
         could_error,
         propagates_nulls,
+        introduces_nulls,
     } = modifiers;
 
     if is_infix_op.is_some() {
-        return Err(darling::Error::unknown_field("is_infix_op"));
+        return Err(darling::Error::unknown_field(
+            "is_infix_op not supported for unary functions",
+        ));
     }
     if negate.is_some() {
-        return Err(darling::Error::unknown_field("negate"));
+        return Err(darling::Error::unknown_field(
+            "negate not supported for unary functions",
+        ));
     }
     if propagates_nulls.is_some() {
-        return Err(darling::Error::unknown_field("propagates_nulls"));
+        return Err(darling::Error::unknown_field(
+            "propagates_nulls not supported for unary functions",
+        ));
     }
 
     let preserves_uniqueness_fn = preserves_uniqueness.map(|preserves_uniqueness| {
@@ -236,10 +244,10 @@ fn unary_func(
         }
     };
 
-    let (output_type, introduces_nulls_fn) = if let Some(output_type) = output_type {
+    let (output_type, mut introduces_nulls_fn) = if let Some(output_type) = output_type {
         let introduces_nulls_fn = quote! {
             fn introduces_nulls(&self) -> bool {
-                <#output_type as mz_repr::DatumType<'_, ()>>::nullable()
+                <#output_type as ::mz_repr::DatumType<'_, ()>>::nullable()
             }
         };
         let output_type = quote! { <#output_type> };
@@ -248,7 +256,13 @@ fn unary_func(
         (quote! { Self::Output }, None)
     };
 
-    let arena = arena.then(|| quote! { , temp_storage });
+    if let Some(introduces_nulls) = introduces_nulls {
+        introduces_nulls_fn = Some(quote! {
+            fn introduces_nulls(&self) -> bool {
+                #introduces_nulls
+            }
+        });
+    }
 
     let could_error_fn = could_error.map(|could_error| {
         quote! {
@@ -267,7 +281,7 @@ fn unary_func(
             type Output = #output_ty;
 
             fn call(&self, a: Self::Input) -> Self::Output {
-                #fn_name(a #arena)
+                #fn_name(a)
             }
 
             fn output_type(&self, input_type: mz_repr::ColumnType) -> mz_repr::ColumnType {
@@ -320,13 +334,18 @@ fn binary_func(
         negate,
         could_error,
         propagates_nulls,
+        introduces_nulls,
     } = modifiers;
 
     if preserves_uniqueness.is_some() {
-        return Err(darling::Error::unknown_field("preserves_uniqueness"));
+        return Err(darling::Error::unknown_field(
+            "preserves_uniqueness not supported for binary functions",
+        ));
     }
     if inverse.is_some() {
-        return Err(darling::Error::unknown_field("inverse"));
+        return Err(darling::Error::unknown_field(
+            "inverse not supported for binary functions",
+        ));
     }
 
     let negate_fn = negate.map(|negate| {
@@ -355,10 +374,10 @@ fn binary_func(
         }
     };
 
-    let (output_type, introduces_nulls_fn) = if let Some(output_type) = output_type {
+    let (output_type, mut introduces_nulls_fn) = if let Some(output_type) = output_type {
         let introduces_nulls_fn = quote! {
             fn introduces_nulls(&self) -> bool {
-                <#output_type as mz_repr::DatumType<'_, ()>>::nullable()
+                <#output_type as ::mz_repr::DatumType<'_, ()>>::nullable()
             }
         };
         let output_type = quote! { <#output_type> };
@@ -367,6 +386,14 @@ fn binary_func(
         (quote! { Self::Output }, None)
     };
 
+    if let Some(introduces_nulls) = introduces_nulls {
+        introduces_nulls_fn = Some(quote! {
+            fn introduces_nulls(&self) -> bool {
+                #introduces_nulls
+            }
+        });
+    }
+
     let arena = if arena {
         quote! { , temp_storage }
     } else {

src/expr-derive/src/lib.rs

@@ -10,6 +10,43 @@
 //! Proc macros to derive SQL function traits.
 
 /// Derive function traits for SQL functions.
+///
+/// The `sqlfunc` attribute macro is used to derive SQL function traits for unary and binary
+/// functions. Depending on the kind of function, it will implement the appropriate traits.
+/// For unary functions, it implements the `EagerUnaryFunc` trait, and for binary functions,
+/// it implements the `EagerBinaryFunc` trait.
+///
+/// The macro takes the following arguments:
+/// * `is_monotone`: An expression indicating whether the function is monotone. For unary functions,
+///   it should be an expression that evaluates to a boolean. For binary functions, it should be a
+///   tuple of two expressions, each evaluating to a boolean. See `LazyBinaryFunc` for details.
+/// * `sqlname`: The SQL name of the function.
+/// * `preserves_uniqueness`: A boolean indicating whether the function preserves uniqueness.
+///   Unary functions only.
+/// * `inverse`: A function that is the inverse of the function. Applies to unary functions only.
+/// * `negate`: A function that negates the function. Applies to binary functions only. For example,
+///   the `!=` operator is the negation of the `=` operator, and we'd mark the `Eq` function with
+///   `negate = Some(BinaryFunc::NotEq)`.
+/// * `is_infix_op`: A boolean indicating whether the function is an infix operator. Applies to
+///   binary functions only.
+/// * `output_type`: The output type of the function.
+/// * `could_error`: A boolean indicating whether the function could error.
+/// * `propagate_nulls`: A boolean indicating whether the function propagates nulls. Applies to
+///   binary functions only.
+///
+/// # Limitations
+/// * The input and output types can contain lifetime parameters, as long as they are `'a`.
+/// * Unary functions cannot yet receive a `&RowArena` as an argument.
+/// * The `output_type`
+///
+/// # Examples
+/// ```ignore
+/// use mz_expr_derive::sqlfunc;
+/// #[sqlfunc(sqlname = "!")]
+/// fn negate(a: bool) -> bool {
+///    !a
+/// }
+/// ```
 #[proc_macro_attribute]
 pub fn sqlfunc(
     attr: proc_macro::TokenStream,

src/expr/src/scalar/snapshots/mz_expr__scalar__func__add_date_interval.snap

@@ -47,7 +47,7 @@ impl<'a> crate::func::binary::EagerBinaryFunc<'a> for AddDateInterval {
             )
     }
     fn introduces_nulls(&self) -> bool {
-        <CheckedTimestamp<NaiveDateTime> as mz_repr::DatumType<'_, ()>>::nullable()
+        <CheckedTimestamp<NaiveDateTime> as ::mz_repr::DatumType<'_, ()>>::nullable()
     }
     fn is_infix_op(&self) -> bool {
         true