-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Subgraph composition spec version #5782
base: zoran/subgraph-composition-rework-vid-wrap2
Are you sure you want to change the base?
Subgraph composition spec version #5782
Conversation
d8bea9d
to
47176c8
Compare
81f0a57
to
133169c
Compare
961556b
to
a49edb8
Compare
6cb3a0c
to
3d25869
Compare
e05ae82
to
522d7f7
Compare
515fdbc
to
e61452f
Compare
e5d997b
to
c8d8103
Compare
e61452f
to
1cb3401
Compare
7173051
to
46fa0f4
Compare
c8d8103
to
a6b7647
Compare
46fa0f4
to
061b9c7
Compare
a6b7647
to
9fa915b
Compare
GetScope::Store => entity == self.store.get(key).unwrap().map(Arc::new), | ||
GetScope::Store => { | ||
// Release build will never call this function and hence it's OK | ||
// when that implementation is not correct. |
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.
What do you mean by 'not correct'? It seems correct to me, though a little slow because of the clone, but that's fine for debug builds
// Changes the way the VID field is generated. It used to be autoincrement. Now its | ||
// based on block number and the order of the entities in a block. The latter | ||
// represents the write order across all entity types in the subgraph. | ||
pub fn strict_vid_order(&self) -> bool { |
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.
That's the method that I would rename to has_vid_seq
; the main reason is because InputSchema.strict_vid_order
and EntityType.strict_vid_order
mean slightly different things. You could also change the comment to (notice the ///
)
/// Whether the table for this entity type uses a sequence for the `vid` or whether
/// `graph-node` sets them explicitly. See also [`InputSchema.strict_vid_order()`]
|
||
pub fn strict_vid_order(&self) -> bool { | ||
self.inner.spec_version >= SPEC_VERSION_1_3_0 | ||
} |
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 might be a good comment for this method:
/// How the values for the VID field are generated.
/// When this is `false`, this subgraph uses the old way of autoincrementing `vid` in the database.
/// When it is `true`, `graph-node` sets the `vid` explicitly to a number based on block number
/// and the order in which entities are written, and comparing by `vid` will order entities by that order
This PR aims to base
VID
generation onspec_version
so that both old and new way of generatingVID
will work simultaneously in the single binary. Some code that was removed in #5737 is reintroduced here.