Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FED-32: Port tryOptimizeSubselectionWithFragments #5195

Merged
merged 14 commits into from
May 22, 2024
Merged

FED-32: Port tryOptimizeSubselectionWithFragments #5195

merged 14 commits into from
May 22, 2024

Conversation

duckki
Copy link
Contributor

@duckki duckki commented May 17, 2024

Implemented try_optimize_with_fragments function.

  • Added a new file optimize.rs under query_plan.
  • Refactored some SelectionSet methods and moved them to SelectionMap for reuse.

Checklist

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

  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Tests added and passing3
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Unfortunately, testing is very cumbersome without the whole optimize implementation (FED-191).
I'll implement optimize and test this together.

Notes

Footnotes

  1. 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.

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples.

  3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

Copy link
Contributor

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

@router-perf
Copy link

router-perf bot commented May 17, 2024

CI performance tests

  • step - Basic stress test that steps up the number of users over time
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • large-request - Stress test with a 1 MB request payload
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • xxlarge-request - Stress test with 100 MB request payload
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • xlarge-request - Stress test with 10 MB request payload
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • no-graphos - Basic stress test, no GraphOS.
  • reload - Reload test over a long period of time at a constant rate of users
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • const - Basic stress test that runs with a constant number of users

Copy link
Member

@lrlna lrlna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 428 to 532
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
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 104 to 146
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))
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 223 to 225
fn from_selection_map(selection_map: &SelectionMap) -> Self {
Self::for_level(&selection_map.fields_in_set())
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline with the previous comments, this should be from_selection_set(Selection_set: &SelectionSet

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

apollo-federation/src/query_plan/optimize.rs Show resolved Hide resolved
apollo-federation/src/query_plan/optimize.rs Outdated Show resolved Hide resolved
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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a SelectionSet instead?

apollo-federation/src/query_plan/optimize.rs Show resolved Hide resolved
apollo-federation/src/query_plan/optimize.rs Show resolved Hide resolved
apollo-federation/src/query_plan/optimize.rs Outdated Show resolved Hide resolved
@lrlna lrlna self-assigned this May 21, 2024
Comment on lines 1234 to 1246
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()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these have to be on the outside of the mod implementation of the Field -

@duckki duckki enabled auto-merge (squash) May 22, 2024 17:00
@duckki duckki merged commit d34c70b into dev May 22, 2024
13 of 14 checks passed
@duckki duckki deleted the duckki/FED-32 branch May 22, 2024 17:17
lrlna added a commit that referenced this pull request Jun 3, 2024
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants