-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Subgraph Composition: Reading the entities for subgraph as a datasource #5544
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
Conversation
7260289 to
37f8a5f
Compare
c33222c to
41a0f8d
Compare
d6be3d7 to
dd39f66
Compare
87f411b to
10725c2
Compare
dd39f66 to
3aba98f
Compare
10725c2 to
0a89a4e
Compare
| schema.entity_type(&self.entity).unwrap() | ||
| } | ||
|
|
||
| pub fn deserialize_block_number<T: FromEntityData>(self) -> Result<BlockNumber, StoreError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is a temporary implementation, needed at this stage. It will be completely removed in a later PR, so it can be safely ignored here.
| out.push_sql(" from "); | ||
| out.push_sql(self.table.qualified_name.as_str()); | ||
| out.push_sql(" e\n where "); | ||
| // TODO: do we need to care about it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we do. Enabled in a later PR.
| fn read_range_mutable() { | ||
| run_test(|store, writable, deployment| async move { | ||
| let num_entities = read_range(store, writable, deployment, true).await; | ||
| assert_eq!(num_entities, 6) // TODO: fix it - it should be 4 as the range is open |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be reworked later on.
| /// Helper for generating SQL fragments for selecting entities in a specific block range | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub enum EntityBlockRange { | ||
| Mutable(BlockRange), // TODO: check if this is a proper type here (maybe Range<BlockNumber>?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be fine.
store/postgres/src/writable.rs
Outdated
| entity_type: &EntityType, | ||
| block_range: Range<BlockNumber>, | ||
| ) -> Result<BTreeMap<BlockNumber, Vec<Entity>>, StoreError> { | ||
| self.store.get_range(entity_type, block_range) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will return the wrong result if block_range covers blocks that are still in the queue - those aren't in the database yet. You'd need to use the BlockTracker to go through the queue and find entities for those blocks. You also only want to look in the store for the part of block_range that isn't in the queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed it from the Queue and from the Writer and left if only in the WritableStore with additional comment. Would that be acceptable? Alternative is to create ReaderStore struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should do that. The dependent subgraph that is only reading from a source subgraph should not have a WritableStore for the source subgraph at all, since it exposes way more functionality than we need, and it's a good idea to make it clear that we do not want to write to the source subgraph.
That will either mean that you add a new method to ReadStore and implement it for DeploymentStore, or create another trait that only has a get_range method and implement that for DeploymentStore. The SubgraphStore then needs a new method to get such a store for a given deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to go with a new trait since the methods of readStore would need to be implemented in WritableStore too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the WritableStore is a private member of the SourceableStore struct. It is only used for implementing the DeploymentCursorTracker methods of the SourceableStore trait, which replaces ReadStore in this context. I hope that addresses your concerns sufficiently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok finally removed all traces of WritableStore from the SourceableStore. Feel free to review it with the other two PRs. Sorry for the confusion before.
8ce342f to
9acd51e
Compare
6a47d26 to
0d7e4d5
Compare
d6ef813 to
77850be
Compare
81fb399 to
70ca609
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I think this is almost ready to merge, but it would be good to address the redundant passing of (DeploymentHash, SourceableStore) and just use SourceableStore there.
| } | ||
|
|
||
| #[async_trait] | ||
| pub trait SourceableStore: Sync + Send + 'static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I like that this is a very small interface which makes it easy to determine what one of these things could possibly do when it is in use.
graph/src/blockchain/mod.rs
Outdated
| store: impl DeploymentCursorTracker, | ||
| start_blocks: Vec<BlockNumber>, | ||
| source_subgraph_stores: Vec<(DeploymentHash, Arc<dyn ReadStore>)>, | ||
| source_subgraph_stores: Vec<(DeploymentHash, Arc<dyn SourceableStore>)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't necessarily need the DeploymentHash here since SourceableStore.input_schema().id() will return it (and there could be a convenience method on SourceableStore for it)
Actually, not having it explicit lowers the risk that there is a mismatch between the two.
| } | ||
| } | ||
|
|
||
| pub async fn hashes_to_read_store<C: Blockchain>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be renamed now that it returns SourceableStore not ReadStore
| } | ||
|
|
||
| pub fn iter<'a>(&'a self) -> impl Iterator<Item = (&K, &V)> { | ||
| pub fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a K, &'a V)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did rustc start complaining about the lifetimes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Already merged in master as our CI updates automatically to the the latest version of rustc.
c7ca25a to
383bbc9
Compare
0a4a2a6 to
b5af690
Compare
b5af690 to
d2f6352
Compare
f46fff0 to
bd0ab9a
Compare
Contains initial implementation of the database access functionality for fetching a single entity in a range of blocks. Will later be extended to support multiple entities.
TODOs in the code are addressed in later PRs.