FED-47: Finish porting `compute_best_plan_from_closed_branches` #277

duckki · 2024-04-26T05:27:16Z

~~Mostly finished, but still in progress. I wanted to get some feedback in a few areas.~~

This PR is now ready for review.

`OpPathTree::merge(&self, ...) -> Self`

I needed OpPathTree::merge(&self, ...) -> Self, but merge name is taken.
We already have PathTree::merge(self: &Arc<Self>, ...) -> Arc<Self>.
Probably, we can refactor the common part out. (@SimonSapin What do you think? Would there be a better way?)
But, how would you like to name the new method? I tentatively chose merge_raw.

Update: I decided to use the existing PathTree::merge method and it worked.

`impl Clone for FetchDependencyGraph`

~~I added a simple cloning, but it's probably wrong, since the JS version looks a lot more complicated.~~

Update: Sachin confirmed the derived Clone implementation would suffice.

Debug logging

~~JS code has debug.log(...) lines. How do we want to port those?~~

Update: I left the logging code, but commented it out. We can decide what to do after rover integration.

…` method

src/query_graph/graph_path.rs

- returning an Iterator, rather than a slice

- So, I could use the existing `merge` method.

lrlna

I am going to "approve" so you're not stuck without a review due to timezones. But it would be good to change the GenerateContext trait into methods directly on the QueryPlanningTraversal impl. It'll simplify some of the definitions and the usage in generate_all_plans_and_find_best.

lrlna · 2024-04-30T12:00:06Z

src/query_graph/graph_path.rs

+impl std::fmt::Display for SimultaneousPaths {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        self.fmt_indented(&mut IndentedFormatter::new(f))
+    }
+}
+


I'd just have the fmt_indented straight up in here, I don't think it needs an extra Impl block.

I also feel a bit strange about matching on sliced elements. A nicer pattern would be to do a .split_first().

Suggested change

impl std::fmt::Display for SimultaneousPaths {

fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {

self.fmt_indented(&mut IndentedFormatter::new(f))

}

}

impl std::fmt::Display for SimultaneousPaths {

fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {

let mut state = IndentedFormatter::new(f);

if let Some((first, elements)) = self.0.split_first() {

state.write_fmt(format_args!("{{ {first} }}"))?;

state.write("{")?;

write_indented_lines(&mut state, elements, |f, elem| f.write(elem))?;

state.write("}")

} else {

state.write("<no path>")

}

}

}

Although fmt_indented is not used elsewhere in federation-next, the original JS has other places using it. So, I wanted to keep this method available for later.

As to matching as_slice() is that the formatting is different if the self.0 has none, one element or more than one. So, I think matching on empty, one item and multiple items cases is easier to understand.

I agree with Iryna that we should just move that logic to the display trait as even if fmt_indented was used elsewhere in JS codebase -> if display is just referencing the fmt_indented in all other places anyway we could now just rely on the display to do the same thing.

It's an anti-pattern in Rust to have formatting done in a non-Display or non-Debug implementations. The Rust implementation should be using those whenever a formatted version of a struct is needed, and not a one-off method. It's an expectation of the ecosystem.

lrlna · 2024-04-30T12:01:17Z

src/query_graph/graph_path.rs

+impl SimultaneousPaths {
+    pub(crate) fn fmt_indented(&self, f: &mut IndentedFormatter) -> std::fmt::Result {
+        match self.0.as_slice() {
+            [] => f.write("<no path>"),
+
+            [first] => f.write_fmt(format_args!("{{ {first} }}")),
+
+            _ => {
+                f.write("{")?;
+                write_indented_lines(f, &self.0, |f, elem| f.write(elem))?;
+                f.write("}")
+            }
+        }
+    }
+}
+


I'd just have this as part of Display impl below (see the following suggestion)

Suggested change

impl SimultaneousPaths {

pub(crate) fn fmt_indented(&self, f: &mut IndentedFormatter) -> std::fmt::Result {

match self.0.as_slice() {

[] => f.write("<no path>"),

[first] => f.write_fmt(format_args!("{{ {first} }}")),

_ => {

f.write("{")?;

write_indented_lines(f, &self.0, |f, elem| f.write(elem))?;

f.write("}")

}

}

}

}

lrlna · 2024-04-30T12:08:19Z

src/query_plan/display.rs

+impl<'a, 'b> State<'a, 'b> {
+    pub(crate) fn new(output: &'a mut fmt::Formatter<'b>) -> State<'a, 'b> {


This is more for a hygiene follow up in the future. It seems like we now how specific display impls in query_plan and query_graph modules. I like that query_plan groups it all together in display.rs, query_graph doesn't have that. I think we can have this State live outside of the two modules (straight up in src?) and have display.rs be implemented in both.

I agree. This is struct is generic and reusable across.

lrlna · 2024-04-30T12:21:47Z

src/query_plan/generate.rs

+// as arguments. However, more than one capture the QueryPlanningTraversal object and
+// one of them mutably captures it, which is not allowed in Rust. Therefore, we use a trait
+// that implements all three methods.
+pub trait GenerateContext<Plan, Element> {


Maybe something like PlanContext? GenerateContext seems a bit vague here.

I agree. I think it should've been PlanGeneratorContext.

lrlna · 2024-04-30T12:27:04Z

src/query_plan/generate.rs

+    ///   with both the cost of that plan and the best cost we have generated thus far
+    ///   (if that's not the first plan generated).
+    ///   This mostly exists to allow some debugging.
+    fn on_plan(&mut self, plan: &Plan, cost: QueryPlanCost, prev_cost: Option<QueryPlanCost>);


on_plan is vague as well, anything can happen in this method kind of thing!🤯 How about context or cost_context or get_context?

While add/cost has specific functions, on_plan is just an event callback and has no expected behaviors. I'll change it to use immutable &self and rename it as on_plan_generated, which would be a bit clearer.
I also decided to rename add to add_to_plan and cost to compute_plan_cost for clarity.

lrlna · 2024-04-30T12:30:23Z

src/query_plan/generate.rs

+// as arguments. However, more than one capture the QueryPlanningTraversal object and
+// one of them mutably captures it, which is not allowed in Rust. Therefore, we use a trait
+// that implements all three methods.
+pub trait GenerateContext<Plan, Element> {


I am also wondering if this needs to be a trait, or can just be a struct, or can these methods live right in the QueryGraphTraversal impl?

generate_all_plans_and_find_best was already written generically (using closures) and tested without QueryGraphTraversal impl. So, I couldn't use QueryGraphTraversal directly in it, which is a bit odd (since it is the only real ussage) but not a bad thing nontheless. Instead, I made it a trait, as an alternative to closure.

lrlna · 2024-04-30T12:48:17Z

src/query_plan/query_planning_traversal.rs

+            fn add(
+                &mut self,
+                (plan_graph, plan_tree): &(FetchDependencyGraph, Arc<OpPathTree>),
+                tree: Arc<OpPathTree>,
+            ) -> (FetchDependencyGraph, Arc<OpPathTree>) {
+                let mut updated_graph = plan_graph.clone();
+                let result = self
+                    .outer_self
+                    .updated_dependency_graph(&mut updated_graph, &tree);
+                if result.is_ok() {
+                    let updated_tree = plan_tree.merge(&tree);
+                    (updated_graph, updated_tree)
+                } else {
+                    // Failed to update. Return the original plan.
+                    (updated_graph, plan_tree.clone())
+                }
+            }


Having read through this impl of the trait's .add, I really don't think it needs to be a trait. This add can just live as a function on this struct instead.

I agree that QueryPlanningTraversal can implement those functions directly. Thought I'd have to keep the trait, I added impl trait for QueryPlanningTraversal.

lrlna · 2024-04-30T12:49:19Z

src/query_plan/query_planning_traversal.rs

+                (plan_graph, plan_tree): &(FetchDependencyGraph, Arc<OpPathTree>),
+                tree: Arc<OpPathTree>,
+            ) -> (FetchDependencyGraph, Arc<OpPathTree>) {
+                let mut updated_graph = plan_graph.clone();


I don't know if it's going to be possible, but perhaps we can see if we can avoid this clone 🤔

Sachin said cloning is a part of the original algorithm. I don't think we can avoid it as it currently stands.
At least, Rust clone() implementation is much cleaner than the JS counterpart.

src/query_plan/generate.rs

…d_find_best`.

duckki · 2024-04-30T20:22:48Z

@lrlna Thanks for the review. I think I addressed most of your comments. But, I kept fmt_indented and the trait for generate_all_plans_and_find_best argument. I hope that's ok with you. I'll go ahead and merge.

…lographql/federation-next#277) - refactor: moved `query_plan::display::State` to `crate::indented_display` module since it's quite reusable across the board.

duckki added 2 commits April 25, 2024 18:36

added impl Display for ClosedPath and SimultaneousPaths

d132e6c

wip: implementing the rest of `compute_best_plan_from_closed_branches…

2d4fb07

…` method

lrlna assigned SimonSapin Apr 26, 2024

SimonSapin reviewed Apr 26, 2024

View reviewed changes

src/query_graph/graph_path.rs Outdated Show resolved Hide resolved

duckki added 4 commits April 26, 2024 15:27

revised ClosedPath::flatten() to follow the idomatic Rust style

d92e45f

- returning an Iterator, rather than a slice

slight refactoring

3c90384

changed to use merge Arc<OpPathTree>, instead of OpPathTree directly

e1163d4

- So, I could use the existing `merge` method.

improved SimultaneousPaths::fmt_indented

c861788

duckki changed the title ~~[Graph Traversal] Finish porting QueryPlanningTraversal::compute_best_plan_from_closed_branches~~ FED-47: Finish porting QueryPlanningTraversal::compute_best_plan_from_closed_branches Apr 27, 2024

duckki changed the title ~~FED-47: Finish porting QueryPlanningTraversal::compute_best_plan_from_closed_branches~~ FED-47: Finish porting compute_best_plan_from_closed_branches Apr 27, 2024

duckki marked this pull request as ready for review April 27, 2024 00:16

duckki requested review from dariuszkuc, sachindshinde and goto-bus-stop as code owners April 27, 2024 00:16

lrlna unassigned SimonSapin Apr 29, 2024

goto-bus-stop assigned goto-bus-stop and lrlna Apr 29, 2024

lrlna approved these changes Apr 30, 2024

View reviewed changes

duckki added 3 commits April 30, 2024 12:42

refactor: addressed review comments related to `generate_all_plans_an…

1d6d51d

…d_find_best`.

refactor: moved the State struct into src/indented_display.rs

72ba969

Merge branch 'main' into duckki/FED-47

85718f9

duckki merged commit 7858182 into main Apr 30, 2024
7 checks passed

duckki deleted the duckki/FED-47 branch April 30, 2024 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-47: Finish porting `compute_best_plan_from_closed_branches` #277

FED-47: Finish porting `compute_best_plan_from_closed_branches` #277

duckki commented Apr 26, 2024 •

edited

lrlna left a comment

lrlna Apr 30, 2024

duckki Apr 30, 2024

dariuszkuc May 1, 2024

lrlna May 1, 2024

lrlna Apr 30, 2024

lrlna Apr 30, 2024

duckki Apr 30, 2024

lrlna Apr 30, 2024

duckki Apr 30, 2024

lrlna Apr 30, 2024

duckki Apr 30, 2024

lrlna Apr 30, 2024

duckki Apr 30, 2024

lrlna Apr 30, 2024

duckki Apr 30, 2024

lrlna Apr 30, 2024

duckki Apr 30, 2024

duckki commented Apr 30, 2024

-impl std::fmt::Display for SimultaneousPaths {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        self.fmt_indented(&mut IndentedFormatter::new(f))
-    }
-}
+impl std::fmt::Display for SimultaneousPaths {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        let mut state = IndentedFormatter::new(f);
+        if let Some((first, elements)) = self.0.split_first() {
+            state.write_fmt(format_args!("{{ {first} }}"))?;
+            state.write("{")?;
+            write_indented_lines(&mut state, elements, |f, elem| f.write(elem))?;
+            state.write("}")
+        } else {
+            state.write("<no path>")
+        }
+    }
+}

		impl<'a, 'b> State<'a, 'b> {
		pub(crate) fn new(output: &'a mut fmt::Formatter<'b>) -> State<'a, 'b> {

FED-47: Finish porting compute_best_plan_from_closed_branches #277

FED-47: Finish porting compute_best_plan_from_closed_branches #277

Conversation

duckki commented Apr 26, 2024 • edited

OpPathTree::merge(&self, ...) -> Self

impl Clone for FetchDependencyGraph

Debug logging

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

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

duckki commented Apr 30, 2024

FED-47: Finish porting `compute_best_plan_from_closed_branches` #277

FED-47: Finish porting `compute_best_plan_from_closed_branches` #277

duckki commented Apr 26, 2024 •

edited

`OpPathTree::merge(&self, ...) -> Self`

`impl Clone for FetchDependencyGraph`