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

Fix duplicate fetches in query plans #579

Closed
wants to merge 5 commits into from

Conversation

martijnwalraven
Copy link
Contributor

Fixes #399 and #294.

The issue fixed by this PR requires a bit of unpacking. The visible result is that in some cases, the query plan will contain duplicate fetches to the same underlying service. For this query, for example:

query {
  topProducts {
    name
    price
    reviews {
      body
      author {
        name
      }
    }
  }
}

We end up fetching the names of authors as many times as there are different types of products. (It isn’t just that we fetch the authors of the reviews of each product type separately, but we literally execute the same query with the same representations, so all authors for all products, multiple times.)

In the case above, this is triggered as a consequence of type explosion, but it is a separate issue and also occurs in other cases where we manually include duplicate nested fields under multiple type conditions.

This query, for example, will also contain as many fetches for authors as there are type conditions (so two fetches here):

query {
  topProducts {
    name
    price
    ... on Book {
      reviews {
        body
        author {
          name
        }
      }
    }
    ... on Video {
      reviews {
        body
        author {
          name
        }
      }
    }
  }
}

The underlying cause is that we don't merge dependent fetch groups from subfields. In this case, we’ll create separate fetch groups for { name } nested under the different type conditions, and even though these operate on the same path and fetch data from the same service, they are treated as separate fetches (that all have the same path however, which is why they take the same representations as input).

This PR attempts to solve this by manually merging nested fetch groups whenever they are compatible. That fixes the issue in most cases, but there are more complicated situations it doesn't address and that will require more substantial changes to the query planner and execution.

In particular, when possible types are spread over multiple services, it will not be possible to merge nested fetch groups from subfields because these should actually result in separate fetches. What you don't want however, is for those fetches to operate on the same objects just fetched by the other, but since representations taken as input are collected by path, there is no way to differentiate between these.

I believe solving this latter problem requires additional capabilities in the query plan (which is something we’ve talked about before). The most straightforward way to fix this seems to be a TypeCondition node in the query plan, which filters input representations based on their type.

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.

This looks good!

The only thing I can add is that I'd make sure y'all run cargo clippy and fix the warning that it gives you. There are a bunch of unused imports and a few other nits in various parts of this repo.

@martijnwalraven
Copy link
Contributor Author

Closing this in favor of #671.

@dariuszkuc dariuszkuc deleted the martijnwalraven/duplicate-fetches-fix branch April 4, 2023 15:19
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.

Query planner duplicate calls to services on interface types
2 participants