Drop experimental reuse fragments optimization from RS QP

.changesets/breaking_drop_reuse_fragment.md

@@ -0,0 +1,7 @@
+### Drop experimental reuse fragment query optimization option ([PR #6353](https://github.com/apollographql/router/pull/6353))
+
+Drop support for the experimental reuse fragment query optimization. This implementation was not only very slow but also very buggy due to its complexity.
+
+Auto generation of fragments is a much simpler (and faster) algorithm that in most cases produces better results. Fragment auto generation is the default optimization since v1.58 release.
+
+By [@dariuszkuc](https://github.com/dariuszkuc) in https://github.com/apollographql/router/pull/6353

apollo-federation/cli/src/main.rs

@@ -23,9 +23,6 @@ struct QueryPlannerArgs {
     /// Enable @defer support.
     #[arg(long, default_value_t = false)]
     enable_defer: bool,
-    /// Reuse fragments to compress subgraph queries.
-    #[arg(long, default_value_t = false)]
-    reuse_fragments: bool,
     /// Generate fragments to compress subgraph queries.
     #[arg(long, default_value_t = false)]
     generate_fragments: bool,
@@ -109,8 +106,6 @@ enum Command {
 impl QueryPlannerArgs {
     fn apply(&self, config: &mut QueryPlannerConfig) {
         config.incremental_delivery.enable_defer = self.enable_defer;
-        // --generate-fragments trumps --reuse-fragments
-        config.reuse_query_fragments = self.reuse_fragments && !self.generate_fragments;
         config.generate_query_fragments = self.generate_fragments;
         config.subgraph_graphql_validation = self.subgraph_validation.unwrap_or(true);
         if let Some(max_evaluated_plans) = self.max_evaluated_plans {

apollo-federation/src/operation/mod.rs

@@ -921,17 +921,6 @@ impl FragmentSpreadSelection {
         })
     }
 
-    pub(crate) fn from_fragment(
-        fragment: &Node<Fragment>,
-        directives: &executable::DirectiveList,
-    ) -> Self {
-        let spread = FragmentSpread::from_fragment(fragment, directives);
-        Self {
-            spread,
-            selection_set: fragment.selection_set.clone(),
-        }
-    }
-
     /// Creates a fragment spread selection (in an optimized operation).
     /// - `named_fragments`: Named fragment definitions that are rebased for the element's schema.
     pub(crate) fn new(
@@ -2171,15 +2160,6 @@ impl SelectionSet {
         })
     }
 
-    /// In a normalized selection set containing only fields and inline fragments,
-    /// iterate over all the fields that may be selected.
-    ///
-    /// # Preconditions
-    /// The selection set must not contain named fragment spreads.
-    pub(crate) fn field_selections(&self) -> FieldSelectionsIter<'_> {
-        FieldSelectionsIter::new(self.selections.values())
-    }
-
     /// # Preconditions
     /// The selection set must not contain named fragment spreads.
     fn fields_in_set(&self) -> Vec<CollectedFieldInSet> {
@@ -2320,36 +2300,6 @@ impl<'a> IntoIterator for &'a SelectionSet {
     }
 }
 
-pub(crate) struct FieldSelectionsIter<'sel> {
-    stack: Vec<selection_map::Values<'sel>>,
-}
-
-impl<'sel> FieldSelectionsIter<'sel> {
-    fn new(iter: selection_map::Values<'sel>) -> Self {
-        Self { stack: vec![iter] }
-    }
-}
-
-impl<'sel> Iterator for FieldSelectionsIter<'sel> {
-    type Item = &'sel Arc<FieldSelection>;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        match self.stack.last_mut()?.next() {
-            None if self.stack.len() == 1 => None,
-            None => {
-                self.stack.pop();
-                self.next()
-            }
-            Some(Selection::Field(field)) => Some(field),
-            Some(Selection::InlineFragment(frag)) => {
-                self.stack.push(frag.selection_set.selections.values());
-                self.next()
-            }
-            Some(Selection::FragmentSpread(_frag)) => unreachable!(),
-        }
-    }
-}
-
 #[derive(Clone, Debug)]
 pub(crate) struct SelectionSetAtPath {
     path: Vec<FetchDataPathElement>,
@@ -2625,16 +2575,6 @@ impl Field {
     pub(crate) fn parent_type_position(&self) -> CompositeTypeDefinitionPosition {
         self.field_position.parent()
     }
-
-    pub(crate) fn types_can_be_merged(&self, other: &Self) -> Result<bool, FederationError> {
-        let self_definition = self.field_position.get(self.schema().schema())?;
-        let other_definition = other.field_position.get(self.schema().schema())?;
-        types_can_be_merged(
-            &self_definition.ty,
-            &other_definition.ty,
-            self.schema().schema(),
-        )
-    }
 }
 
 impl InlineFragmentSelection {
@@ -2732,23 +2672,6 @@ impl InlineFragmentSelection {
         ))
     }
 
-    /// Construct a new InlineFragmentSelection out of a selection set.
-    /// - The new type condition will be the same as the selection set's type.
-    pub(crate) fn from_selection_set(
-        parent_type_position: CompositeTypeDefinitionPosition,
-        selection_set: SelectionSet,
-        directives: DirectiveList,
-    ) -> Self {
-        let inline_fragment_data = InlineFragment {
-            schema: selection_set.schema.clone(),
-            parent_type_position,
-            type_condition_position: selection_set.type_position.clone().into(),
-            directives,
-            selection_id: SelectionId::new(),
-        };
-        InlineFragmentSelection::new(inline_fragment_data, selection_set)
-    }
-
     pub(crate) fn casted_type(&self) -> &CompositeTypeDefinitionPosition {
         self.inline_fragment
             .type_condition_position
@@ -2811,31 +2734,10 @@ impl NamedFragments {
         NamedFragments::initialize_in_dependency_order(fragments, schema)
     }
 
-    pub(crate) fn is_empty(&self) -> bool {
-        self.fragments.len() == 0
-    }
-
-    pub(crate) fn len(&self) -> usize {
-        self.fragments.len()
-    }
-
     pub(crate) fn iter(&self) -> impl Iterator<Item = &Node<Fragment>> {
         self.fragments.values()
     }
 
-    pub(crate) fn iter_rev(&self) -> impl Iterator<Item = &Node<Fragment>> {
-        self.fragments.values().rev()
-    }
-
-    pub(crate) fn iter_mut(&mut self) -> indexmap::map::IterMut<'_, Name, Node<Fragment>> {
-        Arc::make_mut(&mut self.fragments).iter_mut()
-    }
-
-    // Calls `retain` on the underlying `IndexMap`.
-    pub(crate) fn retain(&mut self, mut predicate: impl FnMut(&Name, &Node<Fragment>) -> bool) {
-        Arc::make_mut(&mut self.fragments).retain(|name, fragment| predicate(name, fragment));
-    }
-
     fn insert(&mut self, fragment: Fragment) {
         Arc::make_mut(&mut self.fragments).insert(fragment.name.clone(), Node::new(fragment));
     }
@@ -2929,32 +2831,6 @@ impl NamedFragments {
             }
         })
     }
-
-    /// When we rebase named fragments on a subgraph schema, only a subset of what the fragment handles may belong
-    /// to that particular subgraph. And there are a few sub-cases where that subset is such that we basically need or
-    /// want to consider to ignore the fragment for that subgraph, and that is when:
-    /// 1. the subset that apply is actually empty. The fragment wouldn't be valid in this case anyway.
-    /// 2. the subset is a single leaf field: in that case, using the one field directly is just shorter than using
-    ///    the fragment, so we consider the fragment don't really apply to that subgraph. Technically, using the
-    ///    fragment could still be of value if the fragment name is a lot smaller than the one field name, but it's
-    ///    enough of a niche case that we ignore it. Note in particular that one sub-case of this rule that is likely
-    ///    to be common is when the subset ends up being just `__typename`: this would basically mean the fragment
-    ///    don't really apply to the subgraph, and that this will ensure this is the case.
-    pub(crate) fn is_selection_set_worth_using(selection_set: &SelectionSet) -> bool {
-        if selection_set.selections.len() == 0 {
-            return false;
-        }
-        if selection_set.selections.len() == 1 {
-            // true if NOT field selection OR non-leaf field
-            return if let Some(Selection::Field(field_selection)) = selection_set.selections.first()
-            {
-                field_selection.selection_set.is_some()
-            } else {
-                true
-            };
-        }
-        true
-    }
 }
 
 // @defer handling: removing and normalization
@@ -3393,49 +3269,6 @@ impl Operation {
     }
 }
 
-// Collect fragment usages from operation types.
-
-impl Selection {
-    fn collect_used_fragment_names(&self, aggregator: &mut IndexMap<Name, u32>) {
-        match self {
-            Selection::Field(field_selection) => {
-                if let Some(s) = &field_selection.selection_set {
-                    s.collect_used_fragment_names(aggregator)
-                }
-            }
-            Selection::InlineFragment(inline) => {
-                inline.selection_set.collect_used_fragment_names(aggregator);
-            }
-            Selection::FragmentSpread(fragment) => {
-                let current_count = aggregator
-                    .entry(fragment.spread.fragment_name.clone())
-                    .or_default();
-                *current_count += 1;
-            }
-        }
-    }
-}
-
-impl SelectionSet {
-    pub(crate) fn collect_used_fragment_names(&self, aggregator: &mut IndexMap<Name, u32>) {
-        for s in self.selections.values() {
-            s.collect_used_fragment_names(aggregator);
-        }
-    }
-
-    pub(crate) fn used_fragments(&self) -> IndexMap<Name, u32> {
-        let mut usages = IndexMap::default();
-        self.collect_used_fragment_names(&mut usages);
-        usages
-    }
-}
-
-impl Fragment {
-    pub(crate) fn collect_used_fragment_names(&self, aggregator: &mut IndexMap<Name, u32>) {
-        self.selection_set.collect_used_fragment_names(aggregator)
-    }
-}
-
 // Collect used variables from operation types.
 
 pub(crate) struct VariableCollector<'s> {
@@ -3533,16 +3366,6 @@ impl<'s> VariableCollector<'s> {
     }
 }
 
-impl Fragment {
-    /// Returns the variable names that are used by this fragment.
-    pub(crate) fn used_variables(&self) -> IndexSet<&'_ Name> {
-        let mut collector = VariableCollector::new();
-        collector.visit_directive_list(&self.directives);
-        collector.visit_selection_set(&self.selection_set);
-        collector.into_inner()
-    }
-}
-
 impl SelectionSet {
     /// Returns the variable names that are used by this selection set, including through fragment
     /// spreads.
@@ -3905,7 +3728,9 @@ pub(crate) fn normalize_operation(
         variables: Arc::new(operation.variables.clone()),
         directives: operation.directives.clone().into(),
         selection_set: normalized_selection_set,
-        named_fragments,
+        // fragments were already expanded into selection sets
+        // new ones will be generated when optimizing the final subgraph fetch operations
+        named_fragments: Default::default(),
     };
     Ok(normalized_operation)
 }