-
Notifications
You must be signed in to change notification settings - Fork 80
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
MRG: add Manifest::intersect_manifest
to Rust core
#3305
Changes from 37 commits
6b1d1bb
7e73e8f
2896c49
45b1a8f
e8d7999
53bcf02
d467d9f
5f1eef6
eb46ecd
923af44
315dfff
1fb658a
a122982
39816ab
61416fb
3a7abe9
07e3a09
fe75c6c
5fb20fc
2c59010
c8477df
6fee403
0675402
17f50ef
39c140b
16f72c5
2146f30
32839ae
43ee757
31aa378
1594dc4
2465c02
5af5f45
b46565d
b7a9850
9c26752
f2fde5d
f4e89f4
9b9e17e
aa109f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,6 +215,14 @@ | |
assert_eq!(sig.signatures.len(), 1); | ||
Ok(sig) | ||
} | ||
|
||
pub fn intersect_manifest(&self, mf: &Manifest) -> Self { | ||
let manifest = self.manifest.intersect_manifest(mf); | ||
Self { | ||
manifest, | ||
storage: self.storage.clone(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does just cloning the storage mean that we're carrying around all sigs anyway, but we just don't have access to them via the manifest? This is probably best since we avoid having to find and copy sigs individually? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is really a question for @luizirber at the moment. The alternative would involve references and lifetime annotations, I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
To be clear: I think the answer is yes, esp if it's e.g. a I am worried about duplicating memory unnecessarily. I don't really know how cloning a storage works in practice. I don't like the idea of cloning (which presumably would double the Maybe a better approach here would be to have the result of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
} | ||
} | ||
|
||
impl Select for Collection { | ||
|
@@ -233,10 +241,11 @@ | |
use super::Collection; | ||
|
||
use crate::encodings::HashFunctions; | ||
use crate::manifest::Manifest; | ||
use crate::prelude::Select; | ||
use crate::selection::Selection; | ||
use crate::signature::Signature; | ||
use crate::Result; | ||
Check warning on line 248 in src/core/src/collection.rs
|
||
|
||
#[test] | ||
fn sigstore_selection_with_downsample() { | ||
|
@@ -358,6 +367,32 @@ | |
assert_eq!(cl.len(), 0); | ||
} | ||
|
||
#[test] | ||
fn collection_intersect_manifest() { | ||
// load test sigs | ||
let mut filename = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | ||
// four num=500 sigs | ||
filename.push("../../tests/test-data/genome-s11.fa.gz.sig"); | ||
let file = File::open(filename).unwrap(); | ||
let reader = BufReader::new(file); | ||
let sigs: Vec<Signature> = serde_json::from_reader(reader).expect("Loading error"); | ||
assert_eq!(sigs.len(), 4); | ||
// load sigs into collection + select compatible signatures | ||
let cl = Collection::from_sigs(sigs).unwrap(); | ||
// all sigs should remain | ||
assert_eq!(cl.len(), 4); | ||
|
||
// grab first record | ||
let manifest = cl.manifest(); | ||
let record = manifest.iter().next().unwrap().clone(); | ||
let vr = vec![record]; | ||
|
||
// now intersect: | ||
let manifest2 = Manifest::from(vr); | ||
let cl2 = cl.intersect_manifest(&manifest2); | ||
assert_eq!(cl2.len(), 1); | ||
} | ||
|
||
#[test] | ||
fn sigstore_sig_from_record() { | ||
// load test sigs | ||
|
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.
would it be simpler (or complementary) to enable intersect of the collections directly i.e. pass a ref to the Collection (
other
) and then useself.manifest.intersect_manifest(other.manifest)
?I guess it probably doesn't matter as long as we're passing in a reference to either the collection or manifest.
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 comes directly from the
MultiCollection
code use case, where we read a manifest in directly; would be frustrating to have to create aCollection
first.we might be able to coerce a
Collection
into aManifest
, though, which would make it work more smoothly when you do have aCollection
... I'll look!