Skip to content

Changes to VID generation after rebase with master #5815

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.

Sign up for GitHub

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

Closed
zorancv wants to merge 3 commits into from
Closed

Changes to VID generation after rebase with master #5815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7988c8a
fix wrong merge
Feb 1, 2025
1ae5dd7
fix a test
Feb 2, 2025
46fa0f4
add vid to the list of selected columns
Feb 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion graph/src/components/store/entity_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ impl EntityCache {
) -> Result<Option<Entity>, anyhow::Error> {
match op {
EntityOp::Update(entity) | EntityOp::Overwrite(entity)
if query.matches(key, &entity) =>
if query.matches(key, entity) =>
{
Ok(Some(entity.clone()))
}
Expand Down Expand Up @@ -383,6 +383,7 @@ impl EntityCache {

// The next VID is based on a block number and a sequence within the block
let vid = ((block as i64) << 32) + self.vid_seq as i64;
self.vid_seq += 1;
let mut entity = entity;
let old_vid = entity.set_vid(vid).expect("the vid should be set");
// Make sure that there was no VID previously set for this entity.
Expand All @@ -395,6 +396,8 @@ impl EntityCache {
);
}

self.entity_op(key.clone(), EntityOp::Update(entity));

// The updates we were given are not valid by themselves; force a
// lookup in the database and check again with an entity that merges
// the existing entity with the changes
Expand Down
4 changes: 2 additions & 2 deletions graph/src/data/store/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,9 +914,9 @@ impl Entity {
/// i64 it panics.
pub fn vid(&self) -> i64 {
self.get(VID_FIELD)
.expect("the vid is set")
.expect("the vid must be set")
.as_int8()
.expect("the vid is set to a valid value")
.expect("the vid must be set to a valid value")
}

/// Sets the VID of the entity. The previous one is returned.
Expand Down
5 changes: 4 additions & 1 deletion store/postgres/src/relational/dsl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,10 @@ impl<'a> Table<'a> {
}

match column_names {
AttributeNames::All => cols.extend(self.meta.columns.iter()),
AttributeNames::All => {
cols.extend(self.meta.columns.iter());
cols.push(&*VID_COL);
}
Copy link
Collaborator

@lutter lutter Feb 11, 2025

Choose a reason for hiding this comment

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

Don't we have to include the VID_COL no matter what column_names is?

Copy link
Contributor Author

@zorancv zorancv Feb 12, 2025

Choose a reason for hiding this comment

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

I was not sure as this match side is the only one used. I've changed it to be always be added. Since this is already merged I've addressed it in another PR.

AttributeNames::Select(names) => {
let pk = self.meta.primary_key();
cols.push(pk);
Expand Down
3 changes: 2 additions & 1 deletion store/test-store/tests/postgres/relational.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,8 @@ fn enum_arrays() {
let spectrum = entity! { THINGS_SCHEMA =>
id: "rainbow",
main: "yellow",
all: vec!["yellow", "red", "BLUE"]
all: vec!["yellow", "red", "BLUE"],
vid: 0i64
};

insert_entity(
Expand Down