BE-187: Refactor SQL query compiler with Identifier types and improved JOIN handling (#7951)

Author: TimDiekmann

libs/@local/graph/postgres-store/src/store/postgres/query/compile.rs

@@ -5,7 +5,7 @@ use crate::store::postgres::query::{Expression, Transpile};
 /// A [`Filter`], which can be transpiled.
 ///
 /// [`Filter`]: hash_graph_store::filter::Filter
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub enum Condition {
     All(Vec<Self>),
     Any(Vec<Self>),

libs/@local/graph/postgres-store/src/store/postgres/query/condition.rs

@@ -0,0 +1,140 @@
+use core::fmt::{self, Write as _};
+
+use super::{identifier::Identifier, table_reference::TableReference};
+use crate::store::postgres::query::{Column, Transpile};
+
+/// A column name in a PostgreSQL query.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub enum ColumnName<'name> {
+    /// A dynamically-named column, quoted when transpiled
+    Named(Identifier<'name>),
+    /// A statically-defined column from the database schema
+    Static(Column),
+    /// The asterisk wildcard (`*`)
+    Asterisk,
+}
+
+/// A reference to a column, optionally qualified with a table reference.
+///
+/// Transpiles to `<table>.<column>` or just `<column>` if no table qualification is present.
+#[derive(Clone, PartialEq, Eq, Hash)]
+pub struct ColumnReference<'name> {
+    pub correlation: Option<TableReference<'name>>,
+    pub name: ColumnName<'name>,
+}
+
+impl fmt::Debug for ColumnReference<'_> {
+    fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.transpile(fmt)
+    }
+}
+
+impl Transpile for ColumnReference<'_> {
+    fn transpile(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
+        if let Some(correlation) = &self.correlation {
+            correlation.transpile(fmt)?;
+            fmt.write_char('.')?;
+        }
+
+        match &self.name {
+            ColumnName::Named(name) => name.transpile(fmt),
+            ColumnName::Static(column) => column.transpile(fmt),
+            ColumnName::Asterisk => fmt.write_char('*'),
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::store::postgres::query::expression::table_reference::{SchemaReference, TableName};
+
+    #[test]
+    fn simple_column_reference() {
+        let col_ref = ColumnReference {
+            correlation: None,
+            name: ColumnName::Named(Identifier::from("username")),
+        };
+        assert_eq!(col_ref.transpile_to_string(), r#""username""#);
+    }
+
+    #[test]
+    fn qualified_column_reference() {
+        let col_ref = ColumnReference {
+            correlation: Some(TableReference {
+                schema: None,
+                name: TableName::from("users"),
+                alias: None,
+            }),
+            name: ColumnName::Named(Identifier::from("username")),
+        };
+        assert_eq!(col_ref.transpile_to_string(), r#""users"."username""#);
+    }
+
+    #[test]
+    fn fully_qualified_column_reference() {
+        let col_ref = ColumnReference {
+            correlation: Some(TableReference {
+                schema: Some(SchemaReference {
+                    database: Some(Identifier::from("mydb")),
+                    name: Identifier::from("public"),
+                }),
+                name: TableName::from("users"),
+                alias: None,
+            }),
+            name: ColumnName::Named(Identifier::from("username")),
+        };
+        assert_eq!(
+            col_ref.transpile_to_string(),
+            r#""mydb"."public"."users"."username""#
+        );
+    }
+
+    #[test]
+    fn column_reference_with_special_chars() {
+        let col_ref = ColumnReference {
+            correlation: Some(TableReference {
+                schema: None,
+                name: TableName::from("user-table"),
+                alias: None,
+            }),
+            name: ColumnName::Named(Identifier::from("user name")),
+        };
+        assert_eq!(col_ref.transpile_to_string(), r#""user-table"."user name""#);
+    }
+
+    #[test]
+    fn column_reference_with_quotes() {
+        let col_ref = ColumnReference {
+            correlation: Some(TableReference {
+                schema: None,
+                name: TableName::from(r#"my"table"#),
+                alias: None,
+            }),
+            name: ColumnName::Named(Identifier::from(r#"my"column"#)),
+        };
+        assert_eq!(col_ref.transpile_to_string(), r#""my""table"."my""column""#);
+    }
+
+    #[test]
+    fn asterisk_column_reference() {
+        let col_ref = ColumnReference {
+            correlation: None,
+            name: ColumnName::Asterisk,
+        };
+        assert_eq!(col_ref.transpile_to_string(), "*");
+    }
+
+    #[test]
+    fn qualified_asterisk_column_reference() {
+        let col_ref = ColumnReference {
+            correlation: Some(TableReference {
+                schema: None,
+                name: TableName::from("users"),
+                alias: None,
+            }),
+            name: ColumnName::Asterisk,
+        };
+        assert_eq!(col_ref.transpile_to_string(), r#""users".*"#);
+    }
+}

libs/@local/graph/postgres-store/src/store/postgres/query/expression/column_reference.rs

@@ -4,12 +4,12 @@ use core::fmt::{
 
 use hash_graph_store::filter::PathToken;
 
+use super::ColumnReference;
 use crate::store::postgres::query::{
-    Alias, AliasedTable, Column, SelectStatement, Table, Transpile, WindowStatement,
-    table::DatabaseColumn as _,
+    Alias, Column, SelectStatement, Table, Transpile, WindowStatement, table::DatabaseColumn as _,
 };
 
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub enum Function {
     Min(Box<Expression>),
     Max(Box<Expression>),
@@ -157,11 +157,11 @@ impl Transpile for PostgresType {
 }
 
 /// A compiled expression in Postgres.
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub enum Expression {
     Asterisk,
-    Column(Column),
-    ColumnReference {
+    ColumnReference(ColumnReference<'static>),
+    AliasedColumn {
         column: Column,
         table_alias: Option<Alias>,
     },
@@ -186,26 +186,22 @@ impl Transpile for Expression {
     fn transpile(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
         match self {
             Self::Asterisk => fmt.write_char('*'),
-            Self::Column(column) => column.transpile(fmt),
-            Self::ColumnReference {
+            Self::ColumnReference(column) => column.transpile(fmt),
+            Self::AliasedColumn {
                 column,
                 table_alias,
             } => {
                 let table = column.table();
                 if let Some(alias) = *table_alias {
-                    AliasedTable { table, alias }.transpile(fmt)?;
+                    table.aliased(alias).transpile(fmt)?;
                 } else {
                     table.transpile(fmt)?;
                 }
                 write!(fmt, r#"."{}""#, column.as_str())
             }
             Self::TableReference { table, alias } => {
                 if let Some(alias) = *alias {
-                    AliasedTable {
-                        table: *table,
-                        alias,
-                    }
-                    .transpile(fmt)
+                    table.aliased(alias).transpile(fmt)
                 } else {
                     table.transpile(fmt)
                 }
@@ -271,7 +267,7 @@ mod tests {
     #[test]
     fn transpile_function_expression() {
         assert_eq!(
-            Expression::Function(Function::Min(Box::new(Expression::ColumnReference {
+            Expression::Function(Function::Min(Box::new(Expression::AliasedColumn {
                 column: DataTypeQueryPath::Version.terminating_column().0,
                 table_alias: Some(Alias {
                     condition_index: 1,

libs/@local/graph/postgres-store/src/store/postgres/query/expression/conditional.rs

@@ -2,7 +2,7 @@ use core::fmt;
 
 use crate::store::postgres::query::{Expression, Transpile};
 
-#[derive(Debug, Clone, Default, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, Default, PartialEq, Eq)]
 pub struct GroupByExpression {
     pub expressions: Vec<Expression>,
 }
@@ -36,15 +36,15 @@ mod tests {
     fn order_one() {
         let order_by_expression = GroupByExpression {
             expressions: vec![
-                Expression::ColumnReference {
+                Expression::AliasedColumn {
                     column: EntityQueryPath::WebId.terminating_column().0,
                     table_alias: Some(Alias {
                         condition_index: 1,
                         chain_depth: 2,
                         number: 3,
                     }),
                 },
-                Expression::ColumnReference {
+                Expression::AliasedColumn {
                     column: EntityQueryPath::Uuid.terminating_column().0,
                     table_alias: Some(Alias {
                         condition_index: 4,

libs/@local/graph/postgres-store/src/store/postgres/query/expression/group_by_clause.rs

@@ -0,0 +1,136 @@
+use alloc::borrow::Cow;
+use core::fmt::{self, Write as _};
+
+use crate::store::postgres::query::Transpile;
+
+/// A PostgreSQL identifier (table name, column name, alias, schema name, etc.).
+///
+/// Identifiers are **always** quoted using double quotes (`"`) when transpiled to SQL. This
+/// ensures:
+/// - Protection against SQL injection
+/// - Correct handling of SQL reserved keywords (e.g., `user`, `select`)
+/// - Support for identifiers with special characters
+/// - Preservation of case-sensitivity
+///
+/// Internal double quotes are escaped by doubling them (`""`) as per PostgreSQL's standard.
+#[derive(Clone, PartialEq, Eq, Hash)]
+pub struct Identifier<'name> {
+    name: Cow<'name, str>,
+}
+
+impl fmt::Debug for Identifier<'_> {
+    fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.transpile(fmt)
+    }
+}
+
+impl<'name> From<&'name str> for Identifier<'name> {
+    fn from(name: &'name str) -> Self {
+        Self {
+            name: Cow::Borrowed(name),
+        }
+    }
+}
+
+impl From<String> for Identifier<'_> {
+    fn from(identifier: String) -> Self {
+        Self {
+            name: Cow::Owned(identifier),
+        }
+    }
+}
+
+impl AsRef<str> for Identifier<'_> {
+    fn as_ref(&self) -> &str {
+        &self.name
+    }
+}
+
+impl Transpile for Identifier<'_> {
+    fn transpile(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
+        fmt.write_char('"')?;
+        for ch in self.name.chars() {
+            if ch == '"' {
+                fmt.write_str("\"\"")?;
+            } else {
+                fmt.write_char(ch)?;
+            }
+        }
+        fmt.write_char('"')
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn simple_identifier() {
+        let id = Identifier::from("users");
+        assert_eq!(id.transpile_to_string(), r#""users""#);
+    }
+
+    #[test]
+    fn keyword_identifier() {
+        let id = Identifier::from("select");
+        assert_eq!(id.transpile_to_string(), r#""select""#);
+
+        let id = Identifier::from("user");
+        assert_eq!(id.transpile_to_string(), r#""user""#);
+
+        let id = Identifier::from("table");
+        assert_eq!(id.transpile_to_string(), r#""table""#);
+    }
+
+    #[test]
+    fn identifier_with_special_chars() {
+        let id = Identifier::from("my-table");
+        assert_eq!(id.transpile_to_string(), r#""my-table""#);
+
+        let id = Identifier::from("table name");
+        assert_eq!(id.transpile_to_string(), r#""table name""#);
+
+        let id = Identifier::from("123abc");
+        assert_eq!(id.transpile_to_string(), r#""123abc""#);
+    }
+
+    #[test]
+    fn identifier_with_quotes() {
+        // Single internal quote: my"table → "my""table"
+        let id = Identifier::from(r#"my"table"#);
+        assert_eq!(id.transpile_to_string(), r#""my""table""#);
+
+        // Multiple internal quotes: a"b"c → "a""b""c"
+        let id = Identifier::from(r#"a"b"c"#);
+        assert_eq!(id.transpile_to_string(), r#""a""b""c""#);
+
+        // Just a quote: " → """"
+        let id = Identifier::from(r#"""#);
+        assert_eq!(id.transpile_to_string(), r#""""""#);
+    }
+
+    #[test]
+    fn case_sensitive_identifier() {
+        let id = Identifier::from("UsErS");
+        assert_eq!(id.transpile_to_string(), r#""UsErS""#);
+
+        let id = Identifier::from("USERS");
+        assert_eq!(id.transpile_to_string(), r#""USERS""#);
+    }
+
+    #[test]
+    fn empty_identifier() {
+        let id = Identifier::from("");
+        assert_eq!(id.transpile_to_string(), r#""""#);
+    }
+
+    #[test]
+    #[expect(clippy::non_ascii_literal)]
+    fn unicode_identifier() {
+        let id = Identifier::from("用户表"); // "user table" in Chinese
+        assert_eq!(id.transpile_to_string(), r#""用户表""#);
+
+        let id = Identifier::from("café");
+        assert_eq!(id.transpile_to_string(), r#""café""#);
+    }
+}