-
Notifications
You must be signed in to change notification settings - Fork 1k
Make VID element internal to the Entity #5857
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
Conversation
cee4154
to
eb9d33c
Compare
@@ -740,7 +740,7 @@ lazy_static! { | |||
} | |||
|
|||
/// An entity is represented as a map of attribute names to values. | |||
#[derive(Clone, CacheWeight, PartialEq, Eq, Serialize)] | |||
#[derive(Clone, CacheWeight, Eq, Serialize)] | |||
pub struct Entity(Object<Value>); | |||
|
|||
impl<'a> IntoIterator for &'a Entity { |
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 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)
}
}
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.
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
)
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.
Best I could do is use retain()
. Can we postpone the UT for later moment?
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.
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)
}
}
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.
Beautiful! That removes two copies - the original clone and my retain! BTW added the test.
}; | ||
if len1 != len2 { | ||
return false; | ||
} |
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 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.
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.
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.
This reverts commit 133f79a.
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.
Nice!
Relevant discussion was close and change was implemented
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) |
No description provided.