Refactor ShapeBuilder away from a single shared global value (#491)

extension/partiql-extension-ddl/src/ddl.rs

@@ -228,38 +228,37 @@ mod tests {
     use indexmap::IndexSet;
     use partiql_types::{
         struct_fields, type_array, type_bag, type_float64, type_int8, type_string, type_struct,
-        PartiqlShapeBuilder, StructConstraint,
+        PartiqlShapeBuilder, ShapeBuilderExtensions, StructConstraint,
     };
 
     #[test]
     fn ddl_test() {
+        let mut bld = PartiqlShapeBuilder::default();
         let nested_attrs = struct_fields![
             (
                 "a",
-                PartiqlShapeBuilder::init_or_get().any_of(vec![
-                    PartiqlShapeBuilder::init_or_get().new_static(Static::DecimalP(5, 4)),
-                    PartiqlShapeBuilder::init_or_get().new_static(Static::Int8),
-                ])
+                [
+                    bld.new_static(Static::DecimalP(5, 4)),
+                    bld.new_static(Static::Int8),
+                ]
+                .into_any_of(&mut bld)
             ),
-            ("b", type_array![type_string![]]),
-            ("c", type_float64!()),
+            ("b", type_string![bld].into_array(&mut bld)),
+            ("c", type_float64!(bld)),
         ];
-        let details = type_struct![IndexSet::from([nested_attrs])];
+        let details = type_struct![bld, IndexSet::from([nested_attrs])];
 
         let fields = struct_fields![
-            ("employee_id", type_int8![]),
-            ("full_name", type_string![]),
-            (
-                "salary",
-                PartiqlShapeBuilder::init_or_get().new_static(Static::DecimalP(8, 2))
-            ),
+            ("employee_id", type_int8![bld]),
+            ("full_name", type_string![bld]),
+            ("salary", bld.new_static(Static::DecimalP(8, 2))),
             ("details", details),
-            ("dependents", type_array![type_string![]])
+            ("dependents", type_array![bld, type_string![bld]])
+        ];
+        let ty = type_bag![
+            bld,
+            type_struct![bld, IndexSet::from([fields, StructConstraint::Open(false)])]
         ];
-        let ty = type_bag![type_struct![IndexSet::from([
-            fields,
-            StructConstraint::Open(false)
-        ])]];
 
         let expected_compact = r#""employee_id" TINYINT,"full_name" VARCHAR,"salary" DECIMAL(8, 2),"details" STRUCT<"a": UNION<DECIMAL(5, 4),TINYINT>,"b": type_array<VARCHAR>,"c": DOUBLE>,"dependents" type_array<VARCHAR>"#;
         let expected_pretty = r#""employee_id" TINYINT,

extension/partiql-extension-ddl/tests/ddl-tests.rs

@@ -4,26 +4,30 @@ use partiql_types::{
     struct_fields, type_bag, type_int, type_string, type_struct, PartiqlShapeBuilder,
     StructConstraint, StructField,
 };
-use partiql_types::{BagType, Static, StructType};
+use partiql_types::{Static, StructType};
 
 #[test]
 fn basic_ddl_test() {
-    let details_fields = struct_fields![("age", type_int!())];
-    let details = type_struct![IndexSet::from([details_fields])];
+    let mut bld = PartiqlShapeBuilder::default();
+    let details_fields = struct_fields![("age", type_int!(bld))];
+    let details = type_struct![bld, IndexSet::from([details_fields])];
     let fields = [
-        StructField::new("id", type_int!()),
-        StructField::new("name", type_string!()),
-        StructField::new(
-            "address",
-            PartiqlShapeBuilder::init_or_get().new_non_nullable_static(Static::String),
-        ),
+        StructField::new("id", type_int!(bld)),
+        StructField::new("name", type_string!(bld)),
+        StructField::new("address", bld.new_non_nullable_static(Static::String)),
         StructField::new_optional("details", details.clone()),
     ]
     .into();
-    let shape = type_bag![type_struct![IndexSet::from([
-        StructConstraint::Fields(fields),
-        StructConstraint::Open(false)
-    ])]];
+    let shape = type_bag![
+        bld,
+        type_struct![
+            bld,
+            IndexSet::from([
+                StructConstraint::Fields(fields),
+                StructConstraint::Open(false)
+            ])
+        ]
+    ];
 
     let ddl_compact = PartiqlBasicDdlEncoder::new(DdlFormat::Compact);
     let actual = ddl_compact.ddl(&shape).expect("ddl_output");

partiql-ast/src/builder.rs

@@ -16,9 +16,8 @@ where
     }
 
     pub fn node<T>(&mut self, node: T) -> AstNode<T> {
-        let id = self.id_gen.id();
-        let id = id.read().expect("NodId read lock");
-        AstNode { id: *id, node }
+        let id = self.id_gen.next_id();
+        AstNode { id, node }
     }
 }
 
@@ -36,3 +35,57 @@ pub type AstNodeBuilderWithAutoId = AstNodeBuilder<AutoNodeIdGenerator>;
 
 /// A [`AstNodeBuilder`] whose 'fresh' [`NodeId`]s are always `0`; Useful for testing
 pub type AstNodeBuilderWithNullId = AstNodeBuilder<NullIdGenerator>;
+
+#[cfg(test)]
+mod tests {
+    use super::AstNodeBuilderWithAutoId;
+    use crate::ast;
+
+    use crate::visit::{Traverse, Visit, Visitor};
+    use partiql_common::node::NodeId;
+    use partiql_common::pretty::ToPretty;
+
+    #[test]
+    fn unique_ids() {
+        let mut bld = AstNodeBuilderWithAutoId::default();
+
+        let mut i64_to_expr = |n| Box::new(ast::Expr::Lit(bld.node(ast::Lit::Int64Lit(n))));
+
+        let lhs = i64_to_expr(5);
+        let v1 = i64_to_expr(42);
+        let v2 = i64_to_expr(13);
+        let list = bld.node(ast::List {
+            values: vec![v1, v2],
+        });
+        let rhs = Box::new(ast::Expr::List(list));
+        let op = bld.node(ast::In { lhs, rhs });
+
+        let pretty_printed = op.to_pretty_string(80).expect("pretty print");
+        println!("{pretty_printed}");
+
+        dbg!(&op);
+
+        #[derive(Default)]
+        pub struct IdVisitor {
+            ids: Vec<NodeId>,
+        }
+
+        impl Visitor<'_> for IdVisitor {
+            fn enter_ast_node(&mut self, id: NodeId) -> Traverse {
+                self.ids.push(id);
+                Traverse::Continue
+            }
+        }
+
+        let mut idv = IdVisitor::default();
+        op.visit(&mut idv);
+        let IdVisitor { ids } = idv;
+        dbg!(&ids);
+
+        for i in 0..ids.len() {
+            for j in i + 1..ids.len() {
+                assert_ne!(ids[i], ids[j]);
+            }
+        }
+    }
+}

partiql-common/src/node.rs

@@ -1,5 +1,5 @@
 use indexmap::IndexMap;
-use std::sync::{Arc, RwLock};
+use std::hash::Hash;
 
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
@@ -10,39 +10,30 @@ pub type NodeMap<T> = IndexMap<NodeId, T>;
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub struct NodeId(pub u32);
 
+#[derive(Debug)]
 /// Auto-incrementing [`NodeIdGenerator`]
 pub struct AutoNodeIdGenerator {
-    next_id: Arc<RwLock<NodeId>>,
+    next_id: NodeId,
 }
 
 impl Default for AutoNodeIdGenerator {
     fn default() -> Self {
-        AutoNodeIdGenerator {
-            next_id: Arc::new(RwLock::from(NodeId(1))),
-        }
+        AutoNodeIdGenerator { next_id: NodeId(1) }
     }
 }
 
 /// A provider of 'fresh' [`NodeId`]s.
 pub trait NodeIdGenerator {
-    fn id(&self) -> Arc<RwLock<NodeId>>;
-
     /// Provides a 'fresh' [`NodeId`].
-    fn next_id(&self) -> NodeId;
+    fn next_id(&mut self) -> NodeId;
 }
 
 impl NodeIdGenerator for AutoNodeIdGenerator {
-    fn id(&self) -> Arc<RwLock<NodeId>> {
-        let id = self.next_id();
-        let mut w = self.next_id.write().expect("NodId write lock");
-        *w = id;
-        Arc::clone(&self.next_id)
-    }
-
     #[inline]
-    fn next_id(&self) -> NodeId {
-        let id = &self.next_id.read().expect("NodId read lock");
-        NodeId(id.0 + 1)
+    fn next_id(&mut self) -> NodeId {
+        let mut next = NodeId(&self.next_id.0 + 1);
+        std::mem::swap(&mut self.next_id, &mut next);
+        next
     }
 }
 
@@ -51,11 +42,25 @@ impl NodeIdGenerator for AutoNodeIdGenerator {
 pub struct NullIdGenerator {}
 
 impl NodeIdGenerator for NullIdGenerator {
-    fn id(&self) -> Arc<RwLock<NodeId>> {
-        Arc::new(RwLock::from(self.next_id()))
+    fn next_id(&mut self) -> NodeId {
+        NodeId(0)
     }
+}
 
-    fn next_id(&self) -> NodeId {
-        NodeId(0)
+#[cfg(test)]
+mod tests {
+    use crate::node::{AutoNodeIdGenerator, NodeIdGenerator};
+
+    #[test]
+    fn unique_ids() {
+        let mut gen = AutoNodeIdGenerator::default();
+
+        let ids: Vec<_> = std::iter::repeat_with(|| gen.next_id()).take(15).collect();
+        dbg!(&ids);
+        for i in 0..ids.len() {
+            for j in i + 1..ids.len() {
+                assert_ne!(ids[i], ids[j]);
+            }
+        }
     }
 }

partiql-eval/src/eval/eval_expr_wrapper.rs

@@ -4,7 +4,7 @@ use crate::eval::expr::{BindError, EvalExpr};
 use crate::eval::EvalContext;
 use itertools::Itertools;
 
-use partiql_types::{type_dynamic, PartiqlShape, Static, TYPE_DYNAMIC};
+use partiql_types::{PartiqlShape, Static, TYPE_DYNAMIC};
 use partiql_value::Value::{Missing, Null};
 use partiql_value::{Tuple, Value};
 
@@ -469,7 +469,7 @@ impl UnaryValueExpr {
     where
         F: 'static + Fn(&Value) -> Value,
     {
-        Self::create_typed::<STRICT, F>([type_dynamic!(); 1], args, f)
+        Self::create_typed::<STRICT, F>([PartiqlShape::Dynamic; 1], args, f)
     }
 
     #[allow(dead_code)]

partiql-eval/src/eval/expr/coll.rs

@@ -5,7 +5,7 @@ use crate::eval::expr::{BindError, BindEvalExpr, EvalExpr};
 use itertools::{Itertools, Unique};
 
 use partiql_types::{
-    type_bool, type_numeric, ArrayType, BagType, PartiqlShape, PartiqlShapeBuilder, Static,
+    type_numeric, PartiqlNoIdShapeBuilder, PartiqlShape, ShapeBuilderExtensions, Static,
 };
 use partiql_value::Value::{Missing, Null};
 use partiql_value::{BinaryAnd, BinaryOr, Value, ValueIter};
@@ -51,46 +51,45 @@ impl BindEvalExpr for EvalCollFn {
                 value.sequence_iter().map_or(Missing, &f)
             })
         }
-        let boolean_elems = [PartiqlShapeBuilder::init_or_get().any_of([
-            PartiqlShapeBuilder::init_or_get()
-                .new_static(Static::Array(ArrayType::new(Box::new(type_bool!())))),
-            PartiqlShapeBuilder::init_or_get()
-                .new_static(Static::Bag(BagType::new(Box::new(type_bool!())))),
-        ])];
-        let numeric_elems = [PartiqlShapeBuilder::init_or_get().any_of([
-            PartiqlShapeBuilder::init_or_get().new_static(Static::Array(ArrayType::new(Box::new(
-                PartiqlShapeBuilder::init_or_get().any_of(type_numeric!()),
-            )))),
-            PartiqlShapeBuilder::init_or_get().new_static(Static::Bag(BagType::new(Box::new(
-                PartiqlShapeBuilder::init_or_get().any_of(type_numeric!()),
-            )))),
-        ])];
-        let any_elems = [PartiqlShapeBuilder::init_or_get().any_of([
-            PartiqlShapeBuilder::init_or_get().new_static(Static::Array(ArrayType::new_any())),
-            PartiqlShapeBuilder::init_or_get().new_static(Static::Bag(BagType::new_any())),
-        ])];
+
+        // use DummyShapeBuilder, as we don't care about shape Ids for evaluation dispatch
+        let mut bld = PartiqlNoIdShapeBuilder::default();
+
+        let boolean_elems = [
+            bld.new_array_of_static(Static::Bool),
+            bld.new_bag_of_static(Static::Bool),
+        ]
+        .into_any_of(&mut bld);
+
+        let numeric_elems = [
+            type_numeric!(&mut bld).into_array(&mut bld),
+            type_numeric!(&mut bld).into_bag(&mut bld),
+        ]
+        .into_any_of(&mut bld);
+
+        let any_elems = [bld.new_array_of_dyn(), bld.new_bag_of_dyn()].into_any_of(&mut bld);
 
         match self {
             EvalCollFn::Count(setq) => {
-                create::<{ STRICT }, _>(any_elems, args, move |it| it.coll_count(setq))
+                create::<{ STRICT }, _>([any_elems], args, move |it| it.coll_count(setq))
             }
             EvalCollFn::Avg(setq) => {
-                create::<{ STRICT }, _>(numeric_elems, args, move |it| it.coll_avg(setq))
+                create::<{ STRICT }, _>([numeric_elems], args, move |it| it.coll_avg(setq))
             }
             EvalCollFn::Max(setq) => {
-                create::<{ STRICT }, _>(any_elems, args, move |it| it.coll_max(setq))
+                create::<{ STRICT }, _>([any_elems], args, move |it| it.coll_max(setq))
             }
             EvalCollFn::Min(setq) => {
-                create::<{ STRICT }, _>(any_elems, args, move |it| it.coll_min(setq))
+                create::<{ STRICT }, _>([any_elems], args, move |it| it.coll_min(setq))
             }
             EvalCollFn::Sum(setq) => {
-                create::<{ STRICT }, _>(numeric_elems, args, move |it| it.coll_sum(setq))
+                create::<{ STRICT }, _>([numeric_elems], args, move |it| it.coll_sum(setq))
             }
             EvalCollFn::Any(setq) => {
-                create::<{ STRICT }, _>(boolean_elems, args, move |it| it.coll_any(setq))
+                create::<{ STRICT }, _>([boolean_elems], args, move |it| it.coll_any(setq))
             }
             EvalCollFn::Every(setq) => {
-                create::<{ STRICT }, _>(boolean_elems, args, move |it| it.coll_every(setq))
+                create::<{ STRICT }, _>([boolean_elems], args, move |it| it.coll_every(setq))
             }
         }
     }

partiql-eval/src/eval/expr/datetime.rs

@@ -1,6 +1,6 @@
 use crate::eval::expr::{BindError, BindEvalExpr, EvalExpr};
 
-use partiql_types::type_datetime;
+use partiql_types::{type_datetime, PartiqlNoIdShapeBuilder};
 use partiql_value::Value::Missing;
 use partiql_value::{DateTime, Value};
 
@@ -41,14 +41,18 @@ impl BindEvalExpr for EvalExtractFn {
             let total = Duration::new(u64::from(second), nanosecond).as_nanos() as i128;
             Decimal::from_i128_with_scale(total, NANOSECOND_SCALE).into()
         }
+        // use DummyShapeBuilder, as we don't care about shape Ids for evaluation dispatch
+        let mut bld = PartiqlNoIdShapeBuilder::default();
 
         let create = |f: fn(&DateTime) -> Value| {
-            UnaryValueExpr::create_typed::<{ STRICT }, _>([type_datetime!()], args, move |value| {
-                match value {
+            UnaryValueExpr::create_typed::<{ STRICT }, _>(
+                [type_datetime!(bld)],
+                args,
+                move |value| match value {
                     Value::DateTime(dt) => f(dt.as_ref()),
                     _ => Missing,
-                }
-            })
+                },
+            )
         };
 
         match self {