Rework generation of the VID by zorancv · Pull Request #5737 · graphprotocol/graph-node

zorancv · 2024-12-18T15:25:08Z

This PR changes the generation of the VID in such a way that entities have an order determined by the order of the blockchain transactions from which they originate, rather than to be ordered by the current implementation of the graph-node and the VID autogenerated by the database. This in turn will ensure that the dependant subgraphs would have a deterministic order of their input entities and it would be consistent across different graph-node deployments producing the same POI.

zorancv · 2025-01-07T15:31:35Z

store/postgres/src/relational/ddl.rs

            Ok(cols)
        }

+        let vid_type = if self.object.is_poi() || !self.object.is_object_type() {


@lutter I am not sure which types could have the VIDs still autogenerated as before. Here I choose POI, interface and aggregate types. Is this correct?

Strictly speaking, for POI it doesn't matter, and there's never instances of interfaces (only objects that implement an interface), but I think for simplicity, it might be best to autogenerate a vid for every kind of entity we deal with.

So you would prefer that all is generated by us rather than from the database? Is there inconsistency concern regarding the aggregation (that one is more involved)?

There is a PR #5770 that disables aggregations entities as source entities for now.

lutter

A general question: we already require that every Entity has an id, if it doesn't, things will fail in funky ways. Wouldn't it be simpler to just treat the vid as another attribute of Entity, i.e., require that entity.get("vid") always returns something instead of keeping it separate from the entity?

graph/src/components/store/entity_cache.rs

lutter · 2025-01-09T00:25:18Z

store/postgres/src/relational/ddl.rs

            Ok(cols)
        }

+        let vid_type = if self.object.is_poi() || !self.object.is_object_type() {


Strictly speaking, for POI it doesn't matter, and there's never instances of interfaces (only objects that implement an interface), but I think for simplicity, it might be best to autogenerate a vid for every kind of entity we deal with.

zorancv · 2025-01-20T16:20:29Z

A general question: we already require that every Entity has an id, if it doesn't, things will fail in funky ways. Wouldn't it be simpler to just treat the vid as another attribute of Entity, i.e., require that entity.get("vid") always returns something instead of keeping it separate from the entity?

@lutter I've decided to go that route. It simplifies the code a lot. Also I've added a check in the EntityCache set() that there was no VID previously set.

Finally when deserializing the entities from the database, I added deserialization of the the VID in addition to the list of fields from InputSchema, in order to keep the presence of the VID consistent in the Entity struct.

Those are two major changes since your last review.

graph/src/components/store/entity_cache.rs

lutter

This looks good. I'd mostly just like to understand better what is happening with the explicit block number for the PoI.

Also, I don't think there are any tests that ensure that the vid's are indeed set by insertion order. It would be good to have a test for the EntityCache that ensures that.

graph/src/data/store/mod.rs

graph/src/data/subgraph/api_version.rs

graph/src/schema/input/mod.rs

store/postgres/src/relational/ddl.rs

store/postgres/src/relational/prune.rs

store/postgres/src/relational_queries.rs

graph/src/components/store/entity_cache.rs

core/src/subgraph/runner.rs

lutter · 2025-01-28T22:50:28Z

graph/src/data/store/mod.rs

+    /// Sets the VID of the entity. The previous one is returned.
+    pub fn set_vid(&mut self, value: i64) -> Result<Option<Value>, InternError> {
+        self.0.insert("vid", value.into())
+    }


One additional thought: it would be great if we could guarantee that an entity is always constructed with the vid set, similar to how that's done for the id. I haven't checked how much work that is, but it would make the various expects sprinkled around here easier to accept.

I like the suggestion but would like to postpone it together with the testing request above for a later stage. WDYT?

The test for the VID order is implemented now: 166c909

I am fine with postponing the enforcement of having a vid

lutter

Nice! I left some minor stylistic comments, would be great to address them, but I am totally fine with merging this.

graph/src/components/store/entity_cache.rs

graph/src/data/subgraph/api_version.rs

store/postgres/src/relational/prune.rs

store/postgres/src/relational_queries.rs

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch from 1bbef27 to 5910d01 Compare December 18, 2024 15:47

zorancv self-assigned this Dec 18, 2024

zorancv added the composition label Dec 18, 2024

zorancv added this to the Subgraph Composition milestone Dec 18, 2024

zorancv force-pushed the zoran/rename-structs branch from 632ef9f to 9658d8c Compare December 18, 2024 18:40

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch from 9e6aa6d to 02ce854 Compare December 18, 2024 18:41

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch from 02ce854 to 62d1e2b Compare January 3, 2025 14:25

zorancv requested a review from lutter January 3, 2025 14:46

zorancv commented Jan 7, 2025

View reviewed changes

lutter requested changes Jan 9, 2025

View reviewed changes

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch 3 times, most recently from 70a0f32 to 6e5b0bc Compare January 16, 2025 16:28

zorancv marked this pull request as draft January 16, 2025 16:42

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch 10 times, most recently from b6f7492 to 640e7d1 Compare January 19, 2025 17:34

zorancv changed the base branch from zoran/rename-structs to master January 19, 2025 17:36

zorancv changed the base branch from master to zoran/rename-structs January 19, 2025 17:37

zorancv marked this pull request as ready for review January 20, 2025 16:20

zorancv changed the base branch from zoran/rename-structs to zoran/subgraph-composition-rework-vid-wrap January 20, 2025 22:56

zorancv changed the base branch from zoran/subgraph-composition-rework-vid-wrap to zoran/subgraph-composition-rework-vid-wrap5 January 20, 2025 22:56

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch from 6feee8e to 14671db Compare January 21, 2025 14:34

zorancv changed the base branch from zoran/rename-structs to zoran/subgraph-composition-rework-vid-wrap5 January 21, 2025 15:12

zorancv closed this Jan 21, 2025

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch from 14671db to 640e7d1 Compare January 21, 2025 15:24

zorancv reopened this Jan 21, 2025

zorancv changed the base branch from zoran/subgraph-composition-rework-vid-wrap5 to zoran/rename-structs January 21, 2025 15:47

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch from 14671db to 640e7d1 Compare January 21, 2025 15:48

zorancv requested a review from lutter January 21, 2025 15:54

zorancv mentioned this pull request Jan 27, 2025

Subgraph composition spec version #5782

Closed

lutter reviewed Jan 28, 2025

View reviewed changes

graph/src/components/store/entity_cache.rs Outdated Show resolved Hide resolved

lutter reviewed Jan 28, 2025

View reviewed changes

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch 5 times, most recently from 961556b to a49edb8 Compare January 29, 2025 16:35

lutter approved these changes Jan 31, 2025

View reviewed changes

incrypto32 force-pushed the zoran/rename-structs branch from 9658d8c to cca4d6f Compare January 31, 2025 13:44

incrypto32 force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch 2 times, most recently from e61452f to 1cb3401 Compare January 31, 2025 15:22

zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch 5 times, most recently from 7173051 to 46fa0f4 Compare February 2, 2025 09:28

Subgraph composition: Rework generation of the VID

061b9c7

incrypto32 force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch from 46fa0f4 to 061b9c7 Compare February 3, 2025 09:10

incrypto32 closed this Feb 25, 2025

incrypto32 deleted the zoran/subgraph-composition-rework-vid-wrap2 branch October 13, 2025 13:40

Comments

Conversation

zorancv commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lutter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zorancv commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lutter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lutter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zorancv commented Dec 18, 2024 •

edited

Loading

zorancv commented Jan 20, 2025 •

edited

Loading