FED-191: [Operation Processing] port `operation.optimize` #5207

duckki · 2024-05-21T00:04:46Z

implemented the rest of Operation::optimize method

https://apollographql.atlassian.net/browse/FED-191

Depends on:

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Have a few simple tests. More tests shall be done by poring existing federation tests.

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

github-actions · 2024-05-21T00:04:57Z

@duckki, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

goto-bus-stop · 2024-05-23T11:55:16Z

apollo-federation/src/query_plan/optimize.rs

+        let Some(fragments) = fragments else {
+            return Ok(());
+        };


I feel it makes more sense to do this at the usage site, and require passing in a fragments: &NamedFragments.

goto-bus-stop · 2024-05-23T11:55:58Z

apollo-federation/src/query_plan/optimize.rs

+        if fragments.is_empty() {
+            return Ok(());
+        }
+        assert!(


We can avoid this assert either by taking a Option<NonZeroU32> parameter, or maybe even removing it entirely and hardcoding the value to 2 (AFAIK JS only used it in tests)

Makes sense. I'll remove the parameter altogether.

lrlna

This is one is complicated too 😱 I think I'll have to do another pass on Monday to make sure I didn't miss anything. The biggest thing for me is more tests. I know the JS repo's got a ton of optimize tests that we can borrow.

lrlna · 2024-05-24T13:20:49Z

apollo-federation/src/query_plan/operation.rs

+        pub(crate) fn output_ast_type(&self) -> Result<&ast::Type, FederationError> {
+            Ok(&self.field_position.get(self.schema.schema())?.ty)
+        }
+


this probably also needs to live on an impl outside of this internal mod. perhaps on Field?

router/apollo-federation/src/query_plan/operation.rs

Line 3777 in 1ccd91a

impl Field {

Actually, it's no longer used. Let's make it private and then decide where to put it later.

lrlna · 2024-05-24T13:22:22Z

apollo-federation/src/query_plan/operation.rs

@@ -2616,7 +2600,7 @@ impl SelectionSet {
    //   version, but its Rust version doesn't use `lazy_map`.
    // PORT_NOTE #2: Taking ownership of `self` in this method was considered. However, calling
    // `Arc::make_mut` on the `Arc` fields of `self` didn't seem better than cloning Arc instances.
-    fn lazy_map(
+    pub(crate) fn lazy_map(


don't particularly love that we are exposing more of these methods outside of the operation file, but I suppose we will deal with once we start fixing up this file.

lrlna · 2024-05-24T18:01:59Z

apollo-federation/src/query_plan/optimize.rs

+    /// Compute the reduced set of NamedFragments that are used in the selection set at least
+    /// `min_usage_to_optimize` times. Also, computes the new selection set that uses only the
+    /// reduced set of fragments by expanding the other ones.
+    fn reduce_named_fragments(


just reduce? we are already on the NamedFragments impl. or just call it optimize like on the other impls?

good point.

lrlna · 2024-05-24T18:18:12Z

apollo-federation/src/query_plan/optimize.rs

+        self.named_fragments = final_fragments;
+        Ok(())
+    }
+}


I know you've got some tests below already. There are also some additional tests in the JS repo that specifically look at fragment optimization. It'd be good to add those here as well. This functionality is complicated, and we are not doing exactly the same thing as the JS QP, so it'd be really good to port these along as well. https://github.com/apollographql/federation/blob/28850ffba17d4201321dc6de0edc24cae48ec5e5/internals-js/src/__tests__/operations.test.ts#L37-L1033

Oh, I didn't get to that today. I'll tackle this next week.

BTW, I was wondering if I would step on Simon's foot, porting tests. Let's synchronize on this next week. @SimonSapin

lrlna · 2024-05-24T18:20:35Z

apollo-federation/src/query_plan/optimize.rs

+impl Operation {
+    // PORT_NOTE: The JS version of `optimize` takes an optional `minUsagesToOptimize` argument.
+    //            However, it's only used in tests. So, it's removed in the Rust version.
+    const DEFAULT_MIN_USAGES_TO_OPTIMIZE: u32 = 2;


should we just remove this const entirely then? or do you want to keep it the argument to pass to tests as well?

It didn't seem necessary to test different values. However, I think literals would better be declared as const for clarity. Otherwise, they can become magic numbers that no one knows what they are.

lrlna · 2024-05-24T18:35:29Z

apollo-federation/src/query_plan/optimize.rs

+    pub(crate) fn optimize_at_root(
+        &self,
+        fragments: &NamedFragments,
+    ) -> Result<SelectionSet, FederationError> {


I am wondering why we are not mutating the existing structs with all these optimize functions, but are instead creating a brand new selection set/inline fragment selection/etc. I feel like we are doing a lot of cloning (and I know it's an Arc), only to use that cloned item to create the exact same modified type.

Good idea.

And I noticed a bug in this function and fixed it.

lrlna · 2024-05-27T17:24:41Z

apollo-federation/src/query_plan/optimize.rs

+    fn optimize(
+        &self,
+        fragments: &NamedFragments,
+        validator: &mut FieldsConflictMultiBranchValidator,
+    ) -> Result<Selection, FederationError> {


Do you think it'll be easier to reason about if this returns a FieldSelection instead? Something like this:

Suggested change

fn optimize(

&self,

fragments: &NamedFragments,

validator: &mut FieldsConflictMultiBranchValidator,

) -> Result<Selection, FederationError> {

fn optimize(

&mut self,

fragments: &NamedFragments,

validator: &mut FieldsConflictMultiBranchValidator,

) -> Result<Self, FederationError> {

I am thinking we are walking down the tree from the SelectionSet root, modifying and optimising it's potential children. It might be easier if the results bubble up as their own respective types, and we handle converting them into a Selection at one of the top level nodes?

Potentially might not work due to borrow checker. But I guess what I am trying to say is that I am finding it difficult to follow that FieldSelection/InlineFragmentSelection/etc all optimize, theoretically their own values, but they don't actually always modify their own selection_set field and also don't return the selection_set but a selection

Indeed. In the JS version, optimze() function was a virtual function with the same return type (Selection). But, in Rust, we don't have do that. The wrinkle here is that InlineFragmentSelection can be optimized into FragmentSpreadSelection. So, it makes sense to add a new enum InlineOrFragmentSelection, which helps to eliminate one unreachable!() branch case.

lrlna · 2024-05-27T17:25:15Z

apollo-federation/src/query_plan/optimize.rs

+impl InlineFragmentSelection {
+    fn optimize(
+        &self,
+        fragments: &NamedFragments,
+        validator: &mut FieldsConflictMultiBranchValidator,
+    ) -> Result<Selection, FederationError> {


Same comment as above.

lrlna

Ok, I did a second pass, and nothing really jumps out at me. I think the main thing to do is to port those optimisation tests. It'll be a huge help here!

- Note: JS version caches the named fragments in FragmentSpreadSelection.

…ke a parent type argument

…ectly - also, ported several operation.optimization tests

duckki · 2024-05-29T03:25:42Z

I ported a few tests. That detected a bug in optimization and in a few other places. I'd like to merge this PR and hunt the rest (if any) together as we port tests.

lrlna

Glad you already found some bugs! Let's merge and find some more.

lrlna · 2024-05-29T12:44:39Z

apollo-federation/src/query_plan/operation.rs

+            normalized_operation.named_fragments = Default::default();
+            insta::assert_snapshot!(normalized_operation, @r###"
+                query NamedFragmentQuery {
+                  foo {
+                    id
+                    bar
+                    baz
+                  }
+                }
+            "###);


do we want to have snapshots inside mods? have we done this before? i don't want to us to introduce a new pattern for mod tests

lrlna · 2024-05-29T12:49:51Z

There are some snapshot tests changes, and the new changes produced by these snapshots genuinely look incorrect - spacing for fragment definitions looks off 🤔

- Some have been fixed. Updated their annotations. - The rest are failing with different issues.

duckki · 2024-05-29T21:03:47Z

I fixed the spacing issue and a bug in reuse_query_fragments option handling.
I reviewed the status of failing tests and annotated appropriately (some improved and some failed showing different symptoms).
I think it's ok to merge into dev.

Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>

…hql#5207) Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>

duckki requested review from dariuszkuc, sachindshinde, goto-bus-stop and SimonSapin as code owners May 21, 2024 00:04

duckki changed the base branch from dev to duckki/FED-32 May 21, 2024 00:06

lrlna self-assigned this May 21, 2024

duckki mentioned this pull request May 21, 2024

Ported expand_fragments #5147

Closed

6 tasks

duckki force-pushed the duckki/FED-191 branch 2 times, most recently from f364f53 to f7486ef Compare May 22, 2024 17:01

Base automatically changed from duckki/FED-32 to dev May 22, 2024 17:17

duckki force-pushed the duckki/FED-191 branch from f7486ef to 7b9de3b Compare May 22, 2024 18:01

duckki added 2 commits May 22, 2024 16:26

implemented the rest of Operation::optimize method

b0efd74

updated map_to_expanded_selection_sets method

18cff54

duckki force-pushed the duckki/FED-191 branch from 7b9de3b to 18cff54 Compare May 22, 2024 23:28

goto-bus-stop reviewed May 23, 2024

View reviewed changes

lrlna assigned goto-bus-stop and unassigned lrlna May 23, 2024

addressed review comments

1ccd91a

lrlna reviewed May 24, 2024

View reviewed changes

addressed reviewer comments (take 2)

97e6b67

lrlna reviewed May 27, 2024

View reviewed changes

lrlna assigned lrlna and unassigned goto-bus-stop May 28, 2024

duckki added 3 commits May 28, 2024 10:31

updated fn optimize method's return types to be more specific

15adffa

Same fix in PR #5243 (to be merged)

34f65f0

updated lazy_map to pass the named fragments to make_selection_set

4c35950

- Note: JS version caches the named fragments in FragmentSpreadSelection.

duckki added 2 commits May 28, 2024 20:11

fixed InlineFragmentSelection::from_fragment_spread_selection to ta…

9d46bb3

…ke a parent type argument

fixed NamedFragments::reduce to drop fragments only used once indir…

8f8a7c6

…ectly - also, ported several operation.optimization tests

duckki force-pushed the duckki/FED-191 branch from f40e865 to 8f8a7c6 Compare May 29, 2024 03:24

fixed a linter issue

314f940

lrlna approved these changes May 29, 2024

View reviewed changes

Merge branch 'dev' into duckki/FED-191

21bc0ff

lrlna enabled auto-merge (squash) May 29, 2024 10:17

Merge branch 'dev' into duckki/FED-191

c1c8a52

lrlna reviewed May 29, 2024

View reviewed changes

lrlna mentioned this pull request May 29, 2024

chore(apollo-federation): port skip and include snapshot tests #5273

Closed

duckki added 4 commits May 29, 2024 11:48

fixed write_operation to space fragments correctly

48d9243

fixed reuse_query_fragments option's behavior when it's off

1d60ef9

updated tests

9dbc592

- Some have been fixed. Updated their annotations. - The rest are failing with different issues.

Merge remote-tracking branch 'origin/dev' into duckki/FED-191

9a4679b

lrlna merged commit a9b8d0e into dev May 29, 2024
13 of 14 checks passed

lrlna deleted the duckki/FED-191 branch May 29, 2024 21:19

lrlna added a commit that referenced this pull request Jun 3, 2024

FED-191: [Operation Processing] port operation.optimize (#5207)

d26abf2

Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>

theJC pushed a commit to theJC/router that referenced this pull request Jun 10, 2024

FED-191: [Operation Processing] port operation.optimize (apollograp…

93bf807

…hql#5207) Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-191: [Operation Processing] port `operation.optimize` #5207

FED-191: [Operation Processing] port `operation.optimize` #5207

duckki commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024

goto-bus-stop May 23, 2024

goto-bus-stop May 23, 2024 •

edited

Loading

duckki May 24, 2024

lrlna left a comment

lrlna May 24, 2024

duckki May 25, 2024

lrlna May 24, 2024

duckki May 25, 2024

lrlna May 24, 2024

duckki May 25, 2024

lrlna May 24, 2024

duckki May 25, 2024

lrlna May 24, 2024

duckki May 25, 2024

lrlna May 24, 2024

duckki May 25, 2024

lrlna May 27, 2024 •

edited

Loading

lrlna May 27, 2024

duckki May 28, 2024

lrlna May 27, 2024

lrlna left a comment

duckki commented May 29, 2024

lrlna left a comment

lrlna May 29, 2024

lrlna commented May 29, 2024

duckki commented May 29, 2024

FED-191: [Operation Processing] port operation.optimize #5207

FED-191: [Operation Processing] port operation.optimize #5207

Conversation

duckki commented May 21, 2024 • edited Loading

Footnotes

github-actions bot commented May 21, 2024

Choose a reason for hiding this comment

goto-bus-stop May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrlna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrlna May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrlna left a comment

Choose a reason for hiding this comment

duckki commented May 29, 2024

lrlna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrlna commented May 29, 2024

duckki commented May 29, 2024

FED-191: [Operation Processing] port `operation.optimize` #5207

FED-191: [Operation Processing] port `operation.optimize` #5207

duckki commented May 21, 2024 •

edited

Loading

goto-bus-stop May 23, 2024 •

edited

Loading

lrlna May 27, 2024 •

edited

Loading