Subclass testing bug in AdamContext.adamLoad #12
Conversation
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. |
|
All automated tests passed. |
| @@ -107,7 +107,17 @@ class AdamContext(sc: SparkContext) extends Serializable with Logging { | |||
| */ | |||
| def adamLoad[T <% SpecificRecord : Manifest, U <: UnboundRecordFilter] | |||
| (filePath: String, predicate: Option[Class[U]] = None, projection: Option[Schema] = None): RDD[T] = { | |||
| if (filePath.endsWith(".bam") || filePath.endsWith(".sam") && manifest[T].erasure.isInstanceOf[ADAMRecord]) { | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
This commit looks great and the explanation in this comment is great for reviewers. I'm not sure we need to commit it as will likely confuse future developers as they don't have the context. Otherwise, I'm happy to merge this right away.
I'll try to jump over to your Sequence Dictionary pull request soon.
There was a problem hiding this comment.
I'll pull out the comment, and then try using SparkFunSuite below. No rush on the sequence dictionary -- this was a bug that I actually discovered while testing out some of my work on that request, and I figured could become its own separate bug and report. Not a big deal.
See the comment there -- basically, we weren't testing for a subclass relationship between the type parameter T and ADAMRecord in the right way, I think? Added a test to make sure that we can load SAM files correctly.
|
Merged build triggered. |
|
Merged build started. |
|
It should be ready to go, Matt. |
|
Merged build finished. |
|
All automated tests passed. |
|
Thanks, Timothy! |
|
Wait, did you merge this? Or just close it? |
Subclass testing bug in AdamContext.adamLoad
|
I meant to merge this not just close it. |
|
Thanks! :-D |
merge in upstream, use hammerlab.paths.Path
See the comment there -- basically, we weren't testing for a subclass
relationship between the type parameter T and ADAMRecord in the right way,
I think.
Added a test to make sure that we can load SAM files correctly.