fix nested fields relationships (#564)

### What

Selecting nested field relationships had a bug where it would try to
look up a composite type field column in the parent collection. For
example:

I have a collection `institution` with a field `staff` that is a
composite type that contains the field `favorite_artist_id`. Trying to
have a relationship from this field to the `Artist` table would attempt
to look-up this field in `institution`, instead of in `staff`.

This PR fixes this.

### How

Before, we had a type called `TableNameAndReference` that describe the
metadata name of a table and held a (relationship alias) reference to
it.

- We modify this type to contain a `Source` instead of a `Name`, which
can be a collection name or a nested field name. This is done so it is
clearer whether we are tracking a collection or a type.
- Instead of looking up a `Collection` everywhere, we lookup /anything/
that can have fields by replacing `lookup_collection` with
`lookup_fields_info`.
- When translating nested fields, instead of passing the `joins`
accumulator for the parent table, we create another on the fly, and
translate the joins using the table created for the nested field instead
of the parent table.

We also:

- Create nicer aliases for nested fields so they are easier to track
- Add a test that is based on sql tables from the `v3-engine`, so we add
a new `sql` file and update the configurations accordingly.

Author: Gil Mizrahi

changelog.md

@@ -24,6 +24,8 @@
 
 - Allow Native Operations that end with a semicolon when it's easy to remove them.
   [#566](https://github.com/hasura/ndc-postgres/pull/566)
+- Fix nested field relationships.
+  [#564](https://github.com/hasura/ndc-postgres/pull/564)
 
 ## [v1.0.1]
 

crates/query-engine/sql/src/sql/ast.rs

@@ -139,7 +139,7 @@ pub enum From {
     Unnest {
         expression: Expression,
         alias: TableAlias,
-        column: ColumnAlias,
+        columns: Vec<ColumnAlias>,
     },
     GenerateSeries {
         from: usize,
@@ -306,7 +306,7 @@ pub enum Expression {
 }
 
 /// Represents the name of a field in a nested object.
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct NestedField(pub String);
 
 /// An unary operator
@@ -409,6 +409,11 @@ pub enum TableReference {
     },
     /// refers to an alias we created
     AliasedTable(TableAlias),
+    /// refers to a nested field
+    NestedField {
+        source: Box<TableReference>,
+        field: NestedField,
+    },
 }
 
 /// A database table's column name

crates/query-engine/sql/src/sql/convert.rs

@@ -315,17 +315,26 @@ impl From {
             From::Unnest {
                 expression,
                 alias,
-                column,
+                columns,
             } => {
                 sql.append_syntax("UNNEST");
                 sql.append_syntax("(");
                 expression.to_sql(sql);
                 sql.append_syntax(")");
                 sql.append_syntax(" AS ");
                 alias.to_sql(sql);
-                sql.append_syntax("(");
-                column.to_sql(sql);
-                sql.append_syntax(")");
+                if !columns.is_empty() {
+                    sql.append_syntax("(");
+
+                    for (index, column) in columns.iter().enumerate() {
+                        column.to_sql(sql);
+                        if index < (columns.len() - 1) {
+                            sql.append_syntax(", ");
+                        }
+                    }
+
+                    sql.append_syntax(")");
+                }
             }
             From::GenerateSeries { from, to } => {
                 sql.append_syntax("generate_series");
@@ -701,6 +710,13 @@ impl TableReference {
                 table.to_sql(sql);
             }
             TableReference::AliasedTable(alias) => alias.to_sql(sql),
+            TableReference::NestedField { source, field } => {
+                sql.append_syntax("(");
+                source.to_sql(sql);
+                sql.append_syntax(")");
+                sql.append_syntax(".");
+                field.to_sql(sql);
+            }
         };
     }
 }

crates/query-engine/translation/src/translation/helpers.rs

@@ -53,22 +53,50 @@ pub struct NativeQueryInfo {
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct RootAndCurrentTables {
     /// The root (top-most) table in the query.
-    pub root_table: TableNameAndReference,
+    pub root_table: TableSourceAndReference,
     /// The current table we are processing.
-    pub current_table: TableNameAndReference,
+    pub current_table: TableSourceAndReference,
 }
 
 /// For a table in the query, We'd like to track what is its reference in the query
-/// (the name we can use to address them, an alias we generate), and what is their name in the
+/// (the name we can use to address them, an alias we generate), and what is their source in the
 /// metadata (so we can get their information such as which columns are available for that table).
 #[derive(Debug, Clone, PartialEq, Eq)]
-pub struct TableNameAndReference {
+pub struct TableSourceAndReference {
     /// Table name for column lookup
-    pub name: models::CollectionName,
+    pub source: TableSource,
     /// Table alias to query from
     pub reference: sql::ast::TableReference,
 }
 
+/// How to find the relevant information about a table.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum TableSource {
+    /// Using the collection name.
+    Collection(models::CollectionName),
+    /// Using the nested field path.
+    NestedField {
+        type_name: models::TypeName,
+        // These are used to create a nice table alias.
+        collection_name: models::CollectionName,
+        field_path: FieldPath,
+    },
+}
+
+impl TableSource {
+    /// Generate a nice name that can be used to give a table alias for this source.
+    pub fn name_for_alias(&self) -> String {
+        match self {
+            TableSource::Collection(collection_name) => collection_name.to_string(),
+            TableSource::NestedField {
+                collection_name,
+                field_path,
+                type_name: _,
+            } => format!("{collection_name}.{}", field_path.0.join(".")),
+        }
+    }
+}
+
 #[derive(Debug)]
 /// Information about columns
 pub struct ColumnInfo {
@@ -108,11 +136,11 @@ pub enum CompositeTypeInfo<'env> {
 /// Metadata information about any object that can have fields
 pub enum FieldsInfo<'env> {
     Table {
-        name: &'env models::CollectionName,
+        name: models::CollectionName,
         info: &'env metadata::TableInfo,
     },
     NativeQuery {
-        name: &'env models::CollectionName,
+        name: models::CollectionName,
         info: &'env metadata::NativeQueryInfo,
     },
     CompositeType {
@@ -124,7 +152,10 @@ pub enum FieldsInfo<'env> {
 impl<'a> From<&'a CompositeTypeInfo<'a>> for FieldsInfo<'a> {
     fn from(value: &'a CompositeTypeInfo<'a>) -> Self {
         match value {
-            CompositeTypeInfo::Table { name, info } => FieldsInfo::Table { name, info },
+            CompositeTypeInfo::Table { name, info } => FieldsInfo::Table {
+                name: name.clone(),
+                info,
+            },
             CompositeTypeInfo::CompositeType { name, info } => FieldsInfo::CompositeType {
                 name: name.clone(),
                 info,
@@ -136,8 +167,14 @@ impl<'a> From<&'a CompositeTypeInfo<'a>> for FieldsInfo<'a> {
 impl<'a> From<&'a CollectionInfo<'a>> for FieldsInfo<'a> {
     fn from(value: &'a CollectionInfo<'a>) -> Self {
         match value {
-            CollectionInfo::Table { name, info } => FieldsInfo::Table { name, info },
-            CollectionInfo::NativeQuery { name, info } => FieldsInfo::NativeQuery { name, info },
+            CollectionInfo::Table { name, info } => FieldsInfo::Table {
+                name: (*name).clone(),
+                info,
+            },
+            CollectionInfo::NativeQuery { name, info } => FieldsInfo::NativeQuery {
+                name: (*name).clone(),
+                info,
+            },
         }
     }
 }
@@ -182,55 +219,63 @@ impl<'request> Env<'request> {
     /// Queries, and Composite Types.
     ///
     /// This is used to translate field selection, where any of these may occur.
-    pub fn lookup_fields_info(
-        &self,
-        type_name: &'request models::CollectionName,
-    ) -> Result<FieldsInfo<'request>, Error> {
-        // Lookup the fields of a type name in a specific order:
-        // tables, then composite types, then native queries.
-        let info = self
-            .metadata
-            .tables
-            .0
-            .get(type_name)
-            .map(|t| FieldsInfo::Table {
-                name: type_name,
-                info: t,
-            })
-            .or_else(|| {
-                self.metadata
+    pub fn lookup_fields_info(&self, source: &TableSource) -> Result<FieldsInfo<'request>, Error> {
+        match source {
+            TableSource::NestedField {
+                collection_name: _,
+                type_name,
+                field_path: _,
+            } => {
+                let info = self
+                    .metadata
                     .composite_types
                     .0
                     .get(type_name.as_str())
                     .map(|t| FieldsInfo::CompositeType {
                         name: t.type_name.clone().into(),
                         info: t,
-                    })
-            })
-            .or_else(|| {
-                self.metadata
-                    .native_operations
-                    .queries
+                    });
+
+                info.ok_or(Error::ScalarTypeNotFound(type_name.as_str().into()))
+            }
+            TableSource::Collection(collection_name) => {
+                // Lookup the fields of a type name in a specific order:
+                // tables, then composite types, then native queries.
+                let info = self
+                    .metadata
+                    .tables
                     .0
-                    .get(type_name)
-                    .map(|nq| FieldsInfo::NativeQuery {
-                        name: type_name,
-                        info: nq,
+                    .get(collection_name)
+                    .map(|t| FieldsInfo::Table {
+                        name: collection_name.clone(),
+                        info: t,
                     })
-            })
-            .or_else(|| {
-                self.metadata
-                    .native_operations
-                    .mutations
-                    .0
-                    .get(type_name.as_str())
-                    .map(|nq| FieldsInfo::NativeQuery {
-                        name: type_name,
-                        info: nq,
+                    .or_else(|| {
+                        self.metadata
+                            .native_operations
+                            .queries
+                            .0
+                            .get(collection_name)
+                            .map(|nq| FieldsInfo::NativeQuery {
+                                name: collection_name.clone(),
+                                info: nq,
+                            })
                     })
-            });
-
-        info.ok_or(Error::CollectionNotFound(type_name.as_str().into()))
+                    .or_else(|| {
+                        self.metadata
+                            .native_operations
+                            .mutations
+                            .0
+                            .get(collection_name.as_str())
+                            .map(|nq| FieldsInfo::NativeQuery {
+                                name: collection_name.clone(),
+                                info: nq,
+                            })
+                    });
+
+                info.ok_or(Error::CollectionNotFound(collection_name.as_str().into()))
+            }
+        }
     }
 
     /// Lookup a metadata object which can be described by a Composite Type. This can be any of
@@ -265,7 +310,7 @@ impl<'request> Env<'request> {
                 })
             });
 
-        info.ok_or(Error::CollectionNotFound(type_name.as_str().into()))
+        info.ok_or(Error::ScalarTypeNotFound(type_name.as_str().into()))
     }
 
     /// Lookup a collection's information in the metadata.
@@ -591,7 +636,7 @@ impl NativeQueries {
 }
 
 /// A newtype wrapper around an ndc-spec type which represents accessing a nested field.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub struct FieldPath(pub Vec<models::FieldName>);
 
 impl From<&Option<Vec<models::FieldName>>> for FieldPath {

crates/query-engine/translation/src/translation/mutation/v2/delete.rs

@@ -1,7 +1,7 @@
 //! Auto-generate delete mutations and translate them into sql ast.
 
 use crate::translation::error::Error;
-use crate::translation::helpers::{self, TableNameAndReference};
+use crate::translation::helpers::{self, TableSourceAndReference};
 use crate::translation::query::filtering;
 use crate::translation::query::values;
 use ndc_models as models;
@@ -104,8 +104,8 @@ pub fn translate(
 
             let table_alias = state.make_table_alias(table_name.0.clone());
 
-            let table_name_and_reference = TableNameAndReference {
-                name: collection_name.clone(),
+            let table_name_and_reference = TableSourceAndReference {
+                source: helpers::TableSource::Collection(collection_name.clone()),
                 reference: sql::ast::TableReference::AliasedTable(table_alias.clone()),
             };
 

crates/query-engine/translation/src/translation/mutation/v2/insert.rs

@@ -1,7 +1,7 @@
 //! Auto-generate insert mutations and translate them into sql ast.
 
 use crate::translation::error::Error;
-use crate::translation::helpers::{self, TableNameAndReference};
+use crate::translation::helpers::{self, TableSourceAndReference};
 use crate::translation::mutation::check_columns;
 use crate::translation::query::filtering;
 use crate::translation::query::values;
@@ -215,8 +215,8 @@ pub fn translate(
 
     let (columns, from) = translate_objects_to_columns_and_values(env, state, mutation, object)?;
 
-    let table_name_and_reference = TableNameAndReference {
-        name: mutation.collection_name.clone(),
+    let table_name_and_reference = TableSourceAndReference {
+        source: helpers::TableSource::Collection(mutation.collection_name.clone()),
         reference: sql::ast::TableReference::DBTable {
             schema: mutation.schema_name.clone(),
             table: mutation.table_name.clone(),

crates/query-engine/translation/src/translation/mutation/v2/update.rs

@@ -1,7 +1,7 @@
 //! Auto-generate update mutations and translate them into sql ast.
 
 use crate::translation::error::Error;
-use crate::translation::helpers::{self, TableNameAndReference};
+use crate::translation::helpers::{self, TableSourceAndReference};
 use crate::translation::mutation::check_columns;
 use crate::translation::query::filtering;
 use crate::translation::query::values;
@@ -121,8 +121,8 @@ pub fn translate(
 
             let set = parse_update_columns(env, state, mutation, object)?;
 
-            let table_name_and_reference = TableNameAndReference {
-                name: mutation.collection_name.clone(),
+            let table_name_and_reference = TableSourceAndReference {
+                source: helpers::TableSource::Collection(mutation.collection_name.clone()),
                 reference: sql::ast::TableReference::DBTable {
                     schema: mutation.schema_name.clone(),
                     table: mutation.table_name.clone(),