Implement leaf node merging (#269)

Introduces a new optimization step that merges leaf nodes in the fetch
graph to reduce the number of fetch steps, without degrading the query
performance. This is achieved by merging leaf nodes with other nodes if
they share the same service, response path, input type name, condition,
and no argument conflicts exist.

The snapshot differences are either fields in different order or steps
being merged (always less than in the previous snapshot).

Closes #268

Author: kamilkisiela

Cargo.lock

@@ -11,6 +11,7 @@ graphql-tools = "0.4.0"
 graphql-parser = "0.4.1"
 thiserror = "2.0.12"
 petgraph = "0.8.0"
+rustc-hash = "2.1.1"
 tracing = { version = "0.1.41" }
 serde = "1.0.219"
 serde_json = { version = "1.0.140", features = ["preserve_order"] }

lib/query-planner/Cargo.toml

@@ -1,5 +1,5 @@
-use std::collections::hash_map::DefaultHasher;
-use std::hash::{Hash, Hasher};
+use rustc_hash::{FxBuildHasher, FxHasher};
+use std::hash::{BuildHasher, Hash, Hasher};
 
 use crate::ast::arguments::ArgumentsMap;
 use crate::ast::operation::{OperationDefinition, VariableDefinition};
@@ -14,7 +14,7 @@ pub trait ASTHash {
 }
 
 pub fn ast_hash(query: &OperationDefinition) -> u64 {
-    let mut hasher = DefaultHasher::new();
+    let mut hasher = FxHasher::default();
     query.ast_hash(&mut hasher);
     hasher.finish()
 }
@@ -111,32 +111,38 @@ impl ASTHash for &InlineFragmentSelection {
 }
 
 impl ASTHash for ArgumentsMap {
-    fn ast_hash<H: Hasher>(&self, hasher: &mut H) {
-        // Order does not matter for hashing
-        // The order of arguments does not matter.
-        // To achieve order-insensitivity, we get all keys, sort them, and then
-        // hash them with their values in that order.
-        let mut keys: Vec<_> = self.keys().collect();
-        keys.sort_unstable();
-        for key in keys {
-            key.hash(hasher);
-            // We can unwrap here because we are iterating over existing keys
-            self.get_argument(key).unwrap().ast_hash(hasher);
+    fn ast_hash<H: Hasher>(&self, state: &mut H) {
+        let mut combined_hash: u64 = 0;
+        let build_hasher = FxBuildHasher;
+
+        // To achieve an order-independent hash, we hash each key-value pair
+        // individually and then combine their hashes using XOR (^).
+        // Since XOR is commutative, the final hash is not affected by the iteration order.
+        for (key, value) in self.into_iter() {
+            let mut key_val_hasher = build_hasher.build_hasher();
+            key.hash(&mut key_val_hasher);
+            value.ast_hash(&mut key_val_hasher);
+            combined_hash ^= key_val_hasher.finish();
         }
+
+        state.write_u64(combined_hash);
     }
 }
 
 impl ASTHash for Vec<VariableDefinition> {
     fn ast_hash<H: Hasher>(&self, hasher: &mut H) {
-        // Order does not matter for hashing
-        // The order of variables does not matter.
-        // We do a similar thing as for ArgumentsMap,
-        // we sort the variables by their names.
-        let mut variables: Vec<_> = self.iter().collect();
-        variables.sort_by_key(|f| &f.name);
-        for var in variables {
-            var.ast_hash(hasher);
+        let mut combined_hash: u64 = 0;
+        let build_hasher = FxBuildHasher;
+        // To achieve an order-independent hash, we hash each key-value pair
+        // individually and then combine their hashes using XOR (^).
+        // Since XOR is commutative, the final hash is not affected by the iteration order.
+        for variable in self.iter() {
+            let mut local_hasher = build_hasher.build_hasher();
+            variable.ast_hash(&mut local_hasher);
+            combined_hash ^= local_hasher.finish();
         }
+
+        hasher.write_u64(combined_hash);
     }
 }
 
@@ -196,3 +202,164 @@ impl ASTHash for Value {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::ast::arguments::ArgumentsMap;
+    use crate::ast::operation::{OperationDefinition, VariableDefinition};
+    use crate::ast::selection_item::SelectionItem;
+    use crate::ast::selection_set::{FieldSelection, SelectionSet};
+    use crate::ast::value::Value;
+    use crate::state::supergraph_state::{OperationKind, TypeNode};
+    use std::collections::BTreeMap;
+
+    fn create_test_operation() -> OperationDefinition {
+        let mut arguments = ArgumentsMap::new();
+        arguments.add_argument("limit".to_string(), Value::Int(10));
+        arguments.add_argument("sort".to_string(), Value::Enum("ASC".to_string()));
+
+        let mut nested_object = BTreeMap::new();
+        nested_object.insert(
+            "nestedKey".to_string(),
+            Value::String("nestedValue".to_string()),
+        );
+
+        arguments.add_argument("obj".to_string(), Value::Object(nested_object));
+
+        let field_selection = FieldSelection {
+            name: "users".to_string(),
+            alias: Some("all_users".to_string()),
+            selections: SelectionSet {
+                items: vec![
+                    SelectionItem::Field(FieldSelection {
+                        name: "id".to_string(),
+                        alias: None,
+                        selections: SelectionSet { items: vec![] },
+                        arguments: None,
+                        include_if: None,
+                        skip_if: None,
+                    }),
+                    SelectionItem::Field(FieldSelection {
+                        name: "name".to_string(),
+                        alias: None,
+                        selections: SelectionSet { items: vec![] },
+                        arguments: None,
+                        include_if: Some("includeName".to_string()),
+                        skip_if: None,
+                    }),
+                ],
+            },
+            arguments: Some(arguments),
+            include_if: None,
+            skip_if: Some("skipUsers".to_string()),
+        };
+
+        let selection_set = SelectionSet {
+            items: vec![SelectionItem::Field(field_selection)],
+        };
+
+        let variable_definitions = vec![
+            VariableDefinition {
+                name: "skipUsers".to_string(),
+                variable_type: TypeNode::NonNull(Box::new(TypeNode::Named("Boolean".to_string()))),
+                default_value: Some(Value::Boolean(false)),
+            },
+            VariableDefinition {
+                name: "includeName".to_string(),
+                variable_type: TypeNode::Named("Boolean".to_string()),
+                default_value: None,
+            },
+        ];
+
+        OperationDefinition {
+            operation_kind: Some(OperationKind::Query),
+            selection_set,
+            variable_definitions: Some(variable_definitions),
+            name: Some("TestQuery".to_string()),
+        }
+    }
+
+    #[test]
+    fn test_ast_hash_is_deterministic() {
+        let operation = create_test_operation();
+
+        let hash1 = ast_hash(&operation);
+        let hash2 = ast_hash(&operation);
+
+        // Test that the hash is consistent within the same run
+        assert_eq!(hash1, hash2, "AST hash should be consistent");
+
+        // Snapshot test: compare against a known, pre-calculated hash.
+        // If the hashing logic changes, this value will need to be updated.
+        let expected_hash = 8854078506550230644;
+        assert_eq!(
+            hash1, expected_hash,
+            "AST hash does not match the snapshot value. If this change is intentional, update the snapshot."
+        );
+    }
+
+    #[test]
+    fn test_order_independent_hashing_for_arguments() {
+        let mut args1 = ArgumentsMap::new();
+        args1.add_argument("a".to_string(), Value::Int(1));
+        args1.add_argument("b".to_string(), Value::Int(2));
+
+        let mut args2 = ArgumentsMap::new();
+        args2.add_argument("b".to_string(), Value::Int(2));
+        args2.add_argument("a".to_string(), Value::Int(1));
+
+        let mut hasher1 = FxHasher::default();
+        args1.ast_hash(&mut hasher1);
+
+        let mut hasher2 = FxHasher::default();
+        args2.ast_hash(&mut hasher2);
+
+        assert_eq!(
+            hasher1.finish(),
+            hasher2.finish(),
+            "ArgumentsMap hashing should be order-independent"
+        );
+    }
+
+    #[test]
+    fn test_order_independent_hashing_for_variables() {
+        let vars1 = vec![
+            VariableDefinition {
+                name: "varA".to_string(),
+                variable_type: TypeNode::Named("String".to_string()),
+                default_value: None,
+            },
+            VariableDefinition {
+                name: "varB".to_string(),
+                variable_type: TypeNode::Named("Int".to_string()),
+                default_value: Some(Value::Int(0)),
+            },
+        ];
+
+        let vars2 = vec![
+            VariableDefinition {
+                name: "varB".to_string(),
+                variable_type: TypeNode::Named("Int".to_string()),
+                default_value: Some(Value::Int(0)),
+            },
+            VariableDefinition {
+                name: "varA".to_string(),
+                variable_type: TypeNode::Named("String".to_string()),
+                default_value: None,
+            },
+        ];
+
+        let mut hasher1 = FxHasher::default();
+        vars1.ast_hash(&mut hasher1);
+
+        let mut hasher2 = FxHasher::default();
+        vars2.ast_hash(&mut hasher2);
+
+        assert_eq!(
+            hasher1.finish(),
+            hasher2.finish(),
+            "VariableDefinition vector hashing should be order-independent"
+        );
+    }
+}

lib/query-planner/src/ast/hash.rs

@@ -12,6 +12,7 @@ use crate::planner::tree::query_tree_node::{MutationFieldPosition, QueryTreeNode
 use crate::planner::walker::path::OperationPath;
 use crate::planner::walker::pathfinder::can_satisfy_edge;
 use crate::state::supergraph_state::{SubgraphName, SupergraphState};
+use petgraph::algo::has_path_connecting;
 use petgraph::graph::EdgeReference;
 use petgraph::stable_graph::{EdgeIndex, NodeIndex, NodeIndices, NodeReferences, StableDiGraph};
 use petgraph::visit::EdgeRef;
@@ -58,6 +59,10 @@ impl FetchGraph {
         self.graph.edges_directed(index, Direction::Outgoing)
     }
 
+    pub fn is_descendant_of(&self, descendant: NodeIndex, ancestor: NodeIndex) -> bool {
+        has_path_connecting(&self.graph, ancestor, descendant, None)
+    }
+
     pub fn step_indices(&self) -> NodeIndices<FetchStepData> {
         self.graph.node_indices()
     }

lib/query-planner/src/planner/fetch/fetch_graph.rs

@@ -0,0 +1,108 @@
+use petgraph::graph::NodeIndex;
+use tracing::{instrument, trace};
+
+use crate::{
+    ast::type_aware_selection::find_arguments_conflicts,
+    planner::fetch::{
+        error::FetchGraphError, fetch_graph::FetchGraph, fetch_step_data::FetchStepData,
+        optimize::utils::perform_fetch_step_merge,
+    },
+};
+
+impl FetchStepData {
+    pub fn can_merge_leafs(
+        &self,
+        self_index: NodeIndex,
+        other_index: NodeIndex,
+        other: &Self,
+        fetch_graph: &FetchGraph,
+    ) -> bool {
+        if self_index == other_index {
+            return false;
+        }
+
+        if self.service_name != other.service_name {
+            return false;
+        }
+
+        if self.response_path != other.response_path {
+            return false;
+        }
+
+        if self.input.type_name != other.input.type_name {
+            return false;
+        }
+
+        if self.condition != other.condition {
+            return false;
+        }
+
+        // otherwise we break the order of mutations
+        if self.mutation_field_position != other.mutation_field_position {
+            return false;
+        }
+
+        // `other` must be a leaf node (no children).
+        if fetch_graph.children_of(other_index).count() != 0 {
+            return false;
+        }
+
+        // We can't merge if one is a descendant of the other,
+        // because there's a dependency between them,
+        // that could lead to incorrect results.
+        // Either input of "other" depends on the output of "self",
+        // or input of "other" depends on the output of one of the steps in between.
+        if fetch_graph.is_descendant_of(other_index, self_index) {
+            return false;
+        }
+
+        let input_conflicts = find_arguments_conflicts(&self.input, &other.input);
+
+        if !input_conflicts.is_empty() {
+            return false;
+        }
+
+        true
+    }
+}
+
+impl FetchGraph {
+    #[instrument(level = "trace", skip_all)]
+    /// This optimization is about merging leaf nodes in the fetch nodes with other nodes.
+    /// It reduces the number of fetch steps, without degrading the query performance.
+    /// The query performance is not degraded, because the leaf node has no children,
+    /// meaning the overall depth (amount of parallel layers) is not increased.
+    pub(crate) fn merge_leafs(&mut self) -> Result<(), FetchGraphError> {
+        while let Some((target_idx, leaf_idx)) = self.find_merge_candidate()? {
+            perform_fetch_step_merge(target_idx, leaf_idx, self)?;
+        }
+
+        Ok(())
+    }
+
+    fn find_merge_candidate(&self) -> Result<Option<(NodeIndex, NodeIndex)>, FetchGraphError> {
+        let all_nodes: Vec<NodeIndex> = self
+            .graph
+            .node_indices()
+            .filter(|&idx| self.root_index != Some(idx))
+            .collect();
+
+        for &target_idx in &all_nodes {
+            for &leaf_idx in &all_nodes {
+                let target_data = self.get_step_data(target_idx)?;
+                let leaf_data = self.get_step_data(leaf_idx)?;
+
+                if target_data.can_merge_leafs(target_idx, leaf_idx, leaf_data, self) {
+                    trace!(
+                        "optimization found: merge leaf [{}] with [{}]",
+                        leaf_idx.index(),
+                        target_idx.index(),
+                    );
+                    return Ok(Some((target_idx, leaf_idx)));
+                }
+            }
+        }
+
+        Ok(None)
+    }
+}