Move NodeId Generation to a separate crate (#484)

Moves the `NodeId` and its Id generation to a new crate, `partiql-common` so that it can get consumed by multiple crates.

---------

Co-authored-by: Josh Pschorr <[email protected]>

Cargo.toml

@@ -16,7 +16,7 @@ members = [
     "partiql-catalog",
     "partiql-conformance-tests",
     "partiql-conformance-test-generator",
-    "partiql-source-map",
+    "partiql-common",
     "partiql-logical-planner",
     "partiql-logical",
     "partiql-eval",

partiql-ast-passes/Cargo.toml

@@ -22,6 +22,7 @@ bench = false
 [dependencies]
 partiql-ast = { path = "../partiql-ast", version = "0.10.*" }
 partiql-catalog = { path = "../partiql-catalog", version = "0.10.*" }
+partiql-common = { path = "../partiql-common", version = "0.10.*" }
 partiql-types = { path = "../partiql-types", version = "0.10.*" }
 
 assert_matches = "1.5.*"

partiql-ast-passes/src/name_resolver.rs

@@ -5,6 +5,7 @@ use partiql_ast::ast;
 use partiql_ast::ast::{GroupByExpr, GroupKey};
 use partiql_ast::visit::{Traverse, Visit, Visitor};
 use partiql_catalog::Catalog;
+use partiql_common::node::NodeId;
 use std::sync::atomic::{AtomicU32, Ordering};
 
 type FnvIndexSet<T> = IndexSet<T, FnvBuildHasher>;
@@ -40,9 +41,9 @@ pub struct KeySchema {
 
 #[derive(Default, Debug, Clone)]
 pub struct KeyRegistry {
-    pub in_scope: FnvIndexMap<ast::NodeId, Vec<ast::NodeId>>,
-    pub schema: FnvIndexMap<ast::NodeId, KeySchema>,
-    pub aliases: FnvIndexMap<ast::NodeId, Symbol>,
+    pub in_scope: FnvIndexMap<NodeId, Vec<NodeId>>,
+    pub schema: FnvIndexMap<NodeId, KeySchema>,
+    pub aliases: FnvIndexMap<NodeId, Symbol>,
 }
 
 #[derive(Debug)]
@@ -87,17 +88,17 @@ enum EnclosingClause {
 #[allow(dead_code)]
 pub struct NameResolver<'c> {
     // environment stack tracking
-    id_path_to_root: Vec<ast::NodeId>,
-    id_child_stack: Vec<Vec<ast::NodeId>>,
+    id_path_to_root: Vec<NodeId>,
+    id_child_stack: Vec<Vec<NodeId>>,
     keyref_stack: Vec<KeyRefs>,
-    lateral_stack: Vec<Vec<ast::NodeId>>,
+    lateral_stack: Vec<Vec<NodeId>>,
     id_gen: IdGenerator,
 
     // data flow tracking
-    enclosing_clause: FnvIndexMap<EnclosingClause, Vec<ast::NodeId>>,
-    in_scope: FnvIndexMap<ast::NodeId, Vec<ast::NodeId>>,
-    schema: FnvIndexMap<ast::NodeId, KeySchema>,
-    aliases: FnvIndexMap<ast::NodeId, Symbol>,
+    enclosing_clause: FnvIndexMap<EnclosingClause, Vec<NodeId>>,
+    in_scope: FnvIndexMap<NodeId, Vec<NodeId>>,
+    schema: FnvIndexMap<NodeId, KeySchema>,
+    aliases: FnvIndexMap<NodeId, Symbol>,
 
     // errors that occur during name resolution
     errors: Vec<AstTransformError>,
@@ -148,7 +149,7 @@ impl<'c> NameResolver<'c> {
     }
 
     #[inline]
-    fn current_node(&self) -> &ast::NodeId {
+    fn current_node(&self) -> &NodeId {
         self.id_path_to_root.last().unwrap()
     }
 
@@ -175,7 +176,7 @@ impl<'c> NameResolver<'c> {
     }
 
     #[inline]
-    fn exit_lateral(&mut self) -> Result<Vec<ast::NodeId>, AstTransformError> {
+    fn exit_lateral(&mut self) -> Result<Vec<NodeId>, AstTransformError> {
         self.lateral_stack.pop().ok_or_else(|| {
             AstTransformError::IllegalState("Expected non-empty lateral stack".to_string())
         })
@@ -187,7 +188,7 @@ impl<'c> NameResolver<'c> {
     }
 
     #[inline]
-    fn exit_child_stack(&mut self) -> Result<Vec<ast::NodeId>, AstTransformError> {
+    fn exit_child_stack(&mut self) -> Result<Vec<NodeId>, AstTransformError> {
         self.id_child_stack.pop().ok_or_else(|| {
             AstTransformError::IllegalState("Expected non-empty child stack".to_string())
         })
@@ -212,14 +213,14 @@ impl<'c> NameResolver<'c> {
 }
 
 impl<'ast, 'c> Visitor<'ast> for NameResolver<'c> {
-    fn enter_ast_node(&mut self, id: ast::NodeId) -> Traverse {
+    fn enter_ast_node(&mut self, id: NodeId) -> Traverse {
         self.id_path_to_root.push(id);
         if let Some(children) = self.id_child_stack.last_mut() {
             children.push(id);
         }
         Traverse::Continue
     }
-    fn exit_ast_node(&mut self, id: ast::NodeId) -> Traverse {
+    fn exit_ast_node(&mut self, id: NodeId) -> Traverse {
         assert_eq!(self.id_path_to_root.pop(), Some(id));
         Traverse::Continue
     }

partiql-ast/Cargo.toml

@@ -20,6 +20,7 @@ path = "src/lib.rs"
 bench = false
 
 [dependencies]
+partiql-common = { path = "../partiql-common", version = "0.10.*" }
 indexmap = "2.2"
 rust_decimal = { version = "1.25.0", default-features = false, features = ["std"] }
 serde = { version = "1.*", features = ["derive"], optional = true }
@@ -33,6 +34,7 @@ serde = [
     "rust_decimal/serde-with-str",
     "rust_decimal/serde",
     "indexmap/serde",
+    "partiql-common/serde"
 ]
 
 [dependencies.partiql-ast-macros]

partiql-ast/src/ast.rs

@@ -8,7 +8,6 @@
 // As more changes to this AST are expected, unless explicitly advised, using the structures exposed
 // in this crate directly is not recommended.
 
-use indexmap::IndexMap;
 use rust_decimal::Decimal as RustDecimal;
 
 use std::fmt;
@@ -17,12 +16,7 @@ use std::fmt;
 use serde::{Deserialize, Serialize};
 
 use partiql_ast_macros::Visit;
-
-pub type AstTypeMap<T> = IndexMap<NodeId, T>;
-
-#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
-#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
-pub struct NodeId(pub u32);
+use partiql_common::node::NodeId;
 
 /// Represents an AST node.
 #[derive(Clone, Debug, Eq, PartialEq)]

partiql-ast/src/builder.rs

@@ -1,53 +1,17 @@
-use crate::ast;
-use crate::ast::{AstNode, NodeId};
+use crate::ast::AstNode;
+use partiql_common::node::{AutoNodeIdGenerator, NodeIdGenerator, NullIdGenerator};
 
-/// A provider of 'fresh' [`NodeId`]s.
-pub trait IdGenerator {
-    /// Provides a 'fresh' [`NodeId`].
-    fn id(&mut self) -> NodeId;
-}
-
-/// Auto-incrementing [`IdGenerator`]
-pub struct AutoNodeIdGenerator {
-    next_id: ast::NodeId,
-}
-
-impl Default for AutoNodeIdGenerator {
-    fn default() -> Self {
-        AutoNodeIdGenerator { next_id: NodeId(1) }
-    }
-}
-
-impl IdGenerator for AutoNodeIdGenerator {
-    #[inline]
-    fn id(&mut self) -> NodeId {
-        let mut next = NodeId(&self.next_id.0 + 1);
-        std::mem::swap(&mut self.next_id, &mut next);
-        next
-    }
-}
-
-/// A provider of [`NodeId`]s that are always `0`; Useful for testing
-#[derive(Default)]
-pub struct NullIdGenerator {}
-
-impl IdGenerator for NullIdGenerator {
-    fn id(&mut self) -> NodeId {
-        NodeId(0)
-    }
-}
-
-/// A Builder for [`AstNode`]s that uses a [`IdGenerator`] to assign [`NodeId`]s
-pub struct NodeBuilder<Id: IdGenerator> {
+/// A Builder for [`AstNode`]s that uses a [`NodeIdGenerator`] to assign [`NodeId`]s
+pub struct AstNodeBuilder<IdGen: NodeIdGenerator> {
     /// Generator for 'fresh' [`NodeId`]s
-    pub id_gen: Id,
+    pub id_gen: IdGen,
 }
 
-impl<Id> NodeBuilder<Id>
+impl<IdGen> AstNodeBuilder<IdGen>
 where
-    Id: IdGenerator,
+    IdGen: NodeIdGenerator,
 {
-    pub fn new(id_gen: Id) -> Self {
+    pub fn new(id_gen: IdGen) -> Self {
         Self { id_gen }
     }
 
@@ -57,17 +21,17 @@ where
     }
 }
 
-impl<T> Default for NodeBuilder<T>
+impl<T> Default for AstNodeBuilder<T>
 where
-    T: IdGenerator + Default,
+    T: NodeIdGenerator + Default,
 {
     fn default() -> Self {
         Self::new(T::default())
     }
 }
 
-/// A [`NodeBuilder`] whose 'fresh' [`NodeId`]s are Auto-incrementing.
-pub type NodeBuilderWithAutoId = NodeBuilder<AutoNodeIdGenerator>;
+/// A [`AstNodeBuilder`] whose 'fresh' [`NodeId`]s are Auto-incrementing.
+pub type AstNodeBuilderWithAutoId = AstNodeBuilder<AutoNodeIdGenerator>;
 
-/// A [`NodeBuilder`] whose 'fresh' [`NodeId`]s are always `0`; Useful for testing
-pub type NodeBuilderWithNullId = NodeBuilder<NullIdGenerator>;
+/// A [`AstNodeBuilder`] whose 'fresh' [`NodeId`]s are always `0`; Useful for testing
+pub type AstNodeBuilderWithNullId = AstNodeBuilder<NullIdGenerator>;

partiql-ast/src/visit.rs

@@ -1,5 +1,5 @@
 use crate::ast;
-use crate::ast::NodeId;
+use partiql_common::node::NodeId;
 
 /// Indicates if tree traversal of the entire tree should continue or not.
 #[derive(PartialEq, Debug)]
@@ -561,7 +561,8 @@ pub trait Visitor<'ast> {
 mod tests {
     use crate::ast;
     use crate::visit::{Traverse, Visitor};
-    use ast::{AstNode, BinOp, BinOpKind, Expr, Lit, NodeId};
+    use ast::{AstNode, BinOp, BinOpKind, Expr, Lit};
+    use partiql_common::node::NodeId;
     use std::ops::AddAssign;
 
     #[test]

partiql-source-map/Cargo.toml → partiql-common/Cargo.toml

@@ -1,12 +1,12 @@
 [package]
-name = "partiql-source-map"
-description = "PartiQL Source Map"
+name = "partiql-common"
+description = "PartiQL Core"
 authors.workspace = true
 homepage.workspace = true
 repository.workspace = true
 license = "Apache-2.0"
 readme = "../README.md"
-keywords = ["sql", "sourcemap", "query", "compilers", "interpreters"]
+keywords = ["sql", "ast", "query", "compilers", "interpreters"]
 categories = ["database", "compilers"]
 exclude = [
     "**/.git/**",
@@ -16,18 +16,20 @@ version.workspace = true
 edition.workspace = true
 
 [lib]
+path = "src/lib.rs"
 bench = false
 
 [dependencies]
-partiql-ast = { path = "../partiql-ast", version = "0.10.*" }
-
-smallvec = { version = "1.*" }
+indexmap = "2.2"
+pretty = "0.12"
 serde = { version = "1.*", features = ["derive"], optional = true }
-
-
-[dev-dependencies]
-
+smallvec = { version = "1.*" }
+thiserror = "1.0"
 
 [features]
 default = []
-serde = ["dep:serde", "smallvec/serde"]
+serde = [
+    "dep:serde",
+    "indexmap/serde",
+    "smallvec/serde"
+]

partiql-common/src/lib.rs

@@ -0,0 +1,4 @@
+#![deny(rust_2018_idioms)]
+#![deny(clippy::all)]
+pub mod node;
+pub mod syntax;

partiql-common/src/node.rs

@@ -0,0 +1,47 @@
+use indexmap::IndexMap;
+use std::hash::Hash;
+
+#[cfg(feature = "serde")]
+use serde::{Deserialize, Serialize};
+
+pub type NodeMap<T> = IndexMap<NodeId, T>;
+
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub struct NodeId(pub u32);
+
+/// Auto-incrementing [`NodeIdGenerator`]
+pub struct AutoNodeIdGenerator {
+    next_id: NodeId,
+}
+
+impl Default for AutoNodeIdGenerator {
+    fn default() -> Self {
+        AutoNodeIdGenerator { next_id: NodeId(1) }
+    }
+}
+
+/// A provider of 'fresh' [`NodeId`]s.
+pub trait NodeIdGenerator {
+    /// Provides a 'fresh' [`NodeId`].
+    fn id(&mut self) -> NodeId;
+}
+
+impl NodeIdGenerator for AutoNodeIdGenerator {
+    #[inline]
+    fn id(&mut self) -> NodeId {
+        let mut next = NodeId(&self.next_id.0 + 1);
+        std::mem::swap(&mut self.next_id, &mut next);
+        next
+    }
+}
+
+/// A provider of [`NodeId`]s that are always `0`; Useful for testing
+#[derive(Default)]
+pub struct NullIdGenerator {}
+
+impl NodeIdGenerator for NullIdGenerator {
+    fn id(&mut self) -> NodeId {
+        NodeId(0)
+    }
+}

partiql-source-map/src/line_offset_tracker.rs → partiql-common/src/syntax/line_offset_tracker.rs

@@ -1,6 +1,6 @@
 //! [`LineOffsetTracker`] and related types for mapping locations in source `str`s.
 
-use crate::location::{ByteOffset, BytePosition, LineAndCharPosition, LineOffset};
+use crate::syntax::location::{ByteOffset, BytePosition, LineAndCharPosition, LineOffset};
 use smallvec::{smallvec, SmallVec};
 use std::ops::Range;
 
@@ -14,8 +14,8 @@ use serde::{Deserialize, Serialize};
 /// ## Example
 ///
 /// ```rust
-/// use partiql_source_map::location::{ByteOffset, LineAndCharPosition};
-/// use partiql_source_map::line_offset_tracker::{LineOffsetError, LineOffsetTracker};
+/// use partiql_common::syntax::location::{ByteOffset, LineAndCharPosition};
+/// use partiql_common::syntax::line_offset_tracker::{LineOffsetError, LineOffsetTracker};
 ///
 /// let source = "12345\n789012345\n789012345\n789012345";
 /// let mut tracker = LineOffsetTracker::default();