Derived fields getter #4434

gusinacio · 2023-03-08T16:19:14Z

This PR aims to add a getter for derivedFields. Fixes #4004

Roadmap

add store.loadRelated host function
create query for find_derived
retrive derived entities from the database
create ASC conversion
get derived from queue
optimize the get derived query to filter entities ids
tests

graph/src/components/store/mod.rs

gusinacio · 2023-03-08T22:16:41Z

I splitted and renamed the structs for a more semantic naming and usage.

gusinacio · 2023-03-08T22:23:09Z

@lutter do you think the implementation for the Queue with a Writer::Async is needed? And if so, how do you suggest to implement?

lutter · 2023-03-14T22:31:11Z

@lutter do you think the implementation for the Queue with a Writer::Async is needed? And if so, how do you suggest to implement?

Sorry, I completely missed that. Yes, we do need the implementation for Writer::Async - that is now the main way how changes get written. The implementation Writer::Sync is only there as a safety measure in case we discover bugs with Writer::Async, and we could probably remove it at this point.

From a quick glance at your last commit, it looks like you figured out what needs to happen: the query needs to be run against the store and the result needs to be updated according to the changes still in the queue similar to what get and get_many do.

I'll try and review what you have in the next few days.

gusinacio · 2023-03-14T22:43:05Z

From a quick glance at your last commit, it looks like you figured out what needs to happen: the query needs to be run against the store and the result needs to be updated according to the changes still in the queue similar to what get and get_many do.

Yes, I figured out! The only point of optimization is to filter in the database query the entities ids that we already have from queue so we can save some cpu clocks.

gusinacio · 2023-03-15T15:04:49Z

@lutter I did some optimizations with the query but I would be more secure if I could test it. Do you have any suggestions where I can place the tests for it?

lutter

This all looks very good. There is one issue in writable.rs that needs to be addressed and a few small stylistic comments.

runtime/wasm/src/module/mod.rs

graph/src/data/schema.rs

lutter · 2023-03-16T20:37:09Z

graph/src/data/schema.rs

@@ -643,6 +643,43 @@ impl Schema {
        }
    }

+    pub fn get_type_for_field(&self, key: &LoadRelatedRequest) -> Result<(&str, &str), Error> {


I think it would be clearer here if the second entry in the tuple was a &Field - would be nice to make the first one an ObjectType but that would require serching the schema for that, and that doesn't seem worth the effort.

How is this extensible when we start to use non-derived fields? Is there any &Field that represent the relationship between the parent and the child?

Yes, the difference is that with a derived field the schema looks like

type Parent { children [Child!]! @derivedFrom(field: "parent") }

and for a non-derived field it is

type Parent { children [Child!]! }

In that case, the parent has a column children of either type text[] or bytea[] depending on the type of Child.id

store/postgres/src/writable.rs

store/postgres/src/relational.rs

lutter · 2023-03-16T21:27:27Z

@lutter I did some optimizations with the query but I would be more secure if I could test it. Do you have any suggestions where I can place the tests for it?

I think the best place for tests would be in graph/tests/entity_cache.rs so you can test the entire path of loading derived entities. The tests in there right now use a MockStore, but this test should use the proper test_store::STORE etc. It would have to set up its own subgraph with a schema that uses a derived field, populate it with data and then test loading, probably with some edge cases, e.g., for an entity where the set of related entities is empty. Have a look at the tests in store/postgres/tests for some of the helpers we have to set up test subgraphs.

graph/src/data/schema.rs

lutter

Apart from the small type for derivedFrom, this looks good. I just realized that this PR does not take one other situation into account: the field that is mentioned in the @derivedFrom can also be an array. This document goes through all the possible permutations. But I would also rather address this in a follow-on PR; I believe that taking that into account only requires some small changes to the query generation logic.

gusinacio · 2023-03-17T22:30:50Z

Apart from the small type for derivedFrom, this looks good. I just realized that this PR does not take one other situation into account: the field that is mentioned in the @derivedFrom can also be an array. This document goes through all the possible permutations. But I would also rather address this in a follow-on PR; I believe that taking that into account only requires some small changes to the query generation logic.

wow, I didn't know that. Everything that I know about subgraphs was reading the Official documentation and it doesn't have anything related to @derivedFrom with single entity (I found within the code) and with array of entities.

I'll look through the document you sent and, if it is the case, try to fail gracefully until the follow-on PR.

lutter · 2023-03-20T22:52:23Z

This all looks good now.

@azf20 I forget, what is the process for adding a new host function? I think we need to add a API version 0.0.8 and make sure the new host function is only available with that version?

lutter · 2023-03-20T22:53:03Z

@flametuner can you rebase to latest master?

azf20 · 2023-03-20T23:39:14Z

Nice! Process depends on what the functionality is exactly, I think indeed in this case we probably need a new apiVersion, but cc @leoyvens who can confirm what check we then also need on graph-ts?

gusinacio · 2023-03-21T15:13:54Z

@lutter I added some tests. I found a bug while testing and fixed it in writable.rs. I would like you to review again now that I'm 100% comfortable with the code. I tested 9 edge cases with both the query to the database and the query from async queue.

gusinacio · 2023-03-23T00:28:20Z

@lutter is there anything blocking this PR? Please let me know so I can fix it.

lutter · 2023-03-23T23:07:08Z

@lutter is there anything blocking this PR? Please let me know so I can fix it.

This all looks good - can you rebase once more? I can then merge it.

lutter · 2023-03-24T02:44:13Z

@flametuner I think it needs a little bit of additional work - most of the CI checks are failing

gusinacio · 2023-03-24T02:51:44Z

@flametuner I think it needs a little bit of additional work - most of the CI checks are failing

Rebase didn't update some errors types in my code. Just fixed that.

rename variables to be more meaningful

runtime: fix rebase problem

lutter · 2023-03-29T20:15:33Z

@flametuner Thanks a ton for this PR. Great work!

gusinacio changed the title ~~WIP: Derived getter~~ WIP: Derived fields getter Mar 8, 2023

gusinacio marked this pull request as draft March 8, 2023 16:22

lutter reviewed Mar 8, 2023

View reviewed changes

graph/src/components/store/mod.rs Show resolved Hide resolved

gusinacio force-pushed the derived-getter branch from 259b2be to 173b32a Compare March 14, 2023 22:39

gusinacio force-pushed the derived-getter branch 2 times, most recently from 3d9a5b0 to 734cbb4 Compare March 15, 2023 15:01

gusinacio force-pushed the derived-getter branch from 6b7ae9a to c920ff2 Compare March 15, 2023 20:05

gusinacio changed the title ~~WIP: Derived fields getter~~ Derived fields getter Mar 16, 2023

gusinacio marked this pull request as ready for review March 16, 2023 18:09

gusinacio mentioned this pull request Mar 16, 2023

cli: add derived loader graphprotocol/graph-tooling#1190

Closed

lutter requested changes Mar 16, 2023

View reviewed changes

lutter reviewed Mar 17, 2023

View reviewed changes

graph/src/data/schema.rs Outdated Show resolved Hide resolved

lutter requested changes Mar 17, 2023

View reviewed changes

lutter approved these changes Mar 20, 2023

View reviewed changes

gusinacio force-pushed the derived-getter branch from 705af40 to ee6f225 Compare March 21, 2023 15:24

gusinacio force-pushed the derived-getter branch from 96de2db to 1cbb1ea Compare March 24, 2023 01:45

gusinacio force-pushed the derived-getter branch from d112905 to ceeeb96 Compare March 24, 2023 02:54

gusinacio added 22 commits March 28, 2023 23:53

graph,core,runtime,store: add store.getWhere

0519b20

add asc to array of entity

9661088

store: fix lint

357d2e9

store: add FindDerivedQuery

f32ac45

store,runtime,graph: refact where to derived

ef35514

graph: update EntityMultiKey enum

c54887b

store,runtime: cleanup code

f0b8091

graph: fix missing get_derived on mock

cc16efb

all: cargo format

e2a796e

change to load_related, use entitykey in cache

9ff225f

graph: convert string to entity type

40b7b73

graph,graphql,runtime,store: refact structs

6c46d05

graph, store: add get_derived for queue

f02e393

store: fix problem where database overwrite queue

790bba5

graph: fix mockstore

e005333

store: remove unnecessary mut

14d8bdb

graph,store: add database optimization

74f547b

rename variables to be more meaningful

store: check for length in excluded_keys

a3be3de

graph,runtime,store: fix requested changes

be31715

graph: fix case-sensitive mistake

a20f3d4

graph,store: add tests for load_related

6b8fe6e

runtime,graph: fix warnings and rebase types

196c36e

runtime: fix rebase problem

gusinacio force-pushed the derived-getter branch from ceeeb96 to 196c36e Compare March 29, 2023 15:05

lutter merged commit bed5887 into graphprotocol:master Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Derived fields getter #4434

Derived fields getter #4434

gusinacio commented Mar 8, 2023 •

edited

Loading

gusinacio commented Mar 8, 2023

gusinacio commented Mar 8, 2023

lutter commented Mar 14, 2023

gusinacio commented Mar 14, 2023 •

edited

Loading

gusinacio commented Mar 15, 2023

lutter left a comment

lutter Mar 16, 2023

gusinacio Mar 17, 2023

lutter Mar 17, 2023

lutter commented Mar 16, 2023

lutter left a comment

gusinacio commented Mar 17, 2023

lutter commented Mar 20, 2023

lutter commented Mar 20, 2023

azf20 commented Mar 20, 2023

gusinacio commented Mar 21, 2023

gusinacio commented Mar 23, 2023

lutter commented Mar 23, 2023

lutter commented Mar 24, 2023

gusinacio commented Mar 24, 2023

lutter commented Mar 29, 2023

Derived fields getter #4434

Derived fields getter #4434

Conversation

gusinacio commented Mar 8, 2023 • edited Loading

Roadmap

gusinacio commented Mar 8, 2023

gusinacio commented Mar 8, 2023

lutter commented Mar 14, 2023

gusinacio commented Mar 14, 2023 • edited Loading

gusinacio commented Mar 15, 2023

lutter left a comment

Choose a reason for hiding this comment

lutter Mar 16, 2023

Choose a reason for hiding this comment

gusinacio Mar 17, 2023

Choose a reason for hiding this comment

lutter Mar 17, 2023

Choose a reason for hiding this comment

lutter commented Mar 16, 2023

lutter left a comment

Choose a reason for hiding this comment

gusinacio commented Mar 17, 2023

lutter commented Mar 20, 2023

lutter commented Mar 20, 2023

azf20 commented Mar 20, 2023

gusinacio commented Mar 21, 2023

gusinacio commented Mar 23, 2023

lutter commented Mar 23, 2023

lutter commented Mar 24, 2023

gusinacio commented Mar 24, 2023

lutter commented Mar 29, 2023

gusinacio commented Mar 8, 2023 •

edited

Loading

gusinacio commented Mar 14, 2023 •

edited

Loading