Skip to content

Subgraph Composition - #6

Open
ghost wants to merge 18 commits into
masterfrom
subgraph-composition
Open

Subgraph Composition#6
ghost wants to merge 18 commits into
masterfrom
subgraph-composition

Conversation

@ghost

@ghost ghost commented Dec 13, 2019

Copy link
Copy Markdown

The engineering plan only includes schema merging right now, as I'm still working through query execution.

Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
ghost pushed a commit that referenced this pull request Dec 16, 2019

@Jannis Jannis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quick comment: This is missing entries in SUMMARY.md and approved.md like these:

image

@Jannis
Jannis requested a review from a team December 17, 2019 15:13
@Jannis Jannis assigned ghost Dec 17, 2019
@Jannis Jannis added the engineering-plan This is an Engineering Plan label Dec 17, 2019
@ghost

ghost commented Dec 17, 2019

Copy link
Copy Markdown
Author

@Jannis Comments addressed except for the one from this morning, addressing that now

ghost pushed a commit that referenced this pull request Dec 17, 2019
@ghost
ghost force-pushed the subgraph-composition branch from ef64950 to 80ffec0 Compare December 17, 2019 18:42
@ghost

ghost commented Dec 17, 2019

Copy link
Copy Markdown
Author

@Jannis I added the list item in the SUMMARY.md, it felt redundant though

@ghost

ghost commented Dec 17, 2019

Copy link
Copy Markdown
Author

Ahh, the SUMMARY file is for mdBook

@Jannis Jannis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking pretty good! Minor remarks, but you can go ahead and transfer the tasks over to Linear! 👍

Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
Comment thread engineering-plans/0002-subgraph-schema-merging.md Outdated
@lutter

lutter commented Dec 18, 2019

Copy link
Copy Markdown
Contributor

There's one issue here when it comes to querying data from the Store: with prefetching, we sometimes join two tables for a query parents { children { id } } when we get the children. With composition, it is possible that parents and children live in different subgraphs, something that EntityQuery can currently not express. I see two options for making this work:

  1. Rework the prefetch code to avoid joins (we have the data we need from the joins already in memory when we query for children)
  2. Change EntityQuery and the code that runs these queries to allow mixing entities from multiple subgraphs

Since, at some point, we will also want to be able to filter by child fields (parents(where: {children.color: green}) { id }) we should go with the second option, since for that we will need to join parents and children when we look up the parents.

@vercel

vercel Bot commented Jan 14, 2020

Copy link
Copy Markdown

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/graphprotocol/rfcs/dx4p8vuub
✅ Preview: https://rfcs-git-subgraph-composition.graphprotocol1.now.sh

@Jannis

Jannis commented Jan 17, 2020

Copy link
Copy Markdown
Contributor

@lutter Have your concerns been addressed yet? It reads more like it is relevant to query execution rather than merging, i.e. #12 rather than this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engineering-plan This is an Engineering Plan

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants