Skip to content

Make VID element internal to the Entity#5857

Merged
encalypto merged 8 commits into
masterfrom
zoran/vid-private
Feb 28, 2025
Merged

Make VID element internal to the Entity#5857
encalypto merged 8 commits into
masterfrom
zoran/vid-private

Conversation

@zorancv

@zorancv zorancv commented Feb 27, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@zorancv
zorancv marked this pull request as ready for review February 27, 2025 19:03
@zorancv
zorancv requested a review from lutter February 27, 2025 19:20
@zorancv zorancv self-assigned this Feb 27, 2025
Comment thread graph/src/data/store/mod.rs
Comment thread graph/src/data/store/mod.rs Outdated
@zorancv zorancv changed the title Make VID entries internal to the entity Make VID element internal to the entity Feb 27, 2025
@zorancv zorancv changed the title Make VID element internal to the entity Make VID element internal to the Entity Feb 27, 2025
Comment thread graph/src/data/store/mod.rs
#[derive(Clone, CacheWeight, Eq, Serialize)]
pub struct Entity(Object<Value>);

impl<'a> IntoIterator for &'a Entity {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this also needs to be changed so that entity.iter() and entity.into_iter() do not show the vid, something like

impl<'a> IntoIterator for &'a Entity {
    type Item = (Word, Value);

    type IntoIter = intern::ObjectOwningIter<Value>;

    fn into_iter(self) -> Self::IntoIter {
        self.0.clone().into_iter().filter(|(k,_)| k != VID_FIELD)
    }
}

@lutter lutter Feb 27, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You should also add unit tests at teh end of the file that show that the various methods behave well when there is a vid (e.g., that iteration and sorted do not include the vid)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Best I could do is use retain(). Can we postpone the UT for later moment?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I just noticed that the original already had that really surprising and unnecessary clone in it. After playing around a little and consulting this, I think this would work and gets rid of the clone, too:

impl<'a> IntoIterator for &'a Entity {
    type Item = (&'a str, &'a Value);

    type IntoIter =
        std::iter::Filter<intern::ObjectIter<'a, Value>, fn(&(&'a str, &'a Value)) -> bool>;

    fn into_iter(self) -> Self::IntoIter {
        (&self.0).into_iter().filter(|(k, _)| *k != VID_FIELD)
    }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Beautiful! That removes two copies - the original clone and my retain! BTW added the test.

Comment thread graph/src/util/intern.rs
};
if len1 != len2 {
return false;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is very minor, but since you need to traverse the list of key/value pairs to determine the length, there's not much value in trying to short-circuit the comparison based on length since you already need to load everything into cache. I would just leave the length-based optimization out and go straight to comparing based on values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On a second thought the size comparison avoids false positive when elements in self are subset of the ones ni other, so I'll keep it.

@zorancv
zorancv requested a review from lutter February 28, 2025 00:30

@lutter lutter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice!

@encalypto
encalypto dismissed incrypto32’s stale review February 28, 2025 22:35

Relevant discussion was close and change was implemented

@encalypto
encalypto merged commit 6bbd0d2 into master Feb 28, 2025
@encalypto

encalypto commented Feb 28, 2025

Copy link
Copy Markdown
Contributor

Merging this as per offline discussion with @fordN in order to test this sooner, given all issues were resolved & PR has been approved. (Leaving the branch just in case it's needed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants