remote joins: do not include the join field if it already exists (#402)

For remote joins, if join field already exists in the selection set, do
not request it again. Include the join field only if it is not in the
selection set.

A field selection might be expensive, so we do not want to duplicate it.

Fixes: https://hasurahq.atlassian.net/browse/V3API-296
V3_GIT_ORIGIN_REV_ID: d09e4298557ca098433b0758c15703295ccbfd39

v3/crates/engine/src/execute/ir/selection_set.rs

@@ -22,6 +22,7 @@ use crate::metadata::resolved;
 use crate::metadata::resolved::subgraph::{
     Qualified, QualifiedBaseType, QualifiedTypeName, QualifiedTypeReference,
 };
+use crate::metadata::resolved::types::FieldMapping;
 use crate::schema::types::TypeKind;
 use crate::schema::{
     types::{Annotation, OutputAnnotation, RootFieldAnnotation},
@@ -104,6 +105,23 @@ pub(crate) struct ResultSelectionSet<'s> {
     pub(crate) fields: IndexMap<String, FieldSelection<'s>>,
 }
 
+impl<'s> ResultSelectionSet<'s> {
+    /// Takes a 'FieldMapping' and returns the alias, if the field is found in
+    /// existing fields
+    pub(crate) fn contains(&self, other_field: &FieldMapping) -> Option<String> {
+        self.fields.iter().find_map(|(alias, field)| match field {
+            FieldSelection::Column { column, .. } => {
+                if *column == other_field.column {
+                    Some(alias.clone())
+                } else {
+                    None
+                }
+            }
+            _ => None,
+        })
+    }
+}
+
 fn build_global_id_fields(
     global_id_fields: &Vec<FieldName>,
     field_mappings: &BTreeMap<FieldName, resolved::types::FieldMapping>,

v3/crates/engine/src/execute/plan/selection_set.rs

@@ -13,6 +13,7 @@ use crate::execute::remote_joins::types::{
     JoinLocations, JoinNode, LocationKind, MonotonicCounter, RemoteJoinType, TargetField,
 };
 use crate::execute::remote_joins::types::{Location, RemoteJoin};
+use crate::metadata::resolved::types::FieldMapping;
 
 pub(crate) fn process_nested_selection<'s, 'ir>(
     nested_selection: &'ir NestedSelection<'s>,
@@ -154,24 +155,18 @@ pub(crate) fn process_selection_set_ir<'s, 'ir>(
                 ir,
                 relationship_info,
             } => {
-                // For all the left join fields, create an alias and inject
-                // them into the NDC IR
                 let mut join_mapping = HashMap::new();
                 for ((src_field_alias, src_field), target_field) in &relationship_info.join_mapping
                 {
-                    let lhs_alias = make_hasura_phantom_field(&src_field.column);
+                    let ndc_field_alias = process_remote_relationship_field_mapping(
-
+                        model_selection,
-                    ndc_fields.insert(
+                        src_field,
-                        lhs_alias.clone(),
+                        &mut ndc_fields,
-                        ndc::models::Field::Column {
-                            column: src_field.column.clone(),
-                            fields: None,
-                        },
                     );
                     join_mapping.insert(
                         src_field_alias.clone(),
                         (
-                            lhs_alias.clone(),
+                            ndc_field_alias,
                             TargetField::ModelField(target_field.clone()),
                         ),
                     );
@@ -197,24 +192,18 @@ pub(crate) fn process_selection_set_ir<'s, 'ir>(
                 ir,
                 relationship_info,
             } => {
-                // For all the left join fields, create an alias and inject
-                // them into the NDC IR
                 let mut join_mapping = HashMap::new();
                 for ((src_field_alias, src_field), target_field) in &relationship_info.join_mapping
                 {
-                    let lhs_alias = make_hasura_phantom_field(&src_field.column);
+                    let ndc_field_alias = process_remote_relationship_field_mapping(
-
+                        model_selection,
-                    ndc_fields.insert(
+                        src_field,
-                        lhs_alias.clone(),
+                        &mut ndc_fields,
-                        ndc::models::Field::Column {
-                            column: src_field.column.clone(),
-                            fields: None,
-                        },
                     );
                     join_mapping.insert(
                         src_field_alias.clone(),
                         (
-                            lhs_alias.clone(),
+                            ndc_field_alias,
                             TargetField::CommandField(target_field.clone()),
                         ),
                     );
@@ -244,6 +233,34 @@ pub(crate) fn process_selection_set_ir<'s, 'ir>(
     Ok((ndc_fields, join_locations))
 }
 
+/// Processes a remote relationship field mapping, and returns the alias used in
+/// the NDC IR for that field
+///
+/// - if the selection set DOES NOT contain the field, insert it into the NDC IR
+/// (with an internal alias), and return the alias
+/// - if the selection set already contains the field, do not insert the field
+/// in NDC IR, and return the existing alias
+fn process_remote_relationship_field_mapping(
+    selection: &ResultSelectionSet<'_>,
+    field: &FieldMapping,
+    ndc_fields: &mut IndexMap<String, ndc::models::Field>,
+) -> String {
+    match selection.contains(field) {
+        None => {
+            let internal_alias = make_hasura_phantom_field(&field.column);
+            ndc_fields.insert(
+                internal_alias.clone(),
+                ndc::models::Field::Column {
+                    column: field.column.clone(),
+                    fields: None,
+                },
+            );
+            internal_alias
+        }
+        Some(field_alias) => field_alias,
+    }
+}
+
 fn make_hasura_phantom_field(field_name: &str) -> String {
     format!("__hasura_phantom_field__{}", field_name)
 }

v3/crates/engine/tests/explain.rs

@@ -4,7 +4,7 @@ mod common;
 #[test]
 fn test_explain_introspection() {
     common::test_execute_explain(
-        "explain/introspection_query/",
+        "explain/introspection_query",
         "explain/introspection_query/metadata.json",
         &[],
     );
@@ -13,7 +13,7 @@ fn test_explain_introspection() {
 #[test]
 fn test_multi_root_field_queries() {
     common::test_execute_explain(
-        "explain/multi_root_field_queries/",
+        "explain/multi_root_field_queries",
         "execute/multiple_root_fields/successful_execution/metadata.json",
         &[],
     );
@@ -22,7 +22,7 @@ fn test_multi_root_field_queries() {
 #[test]
 fn test_field_with_remote_relationship() {
     common::test_execute_explain(
-        "explain/field_with_remote_relationship/",
+        "explain/field_with_remote_relationship",
         "execute/remote_relationships/array/metadata.json",
         &["execute/common_metadata/two_postgres_connector_schema.json"],
     );
@@ -31,7 +31,7 @@ fn test_field_with_remote_relationship() {
 #[test]
 fn test_field_with_local_relationship() {
     common::test_execute_explain(
-        "explain/field_with_local_relationship/",
+        "explain/field_with_local_relationship",
         "execute/relationships/array/metadata.json",
         &["execute/common_metadata/postgres_connector_schema.json"],
     );
@@ -40,7 +40,7 @@ fn test_field_with_local_relationship() {
 #[test]
 fn test_field_with_multi_remote_relationship_subfields() {
     common::test_execute_explain(
-        "explain/field_with_multi_remote_relationship_subfields/",
+        "explain/field_with_multi_remote_relationship_subfields",
         "explain/field_with_multi_remote_relationship_subfields/metadata.json",
         &["execute/common_metadata/two_postgres_connector_schema.json"],
     );
@@ -49,7 +49,7 @@ fn test_field_with_multi_remote_relationship_subfields() {
 #[test]
 fn test_field_with_nested_remote_relationship_1() {
     common::test_execute_explain(
-        "explain/field_with_nested_remote_relationship_1/",
+        "explain/field_with_nested_remote_relationship_1",
         "explain/field_with_nested_remote_relationship_1/metadata.json",
         &["execute/common_metadata/two_postgres_connector_schema.json"],
     );
@@ -58,7 +58,7 @@ fn test_field_with_nested_remote_relationship_1() {
 #[test]
 fn test_field_with_nested_remote_relationship_2() {
     common::test_execute_explain(
-        "explain/field_with_nested_remote_relationship_2/",
+        "explain/field_with_nested_remote_relationship_2",
         "explain/field_with_nested_remote_relationship_2/metadata.json",
         &["execute/common_metadata/two_postgres_connector_schema.json"],
     );

v3/crates/engine/tests/explain/field_with_multi_remote_relationship_subfields/expected.json

@@ -17,11 +17,6 @@
                     "column": "ArtistId",
                     "fields": null
                   },
-                  "__hasura_phantom_field__ArtistId": {
-                    "type": "column",
-                    "column": "ArtistId",
-                    "fields": null
-                  },
                   "__hasura_phantom_field__AlbumId": {
                     "type": "column",
                     "column": "AlbumId",

v3/crates/engine/tests/explain/field_with_nested_remote_relationship_1/expected.json

@@ -22,11 +22,6 @@
                     "column": "ArtistId",
                     "fields": null
                   },
-                  "__hasura_phantom_field__ArtistId": {
-                    "type": "column",
-                    "column": "ArtistId",
-                    "fields": null
-                  },
                   "Tracks": {
                     "type": "relationship",
                     "query": {

v3/crates/engine/tests/explain/field_with_nested_remote_relationship_2/expected.json

@@ -21,16 +21,6 @@
                     "type": "column",
                     "column": "ArtistId",
                     "fields": null
-                  },
-                  "__hasura_phantom_field__AlbumId": {
-                    "type": "column",
-                    "column": "AlbumId",
-                    "fields": null
-                  },
-                  "__hasura_phantom_field__ArtistId": {
-                    "type": "column",
-                    "column": "ArtistId",
-                    "fields": null
                   }
                 }
               },

v3/crates/engine/tests/explain/field_with_remote_relationship/expected.json

@@ -21,11 +21,6 @@
                     "type": "column",
                     "column": "first_name",
                     "fields": null
-                  },
-                  "__hasura_phantom_field__id": {
-                    "type": "column",
-                    "column": "id",
-                    "fields": null
                   }
                 }
               },