From c17760cec44ee566b3447e8e0891c8bd05183d59 Mon Sep 17 00:00:00 2001
From: Maaz Ahmed <maaz.a@subcom.tech>
Date: Wed, 27 Dec 2023 11:55:03 +0530
Subject: [PATCH] refactor: separate fn name and args formatting in QryFunc

---
 src/query/fns/aggregate.rs | 20 +++++-------
 src/query/fns/label.rs     | 30 ++++++++----------
 src/query/fns/mod.rs       | 62 +++++++++++++++++++++++++++++++++++---
 src/query/fns/rollup.rs    |  4 +--
 src/query/fns/transform.rs | 24 +++++++--------
 5 files changed, 90 insertions(+), 50 deletions(-)

diff --git a/src/query/fns/aggregate.rs b/src/query/fns/aggregate.rs
index cdf5e74..56b50e1 100644
--- a/src/query/fns/aggregate.rs
+++ b/src/query/fns/aggregate.rs
@@ -1,5 +1,5 @@
 //! Aggregate query functions (includes PromQL aggregation operators)
-use super::{basic_fn, QryFunc};
+use super::{basic_fn, qry_fn, QryFunc};
 use crate::{
     query::{
         ops::{
@@ -133,7 +133,7 @@ pub fn count_values<'a>(
     label: &'_ str,
     vec_expr: impl Operable + 'static,
 ) -> AggrFunc<'a, impl Fn(&mut fmt::Formatter) -> fmt::Result + '_> {
-    QryFunc::new(move |f| write!(f, r#"count_values("{label}", {vec_expr})"#)).into()
+    qry_fn!(count_values, r#""{label}", {vec_expr}"#).into()
 }
 
 /// The topk aggregate operator/function
@@ -142,7 +142,7 @@ pub fn topk<'a>(
     k: usize,
     vec_expr: impl Operable + 'static,
 ) -> AggrFunc<'a, impl Fn(&mut fmt::Formatter) -> fmt::Result> {
-    QryFunc::new(move |f| write!(f, "topk({k}, {vec_expr})")).into()
+    qry_fn!(topk, "{k}, {vec_expr}").into()
 }
 
 /// The bottomk aggregate operator/function
@@ -151,7 +151,7 @@ pub fn bottomk<'a>(
     k: usize,
     vec_expr: impl Operable + 'static,
 ) -> AggrFunc<'a, impl Fn(&mut fmt::Formatter) -> fmt::Result> {
-    QryFunc::new(move |f| write!(f, "bottomk({k}, {vec_expr})")).into()
+    qry_fn!(bottomk, "{k}, {vec_expr}").into()
 }
 
 /// The bottomk aggregate operator/function (phi value should always be >= 0 and <= 1)
@@ -160,7 +160,7 @@ pub fn quantile<'a>(
     phi: f32,
     vec_expr: impl Operable + 'static,
 ) -> AggrFunc<'a, impl Fn(&mut fmt::Formatter) -> fmt::Result> {
-    QryFunc::new(move |f| write!(f, "quantile({phi}, {vec_expr})")).into()
+    qry_fn!(quantile, "{phi}, {vec_expr}").into()
 }
 
 #[cfg(test)]
@@ -171,20 +171,14 @@ mod tests {
 
     #[test]
     fn by_mod() {
-        let agr = AggrFunc {
-            inner: QryFunc::new(|f| f.write_str("func(x)")),
-            mod_type: Default::default(),
-        };
+        let agr: AggrFunc<_> = qry_fn!(func, "x").into();
         let by_str = agr.by(["label", "label2"]).to_string();
         assert_eq!(by_str, "func(x) by (label,label2)");
     }
 
     #[test]
     fn without_mod() {
-        let agr = AggrFunc {
-            inner: QryFunc::new(|f| f.write_str("func(x)")),
-            mod_type: Default::default(),
-        };
+        let agr: AggrFunc<_> = qry_fn!(func, "x").into();
         let by_str = agr.without(["label", "label2"]).to_string();
         assert_eq!(by_str, "func(x) without (label,label2)");
     }
diff --git a/src/query/fns/label.rs b/src/query/fns/label.rs
index 2bc3d5b..15b0e28 100644
--- a/src/query/fns/label.rs
+++ b/src/query/fns/label.rs
@@ -2,9 +2,10 @@
 
 use core::fmt;
 
-use crate::query::ops::Operable;
-
-use super::QryFunc;
+use crate::query::{
+    fns::{qry_fn, QryFunc},
+    ops::Operable,
+};
 
 /// MetricsQL's label_map query function
 pub fn mql_label_map<'a>(
@@ -12,13 +13,11 @@ pub fn mql_label_map<'a>(
     label: &'a str,
     src_dst_pairs: &'a [&'a str],
 ) -> QryFunc<impl Fn(&mut fmt::Formatter) -> fmt::Result + 'a> {
-    QryFunc::new(move |f| {
-        write!(
-            f,
-            "label_map({qry_expr},\"{label}\",\"{pairs}\")",
-            pairs = src_dst_pairs.join("\",\"")
-        )
-    })
+    qry_fn!(
+        label_map,
+        "{qry_expr},\"{label}\",\"{pairs}\"",
+        pairs = src_dst_pairs.join("\",\"")
+    )
 }
 
 fn basic_label_fn<'a>(
@@ -26,13 +25,10 @@ fn basic_label_fn<'a>(
     qry_expr: impl Operable + 'a,
     labels: &'a [&'a str],
 ) -> QryFunc<impl Fn(&mut fmt::Formatter) -> fmt::Result + 'a> {
-    QryFunc::new(move |f| {
-        write!(
-            f,
-            "{fn_name}({qry_expr},\"{labels}\")",
-            labels = labels.join("\",\"")
-        )
-    })
+    qry_fn!(var: fn_name,
+        "{qry_expr},\"{labels}\"",
+        labels = labels.join("\",\"")
+    )
 }
 
 /// MetricsQL's sort_by_label query function
diff --git a/src/query/fns/mod.rs b/src/query/fns/mod.rs
index d7f8cab..6314ea1 100644
--- a/src/query/fns/mod.rs
+++ b/src/query/fns/mod.rs
@@ -22,23 +22,46 @@ pub mod rollup;
 #[cfg(feature = "metricsql")]
 pub mod transform;
 
+// This macro takes a name and formatting arguments which are directly passed to the `write` macro
+// and constructs a QryFunc type. The name is generally provided directly as tokens (not as a quoted string).
+// However, variables can be passed to the name field by specifying `var: var_name`
+macro_rules! qry_fn {
+    (var: $name: ident, { $($func: tt)+ }) => {
+        QryFunc::new($name, move $($func)+)
+    };
+    (var: $name: ident, $($fmt: tt)+) => {
+        QryFunc::new($name, move |f| write!(f, $($fmt)+))
+    };
+    ($name: tt, { $($func: tt)+ }) => {
+        QryFunc::new(stringify!($name), move $($func)+)
+    };
+    ($name: tt, $($fmt: tt)+) => {
+        QryFunc::new(stringify!($name), move |f| write!(f, $($fmt)+))
+    };
+}
+
+pub(crate) use qry_fn;
+
 /// Type that represents a query function. Meant to be constructed through the query functions provided in the [`fns`](crate::query::fns) module.
 ///
 /// This internally holds an actual function (closure), that acts as a type that implements the `Display` trait.
 #[derive(Debug, Clone)]
 pub struct QryFunc<F: Fn(&mut fmt::Formatter) -> fmt::Result> {
-    inner: F,
+    name: &'static str,
+    args: F,
 }
 
 impl<F: Fn(&mut fmt::Formatter) -> fmt::Result> QryFunc<F> {
-    fn new(func: F) -> Self {
-        Self { inner: func }
+    fn new(name: &'static str, func: F) -> Self {
+        Self { name, args: func }
     }
 }
 
 impl<F: Fn(&mut fmt::Formatter) -> fmt::Result> Display for QryFunc<F> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        (self.inner)(f)
+        write!(f, "{name}(", name = self.name)?; // write name and the starting parenthesis
+        (self.args)(f)?; // write formatted arguments
+        f.write_str(")") // closing parenthesis
     }
 }
 
@@ -58,5 +81,34 @@ fn basic_fn(
     name: &'static str,
     expr: impl Operable + 'static,
 ) -> QryFunc<impl Fn(&mut fmt::Formatter) -> fmt::Result + '_> {
-    QryFunc::new(move |f| write!(f, "{name}({expr})"))
+    qry_fn!(var: name, "{expr}")
+}
+
+#[cfg(test)]
+mod tests {
+    use super::QryFunc;
+
+    #[test]
+    fn func_format() {
+        let func = QryFunc::new("function", |f| f.write_str("arg1, arg2")).to_string();
+        assert_eq!(func, "function(arg1, arg2)")
+    }
+
+    #[test]
+    fn func_macro() {
+        // Basic usage
+        let func = qry_fn!(name, "args").to_string();
+        assert_eq!(func, "name(args)");
+        // Custom internal fn
+        let func = qry_fn!(name, { |f| f.write_str("args") }).to_string();
+        assert_eq!(func, "name(args)");
+        // With Variables
+        let x = "name";
+        let func = qry_fn!(var: x, "args").to_string();
+        assert_eq!(func, "name(args)");
+        // Custom with Variables
+        let x = "name";
+        let func = qry_fn!(var: x, { |f| f.write_str("args") }).to_string();
+        assert_eq!(func, "name(args)");
+    }
 }
diff --git a/src/query/fns/rollup.rs b/src/query/fns/rollup.rs
index 934e243..25683ca 100644
--- a/src/query/fns/rollup.rs
+++ b/src/query/fns/rollup.rs
@@ -2,7 +2,7 @@ use core::fmt;
 
 use crate::query::ops::Operable;
 
-use super::{basic_fn, QryFunc};
+use super::{basic_fn, qry_fn, QryFunc};
 
 /// The avg_over_time rollup query function
 #[inline]
@@ -50,7 +50,7 @@ pub fn quantile_over_time(
     phi: f32,
     range_vec: impl Operable + 'static,
 ) -> QryFunc<impl Fn(&mut fmt::Formatter) -> fmt::Result> {
-    QryFunc::new(move |f| write!(f, "quantile_over_time({phi},{range_vec})"))
+    qry_fn!(quantile_over_time, "{phi},{range_vec}")
 }
 
 /// The stddev_over_time rollup query function
diff --git a/src/query/fns/transform.rs b/src/query/fns/transform.rs
index e5838fd..7a8b1e8 100644
--- a/src/query/fns/transform.rs
+++ b/src/query/fns/transform.rs
@@ -2,7 +2,7 @@ use core::fmt;
 
 use crate::query::ops::Operable;
 
-use super::QryFunc;
+use super::{qry_fn, QryFunc};
 
 /// MetricsQL `limit_offset` query function
 pub fn mql_limit_offset(
@@ -10,24 +10,22 @@ pub fn mql_limit_offset(
     offset: usize,
     qry_expr: impl Operable,
 ) -> QryFunc<impl Fn(&mut fmt::Formatter) -> fmt::Result> {
-    QryFunc::new(move |f| write!(f, "limit_offset({limit},{offset},{qry_expr})"))
+    qry_fn!(limit_offset, "{limit},{offset},{qry_expr}")
 }
 
 /// MetricsQL `union` query function
 pub fn mql_union(
     qry_expressions: &[impl Operable],
 ) -> QryFunc<impl Fn(&mut fmt::Formatter) -> fmt::Result + '_> {
-    QryFunc::new(|f| {
-        write!(
-            f,
-            "union({queries})",
-            queries = qry_expressions
-                .iter()
-                .map(ToString::to_string)
-                .collect::<Vec<_>>()
-                .join(",")
-        )
-    })
+    qry_fn!(
+        union,
+        "{queries}",
+        queries = qry_expressions
+            .iter()
+            .map(ToString::to_string)
+            .collect::<Vec<_>>()
+            .join(",")
+    )
 }
 
 #[cfg(test)]
-- 
GitLab