Remove needless fmt_sql (#7409)

Remove bespoke `fmt_sql` implementations for scalar functions that do
not need special syntax. These overrides only duplicated the generic
call formatting and could drift from the registered `ScalarFnId`;
relying on the default keeps expression display consistent, preserves
qualified function names/options, and reduces boilerplate for new scalar
functions.

Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: "Nicholas Gates" <nick@nickgates.com>

Author: gatesn

vortex-array/src/expr/mod.rs

@@ -207,7 +207,7 @@ mod tests {
             "(($.col1 < $.col2) or ($.col1 != $.col2))"
         );
 
-        assert_eq!(not(col1).to_string(), "not($.col1)");
+        assert_eq!(not(col1).to_string(), "vortex.not($.col1)");
 
         assert_eq!(
             select(vec![FieldName::from("col1")], root()).to_string(),

vortex-array/src/scalar_fn/fns/fill_null/mod.rs

@@ -3,8 +3,6 @@
 
 mod kernel;
 
-use std::fmt::Formatter;
-
 pub use kernel::*;
 use vortex_error::VortexResult;
 use vortex_error::vortex_bail;
@@ -66,19 +64,6 @@ impl ScalarFnVTable for FillNull {
         }
     }
 
-    fn fmt_sql(
-        &self,
-        _options: &Self::Options,
-        expr: &Expression,
-        f: &mut Formatter<'_>,
-    ) -> std::fmt::Result {
-        write!(f, "fill_null(")?;
-        expr.child(0).fmt_sql(f)?;
-        write!(f, ", ")?;
-        expr.child(1).fmt_sql(f)?;
-        write!(f, ")")
-    }
-
     fn return_dtype(&self, _options: &Self::Options, arg_dtypes: &[DType]) -> VortexResult<DType> {
         vortex_ensure!(
             arg_dtypes[0].eq_ignore_nullability(&arg_dtypes[1]),
@@ -265,6 +250,6 @@ mod tests {
     #[test]
     fn test_display() {
         let expr = fill_null(get_item("value", root()), lit(0i32));
-        assert_eq!(expr.to_string(), "fill_null($.value, 0i32)");
+        assert_eq!(expr.to_string(), "vortex.fill_null($.value, 0i32)");
     }
 }

vortex-array/src/scalar_fn/fns/is_null.rs

@@ -1,8 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
-use std::fmt::Formatter;
-
 use vortex_error::VortexResult;
 use vortex_session::VortexSession;
 
@@ -60,17 +58,6 @@ impl ScalarFnVTable for IsNull {
         }
     }
 
-    fn fmt_sql(
-        &self,
-        _options: &Self::Options,
-        expr: &Expression,
-        f: &mut Formatter<'_>,
-    ) -> std::fmt::Result {
-        write!(f, "is_null(")?;
-        expr.child(0).fmt_sql(f)?;
-        write!(f, ")")
-    }
-
     fn return_dtype(&self, _options: &Self::Options, _arg_dtypes: &[DType]) -> VortexResult<DType> {
         Ok(DType::Bool(Nullability::NonNullable))
     }
@@ -250,10 +237,10 @@ mod tests {
     #[test]
     fn test_display() {
         let expr = is_null(get_item("name", root()));
-        assert_eq!(expr.to_string(), "is_null($.name)");
+        assert_eq!(expr.to_string(), "vortex.is_null($.name)");
 
         let expr2 = is_null(root());
-        assert_eq!(expr2.to_string(), "is_null($)");
+        assert_eq!(expr2.to_string(), "vortex.is_null($)");
     }
 
     #[test]

vortex-array/src/scalar_fn/fns/list_contains/mod.rs

@@ -3,7 +3,6 @@
 
 mod kernel;
 
-use std::fmt::Formatter;
 use std::ops::BitOr;
 
 use arrow_buffer::bit_iterator::BitIndexIterator;
@@ -89,19 +88,6 @@ impl ScalarFnVTable for ListContains {
             ),
         }
     }
-    fn fmt_sql(
-        &self,
-        _options: &Self::Options,
-        expr: &Expression,
-        f: &mut Formatter<'_>,
-    ) -> std::fmt::Result {
-        write!(f, "contains(")?;
-        expr.child(0).fmt_sql(f)?;
-        write!(f, ", ")?;
-        expr.child(1).fmt_sql(f)?;
-        write!(f, ")")
-    }
-
     fn return_dtype(&self, _options: &Self::Options, arg_dtypes: &[DType]) -> VortexResult<DType> {
         let list_dtype = &arg_dtypes[0];
         let needle_dtype = &arg_dtypes[1];
@@ -670,10 +656,10 @@ mod tests {
     #[test]
     pub fn test_display() {
         let expr = list_contains(get_item("tags", root()), lit("urgent"));
-        assert_eq!(expr.to_string(), "contains($.tags, \"urgent\")");
+        assert_eq!(expr.to_string(), "vortex.list.contains($.tags, \"urgent\")");
 
         let expr2 = list_contains(root(), lit(42));
-        assert_eq!(expr2.to_string(), "contains($, 42i32)");
+        assert_eq!(expr2.to_string(), "vortex.list.contains($, 42i32)");
     }
 
     #[test]

vortex-array/src/scalar_fn/fns/mask/mod.rs

@@ -2,7 +2,6 @@
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
 mod kernel;
-use std::fmt::Formatter;
 
 pub use kernel::*;
 use vortex_error::VortexExpect;
@@ -72,19 +71,6 @@ impl ScalarFnVTable for Mask {
         }
     }
 
-    fn fmt_sql(
-        &self,
-        _options: &Self::Options,
-        expr: &Expression,
-        f: &mut Formatter<'_>,
-    ) -> std::fmt::Result {
-        write!(f, "mask(")?;
-        expr.child(0).fmt_sql(f)?;
-        write!(f, ", ")?;
-        expr.child(1).fmt_sql(f)?;
-        write!(f, ")")
-    }
-
     fn return_dtype(&self, _options: &Self::Options, arg_dtypes: &[DType]) -> VortexResult<DType> {
         vortex_ensure!(
             arg_dtypes[1] == DType::Bool(Nullability::NonNullable),

vortex-array/src/scalar_fn/fns/merge.rs

@@ -84,22 +84,6 @@ impl ScalarFnVTable for Merge {
         ChildName::from(Arc::from(format!("{}", child_idx)))
     }
 
-    fn fmt_sql(
-        &self,
-        _options: &Self::Options,
-        expr: &Expression,
-        f: &mut Formatter<'_>,
-    ) -> std::fmt::Result {
-        write!(f, "merge(")?;
-        for (i, child) in expr.children().iter().enumerate() {
-            child.fmt_sql(f)?;
-            if i + 1 < expr.children().len() {
-                write!(f, ", ")?;
-            }
-        }
-        write!(f, ")")
-    }
-
     fn return_dtype(&self, options: &Self::Options, arg_dtypes: &[DType]) -> VortexResult<DType> {
         let mut field_names = Vec::new();
         let mut arrays = Vec::new();
@@ -550,10 +534,13 @@ mod tests {
     #[test]
     pub fn test_display() {
         let expr = merge([get_item("struct1", root()), get_item("struct2", root())]);
-        assert_eq!(expr.to_string(), "merge($.struct1, $.struct2)");
+        assert_eq!(
+            expr.to_string(),
+            "vortex.merge($.struct1, $.struct2, opts=Error)"
+        );
 
         let expr2 = merge(vec![get_item("a", root())]);
-        assert_eq!(expr2.to_string(), "merge($.a)");
+        assert_eq!(expr2.to_string(), "vortex.merge($.a, opts=Error)");
     }
 
     #[test]

vortex-array/src/scalar_fn/fns/not/mod.rs

@@ -3,8 +3,6 @@
 
 mod kernel;
 
-use std::fmt::Formatter;
-
 pub use kernel::*;
 use vortex_error::VortexResult;
 use vortex_error::vortex_bail;
@@ -19,7 +17,6 @@ use crate::arrays::ConstantArray;
 use crate::arrays::bool::BoolArrayExt;
 use crate::builtins::ArrayBuiltins;
 use crate::dtype::DType;
-use crate::expr::Expression;
 use crate::scalar::Scalar;
 use crate::scalar_fn::Arity;
 use crate::scalar_fn::ChildName;
@@ -62,17 +59,6 @@ impl ScalarFnVTable for Not {
         }
     }
 
-    fn fmt_sql(
-        &self,
-        _options: &Self::Options,
-        expr: &Expression,
-        f: &mut Formatter<'_>,
-    ) -> std::fmt::Result {
-        write!(f, "not(")?;
-        expr.child(0).fmt_sql(f)?;
-        write!(f, ")")
-    }
-
     fn return_dtype(&self, _options: &Self::Options, arg_dtypes: &[DType]) -> VortexResult<DType> {
         let child_dtype = &arg_dtypes[0];
         if !matches!(child_dtype, DType::Bool(_)) {
@@ -151,8 +137,8 @@ mod tests {
         let a = not(get_item("a", root()));
         let b = get_item("a", not(root()));
         assert_ne!(a.to_string(), b.to_string());
-        assert_eq!(a.to_string(), "not($.a)");
-        assert_eq!(b.to_string(), "not($).a");
+        assert_eq!(a.to_string(), "vortex.not($.a)");
+        assert_eq!(b.to_string(), "vortex.not($).a");
     }
 
     #[test]

vortex-array/src/scalar_fn/vtable.rs

@@ -22,6 +22,7 @@ use crate::dtype::DType;
 use crate::expr::Expression;
 use crate::expr::StatsCatalog;
 use crate::expr::stats::Stat;
+use crate::expr::traversal::Node;
 use crate::scalar_fn::ScalarFnId;
 use crate::scalar_fn::ScalarFnRef;
 use crate::scalar_fn::TypedScalarFnInstance;
@@ -77,7 +78,21 @@ pub trait ScalarFnVTable: 'static + Sized + Clone + Send + Sync {
         options: &Self::Options,
         expr: &Expression,
         f: &mut Formatter<'_>,
-    ) -> fmt::Result;
+    ) -> fmt::Result {
+        write!(f, "{}(", self.id())?;
+        let nchildren = expr.children_count();
+        for (i, child) in expr.children().iter().enumerate() {
+            child.fmt_sql(f)?;
+            if i + 1 < nchildren {
+                write!(f, ", ")?;
+            }
+        }
+        let opts = format!("{}", options);
+        if !opts.is_empty() {
+            write!(f, ", opts={}", opts)?;
+        }
+        write!(f, ")")
+    }
 
     /// Coerce the arguments of this function.
     ///

vortex-tensor/public-api.lock

@@ -284,8 +284,6 @@ pub fn vortex_tensor::scalar_fns::cosine_similarity::CosineSimilarity::child_nam
 
 pub fn vortex_tensor::scalar_fns::cosine_similarity::CosineSimilarity::execute(&self, &Self::Options, &dyn vortex_array::scalar_fn::vtable::ExecutionArgs, &mut vortex_array::executor::ExecutionCtx) -> vortex_error::VortexResult<vortex_array::array::erased::ArrayRef>
 
-pub fn vortex_tensor::scalar_fns::cosine_similarity::CosineSimilarity::fmt_sql(&self, &Self::Options, &vortex_array::expr::expression::Expression, &mut core::fmt::Formatter<'_>) -> core::fmt::Result
-
 pub fn vortex_tensor::scalar_fns::cosine_similarity::CosineSimilarity::id(&self) -> vortex_array::scalar_fn::ScalarFnId
 
 pub fn vortex_tensor::scalar_fns::cosine_similarity::CosineSimilarity::is_fallible(&self, &Self::Options) -> bool
@@ -326,8 +324,6 @@ pub fn vortex_tensor::scalar_fns::inner_product::InnerProduct::child_name(&self,
 
 pub fn vortex_tensor::scalar_fns::inner_product::InnerProduct::execute(&self, &Self::Options, &dyn vortex_array::scalar_fn::vtable::ExecutionArgs, &mut vortex_array::executor::ExecutionCtx) -> vortex_error::VortexResult<vortex_array::array::erased::ArrayRef>
 
-pub fn vortex_tensor::scalar_fns::inner_product::InnerProduct::fmt_sql(&self, &Self::Options, &vortex_array::expr::expression::Expression, &mut core::fmt::Formatter<'_>) -> core::fmt::Result
-
 pub fn vortex_tensor::scalar_fns::inner_product::InnerProduct::id(&self) -> vortex_array::scalar_fn::ScalarFnId
 
 pub fn vortex_tensor::scalar_fns::inner_product::InnerProduct::is_fallible(&self, &Self::Options) -> bool
@@ -370,8 +366,6 @@ pub fn vortex_tensor::scalar_fns::l2_denorm::L2Denorm::child_name(&self, &Self::
 
 pub fn vortex_tensor::scalar_fns::l2_denorm::L2Denorm::execute(&self, &Self::Options, &dyn vortex_array::scalar_fn::vtable::ExecutionArgs, &mut vortex_array::executor::ExecutionCtx) -> vortex_error::VortexResult<vortex_array::array::erased::ArrayRef>
 
-pub fn vortex_tensor::scalar_fns::l2_denorm::L2Denorm::fmt_sql(&self, &Self::Options, &vortex_array::expr::expression::Expression, &mut core::fmt::Formatter<'_>) -> core::fmt::Result
-
 pub fn vortex_tensor::scalar_fns::l2_denorm::L2Denorm::id(&self) -> vortex_array::scalar_fn::ScalarFnId
 
 pub fn vortex_tensor::scalar_fns::l2_denorm::L2Denorm::is_fallible(&self, &Self::Options) -> bool
@@ -416,8 +410,6 @@ pub fn vortex_tensor::scalar_fns::l2_norm::L2Norm::child_name(&self, &Self::Opti
 
 pub fn vortex_tensor::scalar_fns::l2_norm::L2Norm::execute(&self, &Self::Options, &dyn vortex_array::scalar_fn::vtable::ExecutionArgs, &mut vortex_array::executor::ExecutionCtx) -> vortex_error::VortexResult<vortex_array::array::erased::ArrayRef>
 
-pub fn vortex_tensor::scalar_fns::l2_norm::L2Norm::fmt_sql(&self, &Self::Options, &vortex_array::expr::expression::Expression, &mut core::fmt::Formatter<'_>) -> core::fmt::Result
-
 pub fn vortex_tensor::scalar_fns::l2_norm::L2Norm::id(&self) -> vortex_array::scalar_fn::ScalarFnId
 
 pub fn vortex_tensor::scalar_fns::l2_norm::L2Norm::is_fallible(&self, &Self::Options) -> bool
@@ -504,8 +496,6 @@ pub fn vortex_tensor::scalar_fns::sorf_transform::SorfTransform::child_name(&sel
 
 pub fn vortex_tensor::scalar_fns::sorf_transform::SorfTransform::execute(&self, &Self::Options, &dyn vortex_array::scalar_fn::vtable::ExecutionArgs, &mut vortex_array::executor::ExecutionCtx) -> vortex_error::VortexResult<vortex_array::array::erased::ArrayRef>
 
-pub fn vortex_tensor::scalar_fns::sorf_transform::SorfTransform::fmt_sql(&self, &Self::Options, &vortex_array::expr::expression::Expression, &mut core::fmt::Formatter<'_>) -> core::fmt::Result
-
 pub fn vortex_tensor::scalar_fns::sorf_transform::SorfTransform::id(&self) -> vortex_array::scalar_fn::ScalarFnId
 
 pub fn vortex_tensor::scalar_fns::sorf_transform::SorfTransform::is_fallible(&self, &Self::Options) -> bool

vortex-tensor/src/scalar_fns/cosine_similarity.rs

@@ -3,8 +3,6 @@
 
 //! Cosine similarity expression for tensor-like types.
 
-use std::fmt::Formatter;
-
 use num_traits::Zero;
 use vortex_array::ArrayRef;
 use vortex_array::ExecutionCtx;
@@ -96,19 +94,6 @@ impl ScalarFnVTable for CosineSimilarity {
         }
     }
 
-    fn fmt_sql(
-        &self,
-        _options: &Self::Options,
-        expr: &Expression,
-        f: &mut Formatter<'_>,
-    ) -> std::fmt::Result {
-        write!(f, "cosine_similarity(")?;
-        expr.child(0).fmt_sql(f)?;
-        write!(f, ", ")?;
-        expr.child(1).fmt_sql(f)?;
-        write!(f, ")")
-    }
-
     fn return_dtype(&self, _options: &Self::Options, arg_dtypes: &[DType]) -> VortexResult<DType> {
         let lhs = &arg_dtypes[0];
         let rhs = &arg_dtypes[1];