FED-32: Port tryOptimizeSubselectionWithFragments
#5195
Conversation
- partially implemented `try_optimize_with_fragments`
duckki
requested review from
dariuszkuc,
sachindshinde,
goto-bus-stop and
SimonSapin
as code owners
|
@duckki, please consider creating a changeset entry in |
|
CI performance tests
|
There was a problem hiding this comment.
Wow this is a big one!!
First of all, your Rust has gotten so good. I am gobsmacked and super impressed at how quickly you got to writing this level of Rust. Incredible!
Second of all, I think we should keep the optimised functionality in operation.rs for a little while longer, and have a conversation about doing a refactor after we've finished everything with in the rewrite. It'll be easier to reason about the changes holistically once everything is in.
I am also really not certain working on the SelectionMap directly is a good idea. I played around a little bit with your changes to see if it's possible to work off a SelectionSet, and I think it should be. We just have to reason through the minus and the intersection functionality a little bit. Nothing wrong with doing a SelectionMap::new(), but I am more concerned about moving functions that are not reliant on being updated upon modifications.
There are also a few smaller comments and suggestions along the way.
| let key = TYPENAME_KEY.get_or_init(|| SelectionKey::Field { | ||
| response_name: TYPENAME_FIELD, | ||
| directives: Arc::new(Default::default()), | ||
| }); | ||
|
|
||
| self.contains_key(key) | ||
| } | ||
|
|
||
| pub(crate) fn fields_in_set(&self) -> Vec<CollectedFieldInSet> { | ||
| let mut fields = Vec::new(); | ||
|
|
||
| for (_key, selection) in self.iter() { | ||
| match selection { | ||
| Selection::Field(field) => fields.push(CollectedFieldInSet { | ||
| path: Vec::new(), | ||
| field: field.clone(), | ||
| }), | ||
| Selection::FragmentSpread(_fragment) => { | ||
| todo!() | ||
| } | ||
| Selection::InlineFragment(inline_fragment) => { | ||
| let condition = inline_fragment | ||
| .inline_fragment | ||
| .data() | ||
| .type_condition_position | ||
| .as_ref(); | ||
| let header = match condition { | ||
| Some(cond) => vec![FetchDataPathElement::TypenameEquals( | ||
| cond.type_name().clone().into(), | ||
| )], | ||
| None => vec![], | ||
| }; | ||
| for CollectedFieldInSet { path, field } in inline_fragment | ||
| .selection_set | ||
| .selections | ||
| .fields_in_set() | ||
| .into_iter() | ||
| { | ||
| let mut new_path = header.clone(); | ||
| new_path.extend(path); | ||
| fields.push(CollectedFieldInSet { | ||
| path: new_path, | ||
| field, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| fields | ||
| } | ||
|
|
||
| pub(crate) fn containment(&self, other: &Self, options: ContainmentOptions) -> Containment { | ||
| if other.len() > self.len() { | ||
| // If `other` has more selections but we're ignoring missing __typename, then in the case where | ||
| // `other` has a __typename but `self` does not, then we need the length of `other` to be at | ||
| // least 2 more than other of `self` to be able to conclude there is no contains. | ||
| if !options.ignore_missing_typename | ||
| || other.len() > self.len() + 1 | ||
| || self.has_top_level_typename_field() | ||
| || !other.has_top_level_typename_field() | ||
| { | ||
| return Containment::NotContained; | ||
| } | ||
| } | ||
|
|
||
| let mut is_equal = true; | ||
| let mut did_ignore_typename = false; | ||
|
|
||
| for (key, other_selection) in other.iter() { | ||
| if key.is_typename_field() && options.ignore_missing_typename { | ||
| if !self.has_top_level_typename_field() { | ||
| did_ignore_typename = true; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| let Some(self_selection) = self.get(key) else { | ||
| return Containment::NotContained; | ||
| }; | ||
|
|
||
| match self_selection.containment(other_selection, options) { | ||
| Containment::NotContained => return Containment::NotContained, | ||
| Containment::StrictlyContained if is_equal => is_equal = false, | ||
| Containment::StrictlyContained | Containment::Equal => {} | ||
| } | ||
| } | ||
|
|
||
| let expected_len = if did_ignore_typename { | ||
| self.len() + 1 | ||
| } else { | ||
| self.len() | ||
| }; | ||
|
|
||
| if is_equal && other.len() == expected_len { | ||
| Containment::Equal | ||
| } else { | ||
| Containment::StrictlyContained | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need these methods to kept up to date when a modification happens? The SelectionMap is an "element" type, which keeps the keys updated. The SelectionSet is the right place to have these methods if that's not the case. Given optimisation happens pretty much at the very end of the process, I'd wager we want to keep these out of SelectionMap.
| impl SelectionMap { | ||
| /// Performs set-subtraction (self - other) and returns the result (the difference between self | ||
| /// and other). | ||
| fn minus(&self, other: &SelectionMap) -> Result<SelectionMap, FederationError> { | ||
| let iter = self | ||
| .iter() | ||
| .map(|(k, v)| { | ||
| if let Some(other_v) = other.get(k) { | ||
| v.minus(other_v) | ||
| } else { | ||
| Ok(Some(v.clone())) | ||
| } | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()? // early break in case of Err | ||
| .into_iter() | ||
| .flatten(); | ||
| Ok(SelectionMap::from_iter(iter)) | ||
| } | ||
|
|
||
| /// Computes the set-intersection of self and other | ||
| fn intersection(&self, other: &SelectionMap) -> Result<SelectionMap, FederationError> { | ||
| if self.is_empty() { | ||
| return Ok(self.clone()); | ||
| } | ||
| if other.is_empty() { | ||
| return Ok(other.clone()); | ||
| } | ||
|
|
||
| let iter = self | ||
| .iter() | ||
| .map(|(k, v)| { | ||
| if let Some(other_v) = other.get(k) { | ||
| v.intersection(other_v) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()? // early break in case of Err | ||
| .into_iter() | ||
| .flatten(); | ||
| Ok(SelectionMap::from_iter(iter)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this these two functions should be on the SelectionMap, but rather on the SelectionSet directly. So far SelectionMap has been kept as an internal detail for the SelectionSet and we should keep it that way.
| fn from_selection_map(selection_map: &SelectionMap) -> Self { | ||
| Self::for_level(&selection_map.fields_in_set()) | ||
| } |
There was a problem hiding this comment.
Inline with the previous comments, this should be from_selection_set(Selection_set: &SelectionSet
There was a problem hiding this comment.
Let's keep optimize.rs as part of operation.rs for now until we can assess the changes we want to make it for it more holistically.
There was a problem hiding this comment.
The optimze business is largely isolated and GraphQL-specific (except for the minus/intersection).
I'm afraid that, if we put this in operation.rs, it might become a spaghetti.
| struct FragmentRestrictionAtType { | ||
| /// Selections that are expanded from a given fragment at a given type and then normalized. | ||
| /// - This represents the part of given type's sub-selections that are covered by the fragment. | ||
| selections: SelectionMap, |
There was a problem hiding this comment.
Can this be a SelectionSet instead?
| pub(crate) fn types_can_be_merged(&self, other: &Self) -> Result<bool, FederationError> { | ||
| let self_definition = self.data().field_position.get(self.schema().schema())?; | ||
| let other_definition = other.data().field_position.get(self.schema().schema())?; | ||
| types_can_be_merged( | ||
| &self_definition.ty, | ||
| &other_definition.ty, | ||
| self.schema().schema(), | ||
| ) | ||
| } | ||
|
|
||
| pub(crate) fn parent_type_position(&self) -> CompositeTypeDefinitionPosition { | ||
| self.data().field_position.parent() | ||
| } |
There was a problem hiding this comment.
these have to be on the outside of the mod implementation of the Field -
…of the inner module.
- Added a new file optimize.rs under query_plan. https://apollographql.atlassian.net/browse/FED-32 Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>
Implemented
try_optimize_with_fragmentsfunction.optimize.rsunderquery_plan.Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Unfortunately, testing is very cumbersome without the whole
optimizeimplementation (FED-191).I'll implement
optimizeand test this together.Notes
Footnotes
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. ↩